Re: Unreviewed patches

[Michael Snyder <msnyder@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2521 bytes --]

Kazu Hirata wrote:
> 
> Hi Michael,
> 
> Thanks for reviewing the patches.
> 
> > > [RFA] sim/h8300/h8300.c: Fix the handling of bxor.
> > > http://sources.redhat.com/ml/gdb-patches/2003-01/msg00328.html
> >
> > For this one, I find the expression "!!(ea & m)" a bit obscure.
> > How about "(ea & m) != 0)"?
> 
> Sure.  Does this mean the patch is OK to apply with your sugestion?

Yes.

> > > [RFA] sim/h8300/compile.c: Fix the handling of extu.w.
> > > http://sources.redhat.com/ml/gdb-patches/2002-12/msg00685.html
> >
> > When you say "an 8-bit wide register that does not exist",
> > do you mean "that isn't simulated"?  It seems to me that
> > the more correct solution is that breg[] is not big enough.
> > It ought to be at least 24 words, if not 32.
> 
> Well, H8/300H and H8S have 8 32-bit registers, from er0 to er7.  Each
> of them can be treated as two 16-bit wide registers.  That is, er0 is
> split into e0 and r0.  Furthermore, the lower half, r0, can be divided
> into two parts, r0h and r0l.  The organization of one register looks
> like so:
> 
> 31     24 23    16 15     8 7      0
> +--------+--------+--------+--------+
> |  N/A   |  N/A   |  r0h   |  r0l   |
> +--------+--------+--------+--------+
> |       e0        |       r0        |
> +-----------------+-----------------+
> |                er0                |
> +-----------------------------------+
> 
> Now, "extu.w r0" zero-extends the value of r0l and stores the result
> to r0.  The H8 simulator without my patch correctly handles this.  A
> problem arises when the simulator sees "extu.w e0".  The correct
> behavior is to zero-extend the lower half of e0 and store the result
> into e0.  However, GET_B_REG in the simulator has no way to refer to
> the lower half of the e0.  The real hardware does not even have this,
> so I decided to take e0 and clear the upper half of e0, which also
> works for "extu.w r0".  Just take r0 and clear the upper half of r0.

Yes, thanks.  My problem is this: your patch uses a host-order
sign-extend to simulate a target-order sign-extend.  If the host
and target have different byte orders, you lose.  That's probably
why the simulator uses breg[] to fetch bytes, instead of using
wreg and masking.

I suggest that it would be comparatively easy to extend the
breg[] array so that it would cover at least the first three
bytes in the register (and possibly all four, just because
it's no extra effort).  Something like the attached.

Then the code that references breg[] does not need to change.

[-- Attachment #2: tmp.diff --]
[-- Type: text/plain, Size: 884 bytes --]

*************** compile (SIM_DESC sd, int pc)
*** 777,784 ****
  }
  
  
! static unsigned char  *breg[18];
! static unsigned short *wreg[18];
  static unsigned int   *lreg[18];
  
  #define GET_B_REG(X)     *(breg[X])
--- 777,784 ----
  }
  
  
! static unsigned char  *breg[32];
! static unsigned short *wreg[16];
  static unsigned int   *lreg[18];
  
  #define GET_B_REG(X)     *(breg[X])
*************** init_pointers (SIM_DESC sd)
*** 1065,1077 ****
  	  while (p < e)
  	    {
  	      if (*p == 0x22)
- 		{
  		  breg[i] = p;
- 		}
  	      if (*p == 0x33)
- 		{
  		  breg[i + 8] = p;
! 		}
  	      p++;
  	    }
  
--- 1065,1077 ----
  	  while (p < e)
  	    {
  	      if (*p == 0x22)
  		  breg[i] = p;
  	      if (*p == 0x33)
  		  breg[i + 8] = p;
! 	      if (*p == 0x11)
! 		breg[i + 16] = p;
! 	      if (*p == 0x00)
! 		breg[i + 24] = p;
  	      p++;
  	    }