Unreviewed patches

Unreviewed patches

[Kazu Hirata]

Hi,

If somebody could review these patches, that would be greatly
appreciated.

[RFA] sim/h8300/h8300.c: Fix the handling of bxor.
http://sources.redhat.com/ml/gdb-patches/2003-01/msg00328.html

[RFA] sim/h8300/compile.c: Fix the handling of extu.w.
http://sources.redhat.com/ml/gdb-patches/2002-12/msg00685.html

Kazu Hirata

Re: Unreviewed patches

[Michael Snyder]

Kazu Hirata wrote:
> 
> Hi,
> 
> If somebody could review these patches, that would be greatly
> appreciated.
> 
> [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)"?


> [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.

What do you think?

Michael

Re: Unreviewed patches

[Kazu Hirata]

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?

> > [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.

I hope this clarifies the motivation behind the patch.

Regards,

Kazu Hirata

Re: Unreviewed patches

[Michael Snyder]

[-- 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++;
  	    }
  

Re: Unreviewed patches

[Kazu Hirata]

Hi Michael,

> 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.

I understand your concern.  But then if GET_W_REG may give a
byte-swapped value, would the following be endian unsafe?

	case O (O_ADD, SW):
	  rd = GET_W_REG (code->dst.reg);
	  ea = fetch (&code->src);
	  res = rd + ea;
	  goto alu16;

The addition is done in host-order.

Kazu Hirata

Re: Unreviewed patches

[Michael Snyder]

Kazu Hirata wrote:
> 
> Hi Michael,
> 
> > 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.
> 
> I understand your concern.  But then if GET_W_REG may give a
> byte-swapped value, would the following be endian unsafe?
> 
>         case O (O_ADD, SW):
>           rd = GET_W_REG (code->dst.reg);
>           ea = fetch (&code->src);
>           res = rd + ea;
>           goto alu16;
> 
> The addition is done in host-order.

I don't know, other than to say that it evidently works.
However in this case we are fetching a half-word as a half-word.
In the other case, we are fetching a byte as a half-word, and
assuming (incorrectly, I think) that the byte  we actually want
will be in the lower half of the half-word.

If you look at the existing code for initializing breg, 
it obviously goes to some effort to avoid making that assumption.

Re: Unreviewed patches

[Kazu Hirata]

Hi Michael,

> > I understand your concern.  But then if GET_W_REG may give a
> > byte-swapped value, would the following be endian unsafe?
> > 
> >         case O (O_ADD, SW):
> >           rd = GET_W_REG (code->dst.reg);
> >           ea = fetch (&code->src);
> >           res = rd + ea;
> >           goto alu16;
> > 
> > The addition is done in host-order.
> 
> I don't know, other than to say that it evidently works.  However in
> this case we are fetching a half-word as a half-word.  In the other
> case, we are fetching a byte as a half-word, and assuming
> (incorrectly, I think) that the byte we actually want will be in the
> lower half of the half-word.

Well, not quite.  In *both* cases (O_ADD and O_EXTU), we assume that

  (GET_W_REG(0) & 0xff) == r0l

Otherwise, the instructions like addition, subtraction, and shift
wouldn't work.  If you think of extu.w as a special case of and.w,
where you clear off the upper half by anding with 0x00ff, my patch
should be correct.

What about something like the attached patch?  If every wreg[i] is set
to some useful value, that means that both of

  if (*q == 0x2233)

and

  if (*q == 0x0011)

triggered for every i.  0x2233 is exactly the lower half of
0x00112233.  Likewise, 0x0011 is exactly the upper half of 0x00112233.
This means that wreg[i] can access either the lower or upper half of a
32-bit register without the byte swap.  In turn, this means that
GET_W_REG is guaranteed to return a non-swapped value.  If wreg[i] is
NULL, the simulator won't work, so just abort().

Kazu Hirata

Index: compile.c
===================================================================
RCS file: /cvs/src/src/sim/h8300/compile.c,v
retrieving revision 1.21
diff -u -6 -r1.21 compile.c
--- compile.c	1 Feb 2003 03:00:14 -0000	1.21
+++ compile.c	3 Feb 2003 23:17:16 -0000
@@ -750,24 +750,27 @@
 	      if (*p == 0x33)
 		{
 		  breg[i + 8] = p;
 		}
 	      p++;
 	    }
+	  wreg[i] = wreg[i + 8] = 0;
 	  while (q < u)
 	    {
 	      if (*q == 0x2233)
 		{
 		  wreg[i] = q;
 		}
 	      if (*q == 0x0011)
 		{
 		  wreg[i + 8] = q;
 		}
 	      q++;
 	    }
+	  if (wreg[i] == 0 || wreg[i + 8] == 0)
+	    abort ();
 	  cpu.regs[i] = 0;
 	  lreg[i] = &cpu.regs[i];
 	}
 
       lreg[8] = &cpu.regs[8];
 
@@ -1603,23 +1606,23 @@
 	    else
 	      nz = 0;
 	    SET_L_REG (code->dst.reg, (rd & 0xffff) | (tmp << 16));
 	    goto next;
 	  }
 	case O (O_EXTS, SW):
-	  rd = GET_B_REG (code->src.reg + 8) & 0xff; /* Yes, src, not dst.  */
+	  rd = GET_W_REG (code->src.reg) & 0xff; /* Yes, src, not dst.  */
 	  ea = rd & 0x80 ? -256 : 0;
 	  res = rd + ea;
 	  goto log16;
 	case O (O_EXTS, SL):
 	  rd = GET_W_REG (code->src.reg) & 0xffff;
 	  ea = rd & 0x8000 ? -65536 : 0;
 	  res = rd + ea;
 	  goto log32;
 	case O (O_EXTU, SW):
-	  rd = GET_B_REG (code->src.reg + 8) & 0xff;
+	  rd = GET_W_REG (code->src.reg) & 0xff;
 	  ea = 0;
 	  res = rd + ea;
 	  goto log16;
 	case O (O_EXTU, SL):
 	  rd = GET_W_REG (code->src.reg) & 0xffff;
 	  ea = 0;

Re: Unreviewed patches

[Michael Snyder]

Kazu Hirata wrote:
> 
> Hi Michael,
> 
> > > I understand your concern.  But then if GET_W_REG may give a
> > > byte-swapped value, would the following be endian unsafe?
> > >
> > >         case O (O_ADD, SW):
> > >           rd = GET_W_REG (code->dst.reg);
> > >           ea = fetch (&code->src);
> > >           res = rd + ea;
> > >           goto alu16;
> > >
> > > The addition is done in host-order.
> >
> > I don't know, other than to say that it evidently works.  However in
> > this case we are fetching a half-word as a half-word.  In the other
> > case, we are fetching a byte as a half-word, and assuming
> > (incorrectly, I think) that the byte we actually want will be in the
> > lower half of the half-word.
> 
> Well, not quite.  In *both* cases (O_ADD and O_EXTU), we assume that
> 
>   (GET_W_REG(0) & 0xff) == r0l
> 
> Otherwise, the instructions like addition, subtraction, and shift
> wouldn't work.  If you think of extu.w as a special case of and.w,
> where you clear off the upper half by anding with 0x00ff, my patch
> should be correct.
> 
> What about something like the attached patch?  If every wreg[i] is set
> to some useful value, that means that both of
> 
>   if (*q == 0x2233)
> 
> and
> 
>   if (*q == 0x0011)
> 
> triggered for every i.  0x2233 is exactly the lower half of
> 0x00112233.  Likewise, 0x0011 is exactly the upper half of 0x00112233.
> This means that wreg[i] can access either the lower or upper half of a
> 32-bit register without the byte swap.  In turn, this means that
> GET_W_REG is guaranteed to return a non-swapped value.  If wreg[i] is
> NULL, the simulator won't work, so just abort().
> 
> Kazu Hirata

All right.
 
> Index: compile.c
> ===================================================================
> RCS file: /cvs/src/src/sim/h8300/compile.c,v
> retrieving revision 1.21
> diff -u -6 -r1.21 compile.c
> --- compile.c   1 Feb 2003 03:00:14 -0000       1.21
> +++ compile.c   3 Feb 2003 23:17:16 -0000
> @@ -750,24 +750,27 @@
>               if (*p == 0x33)
>                 {
>                   breg[i + 8] = p;
>                 }
>               p++;
>             }
> +         wreg[i] = wreg[i + 8] = 0;
>           while (q < u)
>             {
>               if (*q == 0x2233)
>                 {
>                   wreg[i] = q;
>                 }
>               if (*q == 0x0011)
>                 {
>                   wreg[i + 8] = q;
>                 }
>               q++;
>             }
> +         if (wreg[i] == 0 || wreg[i + 8] == 0)
> +           abort ();
>           cpu.regs[i] = 0;
>           lreg[i] = &cpu.regs[i];
>         }
> 
>        lreg[8] = &cpu.regs[8];
> 
> @@ -1603,23 +1606,23 @@
>             else
>               nz = 0;
>             SET_L_REG (code->dst.reg, (rd & 0xffff) | (tmp << 16));
>             goto next;
>           }
>         case O (O_EXTS, SW):
> -         rd = GET_B_REG (code->src.reg + 8) & 0xff; /* Yes, src, not dst.  */
> +         rd = GET_W_REG (code->src.reg) & 0xff; /* Yes, src, not dst.  */
>           ea = rd & 0x80 ? -256 : 0;
>           res = rd + ea;
>           goto log16;
>         case O (O_EXTS, SL):
>           rd = GET_W_REG (code->src.reg) & 0xffff;
>           ea = rd & 0x8000 ? -65536 : 0;
>           res = rd + ea;
>           goto log32;
>         case O (O_EXTU, SW):
> -         rd = GET_B_REG (code->src.reg + 8) & 0xff;
> +         rd = GET_W_REG (code->src.reg) & 0xff;
>           ea = 0;
>           res = rd + ea;
>           goto log16;
>         case O (O_EXTU, SL):
>           rd = GET_W_REG (code->src.reg) & 0xffff;
>           ea = 0;

Unreviewed patches

[Kazu Hirata]

Hi,

Could somebody review the following patches?

[patch] Check watchpoint resources before enabling a watchpoint.
http://sourceware.org/ml/gdb-patches/2007-09/msg00409.html

[patch] m68k-tdep.c: Rename %mbb to %mbo.
http://sourceware.org/ml/gdb-patches/2007-09/msg00383.html

[patch] Turn on inaccessible-by-default by default.
http://sourceware.org/ml/gdb-patches/2007-09/msg00386.html
  Eli Zaretskii has approved the documentation part of this patch.

[patch] lib/gdb.exp: Print out $loadtimeout instead of $timeout.
http://sourceware.org/ml/gdb-patches/2007-10/msg00284.html

I just posted the last one, but I included it here for completeness.

Thanks,

Kazu Hirata

Unreviewed patches

[Kazu Hirata]

Hi,

Could somebody review the following patches?

[patch] Check watchpoint resources before enabling a watchpoint.
http://sourceware.org/ml/gdb-patches/2007-09/msg00409.html

[patch] Turn on inaccessible-by-default by default.
http://sourceware.org/ml/gdb-patches/2007-09/msg00386.html
  Eli Zaretskii has approved the documentation part of this patch.

Thanks,

Kazu Hirata

Unreviewed patches

[Kazu Hirata]

Hi,

If somebody could review these patches, that would be greatly
appreciated.

[RFA] sim/h8300/h8300.c: Fix the handling of bxor.
http://sources.redhat.com/ml/gdb-patches/2003-01/msg00328.html

[RFA] sim/h8300/compile.c: Fix the handling of extu.w.
http://sources.redhat.com/ml/gdb-patches/2002-12/msg00685.html

[RFA] Fix the spurious failures on H8/300 port.
http://sources.redhat.com/ml/gdb-patches/2002-12/msg00450.html

Kazu Hirata

Unreviewed patches

[Joern Rennecke]

Wed Jun 12 13:20:51 2002  J"orn Rennecke <joern.rennecke@superh.com>

include/gdb:
        * sim-sh.h: Add enum constants for sh[1-4], sh3e, sh3?-dsp.
sim/sh:
        * Makefile (interp.o): Depend on $(srcroot)/include/gdb/sim-sh.h.
        * interp.c: Include "gdb/sim-sh.h".
        (sim_store_register, sim_fetch_register): Use constants defined there.

http://sources.redhat.com/ml/gdb-patches/2002-06/msg00185.html

Thu Jun 13 23:20:33 2002  J"orn Rennecke <joern.rennecke@superh.com>


sim/sh64:
        * arch.c, arch.h, cpu.c, cpu.h, cpuall.h, decode-compact.c: Regenerate.
        * decode-compact.h, decode-media.c, decode-media.h: Likewise.
        * defs-compact.h, defs-media.h, sem-compact-switch.c: Likewise.
        * sem-compact.c, sem-media-switch.c, sem-media.c, sh-desc.c: Likewise.
        * sh-desc.h, sh-opc.h: Likewise.

http://sources.redhat.com/ml/gdb-patches/2002-06/msg00490.html

-- 
--------------------------
SuperH
2430 Aztec West / Almondsbury / BRISTOL / BS32 4AQ
T:+44 1454 462330

Re: Unreviewed patches

[Elena Zannoni]

Joern Rennecke writes:
 > Wed Jun 12 13:20:51 2002  J"orn Rennecke <joern.rennecke@superh.com>
 > 
 > include/gdb:
 >         * sim-sh.h: Add enum constants for sh[1-4], sh3e, sh3?-dsp.

Yes, sorry for the delay.

Is there any real reason to not just have one enum, which included the
dsp registers as well? I mean, if the simulator didn't reuse register
numbers for FP regs and DPS regs, then you could get rid of this ugly
code in interp.c:

    else case 44:
      if (target_dsp)
	RE = val;
    else case 45: case 46: case 47: case 48: case 49: case 50:
[...]
    case 26:
      val = target_dsp ? A0 : FI (1);
      break;
[...]
etc. etc.

And with that you could get rid of the target_dsp variable.


 > sim/sh:
 >         * Makefile (interp.o): Depend on $(srcroot)/include/gdb/sim-sh.h.
 >         * interp.c: Include "gdb/sim-sh.h".
 >         (sim_store_register, sim_fetch_register): Use constants defined there.
 > 
 > http://sources.redhat.com/ml/gdb-patches/2002-06/msg00185.html
 > 
 > Thu Jun 13 23:20:33 2002  J"orn Rennecke <joern.rennecke@superh.com>
 > 


I think the changes below depend on the cgen patches being accepted first.

Elena


 > 
 > sim/sh64:
 >         * arch.c, arch.h, cpu.c, cpu.h, cpuall.h, decode-compact.c: Regenerate.
 >         * decode-compact.h, decode-media.c, decode-media.h: Likewise.
 >         * defs-compact.h, defs-media.h, sem-compact-switch.c: Likewise.
 >         * sem-compact.c, sem-media-switch.c, sem-media.c, sh-desc.c: Likewise.
 >         * sh-desc.h, sh-opc.h: Likewise.
 > 
 > http://sources.redhat.com/ml/gdb-patches/2002-06/msg00490.html
 > 
 > -- 
 > --------------------------
 > SuperH
 > 2430 Aztec West / Almondsbury / BRISTOL / BS32 4AQ
 > T:+44 1454 462330

Re: Unreviewed patches

[Joern Rennecke]

Elena Zannoni wrote:
> 
> Joern Rennecke writes:
>  > Wed Jun 12 13:20:51 2002  J"orn Rennecke <joern.rennecke@superh.com>
>  >
>  > include/gdb:
>  >         * sim-sh.h: Add enum constants for sh[1-4], sh3e, sh3?-dsp.
> 
> Yes, sorry for the delay.
> 
> Is there any real reason to not just have one enum, which included the
> dsp registers as well?

Yes.  The values are supposed to stay the same, to retain compatibility.
Besides, you would need to re-design parts of the simulator to remove
the overlaps.

> 	                 I mean, if the simulator didn't reuse register
> numbers for FP regs and DPS regs, then you could get rid of this ugly
> code in interp.c:
> 
>     else case 44:
>       if (target_dsp)
>         RE = val;
>     else case 45: case 46: case 47: case 48: case 49: case 50:
> [...]
>     case 26:
>       val = target_dsp ? A0 : FI (1);
>       break;
> [...]
> etc. etc.
> 
> And with that you could get rid of the target_dsp variable.

Definitely not.  The same 0xfxxx opcodes mean different things to sh3-dsp
and sh3e.
Using the same register number for fpscr and dsr is natural, since the same
opcodes are used to load/store dsr on sh3-dsp and fpscr on sh3e.

> I think the changes below depend on the cgen patches being accepted first.

They have.
	
-- 
--------------------------
SuperH
2430 Aztec West / Almondsbury / BRISTOL / BS32 4AQ
T:+44 1454 462330

Re: Unreviewed patches

[Elena Zannoni]

Joern Rennecke writes:
 > Elena Zannoni wrote:
 > > 
 > > Joern Rennecke writes:
 > >  > Wed Jun 12 13:20:51 2002  J"orn Rennecke <joern.rennecke@superh.com>
 > >  >
 > >  > include/gdb:
 > >  >         * sim-sh.h: Add enum constants for sh[1-4], sh3e, sh3?-dsp.
 > > 
 > > Yes, sorry for the delay.
 > > 
 > > Is there any real reason to not just have one enum, which included the
 > > dsp registers as well?
 > 
 > Yes.  The values are supposed to stay the same, to retain compatibility.
 > Besides, you would need to re-design parts of the simulator to remove
 > the overlaps.
 > 


OK, I see what you are saying, but I don't think that removing the overlaps
from the gdb<->sim interface necessarily forces you to remove the overlaps
from inside the simulator.
It just cleans up the sim_fetch_register and sim_store_register code.

I am not sure I understand your claim about compatibility. You mean
older gdb's with new sims? That cannot happen. Or you mean the layout
of the registers that gdb has internally? That is staying the same
(and the 'g' packet layout stays the same as well) The thing you need
is to define a 'translation' step in gdb between gdb and sim register
numbering schemes.  Much the same as dwarf2_reg_to_regnum & friends do
between gcc and gdb.

 > > 	                 I mean, if the simulator didn't reuse register
 > > numbers for FP regs and DPS regs, then you could get rid of this ugly
 > > code in interp.c:
 > > 
 > >     else case 44:
 > >       if (target_dsp)
 > >         RE = val;
 > >     else case 45: case 46: case 47: case 48: case 49: case 50:
 > > [...]
 > >     case 26:
 > >       val = target_dsp ? A0 : FI (1);
 > >       break;
 > > [...]
 > > etc. etc.
 > > 
 > > And with that you could get rid of the target_dsp variable.
 > 
 > Definitely not.  The same 0xfxxx opcodes mean different things to sh3-dsp
 > and sh3e.
 > Using the same register number for fpscr and dsr is natural, since the same
 > opcodes are used to load/store dsr on sh3-dsp and fpscr on sh3e.
 > 

Ok, I see that the variable is used in the simulation of those instructions.
But that bears no relation with the interface we are trying to define here.

 > > I think the changes below depend on the cgen patches being accepted first.
 > 
 > They have.
 > 	

I don't see any replies to your message to the cgen/gdb-patches lists.

Elena


 > -- 
 > --------------------------
 > SuperH
 > 2430 Aztec West / Almondsbury / BRISTOL / BS32 4AQ
 > T:+44 1454 462330

Re: Unreviewed patches

[Joern Rennecke]

Elena Zannoni wrote:
> OK, I see what you are saying, but I don't think that removing the overlaps
> from the gdb<->sim interface necessarily forces you to remove the overlaps
> from inside the simulator.

You have a point here.  The translation is quite arbitrary.

> It just cleans up the sim_fetch_register and sim_store_register code.

It also changes the interface.

> I am not sure I understand your claim about compatibility. You mean
> older gdb's with new sims? That cannot happen. Or you mean the layout

Older gdb's with new sims, or older sims with new gdb's.

Why should it be safe to have different register numbers in the interface
to the same simulator of a slightly different vintage, yet unsafe to
have different meanings for the same register numbers in the sh vs. sh64
simulator? 

>  > > I think the changes below depend on the cgen patches being accepted first.
>  >
>  > They have.
>  >
> 
> I don't see any replies to your message to the cgen/gdb-patches lists.

Still, when you look at ChangeLog and cpu/{sh64-compact.cpu,sh64-media.cpu},
you'll see that Ben Elliston has applied my patch.

-- 
--------------------------
SuperH
2430 Aztec West / Almondsbury / BRISTOL / BS32 4AQ
T:+44 1454 462330

Re: Unreviewed patches

[Elena Zannoni]

Joern Rennecke writes:
 > Elena Zannoni wrote:
 > > OK, I see what you are saying, but I don't think that removing the overlaps
 > > from the gdb<->sim interface necessarily forces you to remove the overlaps
 > > from inside the simulator.
 > 
 > You have a point here.  The translation is quite arbitrary.
 > 
 > > It just cleans up the sim_fetch_register and sim_store_register code.
 > 
 > It also changes the interface.
 > 
 > > I am not sure I understand your claim about compatibility. You mean
 > > older gdb's with new sims? That cannot happen. Or you mean the layout
 > 
 > Older gdb's with new sims, or older sims with new gdb's.
 > 

Hmm, I don't think we ever worried about that. We have broken
compatibility even between gcc and gdb, sometimes. And here the chance
is smaller, given that sim and gdb come from the same repo and are
built and linked together.

 > Why should it be safe to have different register numbers in the interface
 > to the same simulator of a slightly different vintage, yet unsafe to
 > have different meanings for the same register numbers in the sh vs. sh64
 > simulator? 
 > 

I think that this is the opportunity to create a clean(er) interface,
and we should do it right. (I was actually commenting on the sh
numbering, not the sh64 yet).

BTW, there is an example of gdb-to-sim-regnum translation in the d10v:
d10v_ts2_register_sim_regno and d10v_ts3_register_sim_regno.


 > >  > > I think the changes below depend on the cgen patches being accepted first.
 > >  >
 > >  > They have.
 > >  >
 > > 
 > > I don't see any replies to your message to the cgen/gdb-patches lists.
 > 
 > Still, when you look at ChangeLog and cpu/{sh64-compact.cpu,sh64-media.cpu},
 > you'll see that Ben Elliston has applied my patch.
 > 

Oh, yes, I see them. I guess they don't reply with 'accepted' in the
cgen world.
But I cannot approve changes to the sh64 sim.

Elena


 > -- 
 > --------------------------
 > SuperH
 > 2430 Aztec West / Almondsbury / BRISTOL / BS32 4AQ
 > T:+44 1454 462330

Re: Unreviewed patches

[Joern Rennecke]

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

Elena Zannoni wrote:
> Hmm, I don't think we ever worried about that. We have broken
> compatibility even between gcc and gdb, sometimes. And here the chance
> is smaller, given that sim and gdb come from the same repo and are
> built and linked together.

All right, I have appended a patch that uses separate numbers for the sh-dsp
registers.  I've left the other numbers unchanged.

regression tested on i686-px-linux-gnu X sh-elf; the tally before and
after the patch is the same:
# of expected passes            6681
# of unexpected failures        322
# of expected failures          60
# of unresolved testcases       29
# of unsupported tests          7
 
> Oh, yes, I see them. I guess they don't reply with 'accepted' in the
> cgen world.

Actually, he did reply, but it was a personal email.

> But I cannot approve changes to the sh64 sim.

So is the sh64 gdb currently without a maintainer?
	
-- 
--------------------------
SuperH
2430 Aztec West / Almondsbury / BRISTOL / BS32 4AQ
T:+44 1454 462330

[-- Attachment #2: sim-sh-patch-2 --]
[-- Type: text/plain, Size: 21254 bytes --]

Wed Jul  3 21:33:14 2002  J"orn Rennecke <joern.rennecke@superh.com>

include/gdb:
	* sim-sh.h: Add enum constants for sh[1-4], sh3e, sh3?-dsp,
	renumbering the sh-dsp registers to use distinct numbers.
sim/sh:
	* Makefile (interp.o): Depend on $(srcroot)/include/gdb/sim-sh.h.
	* interp.c: Include "gdb/sim-sh.h".
	(sim_store_register, sim_fetch_register): Use constants defined there.
gdb:
	* sh-tydep.c (sh_dsp_register_sim_regno): New function.
	(sh_gdbarch_init): Use it for sh-dsp.

Index: include/gdb/sim-sh.h
===================================================================
RCS file: /cvs/src/src/include/gdb/sim-sh.h,v
retrieving revision 1.1
diff -p -r1.1 sim-sh.h
*** include/gdb/sim-sh.h	10 May 2002 22:53:56 -0000	1.1
--- include/gdb/sim-sh.h	3 Jul 2002 19:46:29 -0000
***************
*** 1,5 ****
  /* This file defines the interface between the sh simulator and gdb.
!    Copyright (C) 2002 Free Software Foundation, Inc.
  
  This file is part of GDB.
  
--- 1,5 ----
  /* This file defines the interface between the sh simulator and gdb.
!    Copyright (C) 2000, 2002 Free Software Foundation, Inc.
  
  This file is part of GDB.
  
*************** extern "C" { // }
*** 27,51 ****
  /* The simulator makes use of the following register information. */
  
  enum
!   {
!     SIM_SH64_R0_REGNUM = 0,
!     SIM_SH64_SP_REGNUM = 15,
!     SIM_SH64_PC_REGNUM = 64,
!     SIM_SH64_SR_REGNUM = 65,
!     SIM_SH64_SSR_REGNUM = 66,
!     SIM_SH64_SPC_REGNUM = 67,
!     SIM_SH64_TR0_REGNUM = 68,
!     SIM_SH64_FPCSR_REGNUM = 76,
!     SIM_SH64_FR0_REGNUM = 77
!   };
  
  enum
!   {
!     SIM_SH64_NR_REGS = 141,  /* total number of architectural registers */
!     SIM_SH64_NR_R_REGS = 64, /* number of general registers */
!     SIM_SH64_NR_TR_REGS = 8, /* number of target registers */
!     SIM_SH64_NR_FP_REGS = 64 /* number of floating point registers */
!   };
  
  #ifdef __cplusplus
  }
--- 27,158 ----
  /* The simulator makes use of the following register information. */
  
  enum
! {
!   SIM_SH_R0_REGNUM = 0,
!   SIM_SH_R1_REGNUM,
!   SIM_SH_R2_REGNUM,
!   SIM_SH_R3_REGNUM,
!   SIM_SH_R4_REGNUM,
!   SIM_SH_R5_REGNUM,
!   SIM_SH_R6_REGNUM,
!   SIM_SH_R7_REGNUM,
!   SIM_SH_R8_REGNUM,
!   SIM_SH_R9_REGNUM,
!   SIM_SH_R10_REGNUM,
!   SIM_SH_R11_REGNUM,
!   SIM_SH_R12_REGNUM,
!   SIM_SH_R13_REGNUM,
!   SIM_SH_R14_REGNUM,
!   SIM_SH_R15_REGNUM,
!   SIM_SH_PC_REGNUM,
!   SIM_SH_PR_REGNUM,
!   SIM_SH_GBR_REGNUM,
!   SIM_SH_VBR_REGNUM,
!   SIM_SH_MACH_REGNUM,
!   SIM_SH_MACL_REGNUM,
!   SIM_SH_SR_REGNUM,
!   SIM_SH_FPUL_REGNUM,
!   SIM_SH_FPSCR_REGNUM,
!   SIM_SH_FR0_REGNUM, /* FRn registers: sh3e / sh4 */
!   SIM_SH_FR1_REGNUM,
!   SIM_SH_FR2_REGNUM,
!   SIM_SH_FR3_REGNUM,
!   SIM_SH_FR4_REGNUM,
!   SIM_SH_FR5_REGNUM,
!   SIM_SH_FR6_REGNUM,
!   SIM_SH_FR7_REGNUM,
!   SIM_SH_FR8_REGNUM,
!   SIM_SH_FR9_REGNUM,
!   SIM_SH_FR10_REGNUM,
!   SIM_SH_FR11_REGNUM,
!   SIM_SH_FR12_REGNUM,
!   SIM_SH_FR13_REGNUM,
!   SIM_SH_FR14_REGNUM,
!   SIM_SH_FR15_REGNUM,
!   SIM_SH_SSR_REGNUM, /* sh3{,e,-dsp}, sh4 */
!   SIM_SH_SPC_REGNUM, /* sh3{,e,-dsp}, sh4 */
!   SIM_SH_R0_BANK0_REGNUM, /* SIM_SH_Rn_BANKm_REGNUM: sh3[e] / sh4 */
!   SIM_SH_R1_BANK0_REGNUM,
!   SIM_SH_R2_BANK0_REGNUM,
!   SIM_SH_R3_BANK0_REGNUM,
!   SIM_SH_R4_BANK0_REGNUM,
!   SIM_SH_R5_BANK0_REGNUM,
!   SIM_SH_R6_BANK0_REGNUM,
!   SIM_SH_R7_BANK0_REGNUM,
!   SIM_SH_R0_BANK1_REGNUM,
!   SIM_SH_R1_BANK1_REGNUM,
!   SIM_SH_R2_BANK1_REGNUM,
!   SIM_SH_R3_BANK1_REGNUM,
!   SIM_SH_R4_BANK1_REGNUM,
!   SIM_SH_R5_BANK1_REGNUM,
!   SIM_SH_R6_BANK1_REGNUM,
!   SIM_SH_R7_BANK1_REGNUM,
!   SIM_SH_XF0_REGNUM,
!   SIM_SH_XF1_REGNUM,
!   SIM_SH_XF2_REGNUM,
!   SIM_SH_XF3_REGNUM,
!   SIM_SH_XF4_REGNUM,
!   SIM_SH_XF5_REGNUM,
!   SIM_SH_XF6_REGNUM,
!   SIM_SH_XF7_REGNUM,
!   SIM_SH_XF8_REGNUM,
!   SIM_SH_XF9_REGNUM,
!   SIM_SH_XF10_REGNUM,
!   SIM_SH_XF11_REGNUM,
!   SIM_SH_XF12_REGNUM,
!   SIM_SH_XF13_REGNUM,
!   SIM_SH_XF14_REGNUM,
!   SIM_SH_XF15_REGNUM,
!   SIM_SH_SGR_REGNUM,
!   SIM_SH_DBR_REGNUM,
!   SIM_SH4_NUM_REGS, /* 77 */
! 
!   /* sh[3]-dsp */
!   SIM_SH_DSR_REGNUM,
!   SIM_SH_A0G_REGNUM,
!   SIM_SH_A0_REGNUM,
!   SIM_SH_A1G_REGNUM,
!   SIM_SH_A1_REGNUM,
!   SIM_SH_M0_REGNUM,
!   SIM_SH_M1_REGNUM,
!   SIM_SH_X0_REGNUM,
!   SIM_SH_X1_REGNUM,
!   SIM_SH_Y0_REGNUM,
!   SIM_SH_Y1_REGNUM,
!   SIM_SH_MOD_REGNUM,
!   SIM_SH_RS_REGNUM,
!   SIM_SH_RE_REGNUM,
!   SIM_SH_R0_BANK_REGNUM,
!   SIM_SH_R1_BANK_REGNUM,
!   SIM_SH_R2_BANK_REGNUM,
!   SIM_SH_R3_BANK_REGNUM,
!   SIM_SH_R4_BANK_REGNUM,
!   SIM_SH_R5_BANK_REGNUM,
!   SIM_SH_R6_BANK_REGNUM,
!   SIM_SH_R7_BANK_REGNUM
!   /* 100..127: room for expansion.  */
! };
  
  enum
! {
!   SIM_SH64_R0_REGNUM = 0,
!   SIM_SH64_SP_REGNUM = 15,
!   SIM_SH64_PC_REGNUM = 64,
!   SIM_SH64_SR_REGNUM = 65,
!   SIM_SH64_SSR_REGNUM = 66,
!   SIM_SH64_SPC_REGNUM = 67,
!   SIM_SH64_TR0_REGNUM = 68,
!   SIM_SH64_FPCSR_REGNUM = 76,
!   SIM_SH64_FR0_REGNUM = 77
! };
! 
! enum
! {
!   SIM_SH64_NR_REGS = 141,  /* total number of architectural registers */
!   SIM_SH64_NR_R_REGS = 64, /* number of general registers */
!   SIM_SH64_NR_TR_REGS = 8, /* number of target registers */
!   SIM_SH64_NR_FP_REGS = 64 /* number of floating point registers */
! };
  
  #ifdef __cplusplus
  }
Index: sim/sh/interp.c
===================================================================
RCS file: /cvs/src/src/sim/sh/interp.c,v
retrieving revision 1.6
diff -p -r1.6 interp.c
*** sim/sh/interp.c	18 Jun 2002 15:54:44 -0000	1.6
--- sim/sh/interp.c	3 Jul 2002 19:46:29 -0000
***************
*** 29,34 ****
--- 29,35 ----
  #include "bfd.h"
  #include "gdb/callback.h"
  #include "gdb/remote-sim.h"
+ #include "gdb/sim-sh.h"
  
  /* This file is local - if newlib changes, then so should this.  */
  #include "syscall.h"
*************** sim_store_register (sd, rn, memory, leng
*** 1790,1887 ****
    val = swap (* (int *)memory);
    switch (rn)
      {
!     case  0: case  1: case  2: case  3: case  4: case  5: case  6: case  7:
!     case  8: case  9: case 10: case 11: case 12: case 13: case 14: case 15:
        saved_state.asregs.regs[rn] = val;
        break;
!     case 16:
        saved_state.asregs.pc = val;
        break;
!     case 17:
        PR = val;
        break;
!     case 18:
        GBR = val;
        break;
!     case 19:
        VBR = val;
        break;
!     case 20:
        MACH = val;
        break;
!     case 21:
        MACL = val;
        break;
!     case 22:
        SET_SR (val);
        break;
!     case 23:
        FPUL = val;
        break;
!     case 24:
        SET_FPSCR (val);
        break;
!     case 25:
!       if (target_dsp)
! 	A0G = val;
!     else case 26:
!       if (target_dsp)
! 	A0 = val;
!     else case 27:
!       if (target_dsp)
! 	A1G = val;
!     else case 28:
!       if (target_dsp)
! 	A1 = val;
!     else case 29:
!       if (target_dsp)
! 	M0 = val;
!     else case 30:
!       if (target_dsp)
! 	M1 = val;
!     else case 31:
!       if (target_dsp)
! 	X0 = val;
!     else case 32:
!       if (target_dsp)
! 	X1 = val;
!     else case 33:
!       if (target_dsp)
! 	Y0 = val;
!     else case 34:
!       if (target_dsp)
! 	Y1 = val;
!     else case 40:
!       if (target_dsp)
! 	SET_MOD (val);
!     else case 35: case 36: case 37: case 38: case 39:
! 	SET_FI (rn - 25, val);
        break;
!     case 41:
        SSR = val;
        break;
!     case 42:
        SPC = val;
        break;
      /* The rn_bank idiosyncracies are not due to hardware differences, but to
         a weird aliasing naming scheme for sh3 / sh3e / sh4.  */
!     case 43:
!       if (target_dsp)
! 	RS = val;
!     else case 44:
!       if (target_dsp)
! 	RE = val;
!     else case 45: case 46: case 47: case 48: case 49: case 50:
        if (SR_MD && SR_RB)
! 	Rn_BANK (rn - 43) = val;
        else
! 	saved_state.asregs.regs[rn - 43] = val;
        break;
!     case 51: case 52: case 53: case 54: case 55: case 56: case 57: case 58:
!       if (target_dsp || ! SR_MD || ! SR_RB)
! 	SET_Rn_BANK (rn - 51, val);
        else
! 	saved_state.asregs.regs[rn - 51] = val;
        break;
      default:
        return 0;
--- 1791,1912 ----
    val = swap (* (int *)memory);
    switch (rn)
      {
!     case SIM_SH_R0_REGNUM: case SIM_SH_R1_REGNUM: case SIM_SH_R2_REGNUM:
!     case SIM_SH_R3_REGNUM: case SIM_SH_R4_REGNUM: case SIM_SH_R5_REGNUM:
!     case SIM_SH_R6_REGNUM: case SIM_SH_R7_REGNUM: case SIM_SH_R8_REGNUM:
!     case SIM_SH_R9_REGNUM: case SIM_SH_R10_REGNUM: case SIM_SH_R11_REGNUM:
!     case SIM_SH_R12_REGNUM: case SIM_SH_R13_REGNUM: case SIM_SH_R14_REGNUM:
!     case SIM_SH_R15_REGNUM:
        saved_state.asregs.regs[rn] = val;
        break;
!     case SIM_SH_PC_REGNUM:
        saved_state.asregs.pc = val;
        break;
!     case SIM_SH_PR_REGNUM:
        PR = val;
        break;
!     case SIM_SH_GBR_REGNUM:
        GBR = val;
        break;
!     case SIM_SH_VBR_REGNUM:
        VBR = val;
        break;
!     case SIM_SH_MACH_REGNUM:
        MACH = val;
        break;
!     case SIM_SH_MACL_REGNUM:
        MACL = val;
        break;
!     case SIM_SH_SR_REGNUM:
        SET_SR (val);
        break;
!     case SIM_SH_FPUL_REGNUM:
        FPUL = val;
        break;
!     case SIM_SH_FPSCR_REGNUM:
        SET_FPSCR (val);
        break;
!     case SIM_SH_FR0_REGNUM: case SIM_SH_FR1_REGNUM: case SIM_SH_FR2_REGNUM:
!     case SIM_SH_FR3_REGNUM: case SIM_SH_FR4_REGNUM: case SIM_SH_FR5_REGNUM:
!     case SIM_SH_FR6_REGNUM: case SIM_SH_FR7_REGNUM: case SIM_SH_FR8_REGNUM:
!     case SIM_SH_FR9_REGNUM: case SIM_SH_FR10_REGNUM: case SIM_SH_FR11_REGNUM:
!     case SIM_SH_FR12_REGNUM: case SIM_SH_FR13_REGNUM: case SIM_SH_FR14_REGNUM:
!     case SIM_SH_FR15_REGNUM:
!       SET_FI (rn - SIM_SH_FR0_REGNUM, val);
        break;
!     case SIM_SH_DSR_REGNUM:
!       DSR = val;
!       break;
!     case SIM_SH_A0G_REGNUM:
!       A0G = val;
!       break;
!     case SIM_SH_A0_REGNUM:
!       A0 = val;
!       break;
!     case SIM_SH_A1G_REGNUM:
!       A1G = val;
!       break;
!     case SIM_SH_A1_REGNUM:
!       A1 = val;
!       break;
!     case SIM_SH_M0_REGNUM:
!       M0 = val;
!       break;
!     case SIM_SH_M1_REGNUM:
!       M1 = val;
!       break;
!     case SIM_SH_X0_REGNUM:
!       X0 = val;
!       break;
!     case SIM_SH_X1_REGNUM:
!       X1 = val;
!       break;
!     case SIM_SH_Y0_REGNUM:
!       Y0 = val;
!       break;
!     case SIM_SH_Y1_REGNUM:
!       Y1 = val;
!       break;
!     case SIM_SH_MOD_REGNUM:
!       SET_MOD (val);
!       break;
!     case SIM_SH_RS_REGNUM:
!       RS = val;
!       break;
!     case SIM_SH_RE_REGNUM:
!       RE = val;
!       break;
!     case SIM_SH_SSR_REGNUM:
        SSR = val;
        break;
!     case SIM_SH_SPC_REGNUM:
        SPC = val;
        break;
      /* The rn_bank idiosyncracies are not due to hardware differences, but to
         a weird aliasing naming scheme for sh3 / sh3e / sh4.  */
!     case SIM_SH_R0_BANK0_REGNUM: case SIM_SH_R1_BANK0_REGNUM:
!     case SIM_SH_R2_BANK0_REGNUM: case SIM_SH_R3_BANK0_REGNUM:
!     case SIM_SH_R4_BANK0_REGNUM: case SIM_SH_R5_BANK0_REGNUM:
!     case SIM_SH_R6_BANK0_REGNUM: case SIM_SH_R7_BANK0_REGNUM:
        if (SR_MD && SR_RB)
! 	Rn_BANK (rn - SIM_SH_R0_BANK0_REGNUM) = val;
        else
! 	saved_state.asregs.regs[rn - SIM_SH_R0_BANK0_REGNUM] = val;
        break;
!     case SIM_SH_R0_BANK1_REGNUM: case SIM_SH_R1_BANK1_REGNUM:
!     case SIM_SH_R2_BANK1_REGNUM: case SIM_SH_R3_BANK1_REGNUM:
!     case SIM_SH_R4_BANK1_REGNUM: case SIM_SH_R5_BANK1_REGNUM:
!     case SIM_SH_R6_BANK1_REGNUM: case SIM_SH_R7_BANK1_REGNUM:
!       if (SR_MD && SR_RB)
! 	saved_state.asregs.regs[rn - SIM_SH_R0_BANK1_REGNUM] = val;
        else
! 	Rn_BANK (rn - SIM_SH_R0_BANK1_REGNUM) = val;
!       break;
!     case SIM_SH_R0_BANK_REGNUM: case SIM_SH_R1_BANK_REGNUM:
!     case SIM_SH_R2_BANK_REGNUM: case SIM_SH_R3_BANK_REGNUM:
!     case SIM_SH_R4_BANK_REGNUM: case SIM_SH_R5_BANK_REGNUM:
!     case SIM_SH_R6_BANK_REGNUM: case SIM_SH_R7_BANK_REGNUM:
!       SET_Rn_BANK (rn - SIM_SH_R0_BANK_REGNUM, val);
        break;
      default:
        return 0;
*************** sim_fetch_register (sd, rn, memory, leng
*** 1901,1996 ****
    init_pointers ();
    switch (rn)
      {
!     case  0: case  1: case  2: case  3: case  4: case  5: case  6: case  7:
!     case  8: case  9: case 10: case 11: case 12: case 13: case 14: case 15:
        val = saved_state.asregs.regs[rn];
        break;
!     case 16:
        val = saved_state.asregs.pc;
        break;
!     case 17:
        val = PR;
        break;
!     case 18:
        val = GBR;
        break;
!     case 19:
        val = VBR;
        break;
!     case 20:
        val = MACH;
        break;
!     case 21:
        val = MACL;
        break;
!     case 22:
        val = GET_SR ();
        break;
!     case 23:
        val = FPUL;
        break;
!     case 24:
        val = GET_FPSCR ();
        break;
!     case 25:
!       val = target_dsp ? SEXT (A0G) : FI (0);
        break;
!     case 26:
!       val = target_dsp ? A0 : FI (1);
        break;
!     case 27:
!       val = target_dsp ? SEXT (A1G) : FI (2);
        break;
!     case 28:
!       val = target_dsp ? A1 : FI (3);
        break;
!     case 29:
!       val = target_dsp ? M0 : FI (4);
        break;
!     case 30:
!       val = target_dsp ? M1 : FI (5);
        break;
!     case 31:
!       val = target_dsp ? X0 : FI (6);
        break;
!     case 32:
!       val = target_dsp ? X1 : FI (7);
        break;
!     case 33:
!       val = target_dsp ? Y0 : FI (8);
        break;
!     case 34:
!       val = target_dsp ? Y1 : FI (9);
        break;
!     case 35: case 36: case 37: case 38: case 39:
!       val = FI (rn - 25);
        break;
!     case 40:
!       val = target_dsp ? MOD : FI (15);
        break;
!     case 41:
        val = SSR;
        break;
!     case 42:
        val = SPC;
        break;
      /* The rn_bank idiosyncracies are not due to hardware differences, but to
         a weird aliasing naming scheme for sh3 / sh3e / sh4.  */
!     case 43:
!       if (target_dsp)
! 	val = RS;
!     else case 44:
!       if (target_dsp)
! 	val = RE;
!     else case 45: case 46: case 47: case 48: case 49: case 50:
! 	val = (SR_MD && SR_RB
! 	       ? Rn_BANK (rn - 43)
! 	       : saved_state.asregs.regs[rn - 43]);
!       break;
!     case 51: case 52: case 53: case 54: case 55: case 56: case 57: case 58:
!       val = (target_dsp || ! SR_MD || ! SR_RB
! 	     ? Rn_BANK (rn - 51)
! 	     : saved_state.asregs.regs[rn - 51]);
        break;
      default:
        return 0;
--- 1926,2045 ----
    init_pointers ();
    switch (rn)
      {
!     case SIM_SH_R0_REGNUM: case SIM_SH_R1_REGNUM: case SIM_SH_R2_REGNUM:
!     case SIM_SH_R3_REGNUM: case SIM_SH_R4_REGNUM: case SIM_SH_R5_REGNUM:
!     case SIM_SH_R6_REGNUM: case SIM_SH_R7_REGNUM: case SIM_SH_R8_REGNUM:
!     case SIM_SH_R9_REGNUM: case SIM_SH_R10_REGNUM: case SIM_SH_R11_REGNUM:
!     case SIM_SH_R12_REGNUM: case SIM_SH_R13_REGNUM: case SIM_SH_R14_REGNUM:
!     case SIM_SH_R15_REGNUM:
        val = saved_state.asregs.regs[rn];
        break;
!     case SIM_SH_PC_REGNUM:
        val = saved_state.asregs.pc;
        break;
!     case SIM_SH_PR_REGNUM:
        val = PR;
        break;
!     case SIM_SH_GBR_REGNUM:
        val = GBR;
        break;
!     case SIM_SH_VBR_REGNUM:
        val = VBR;
        break;
!     case SIM_SH_MACH_REGNUM:
        val = MACH;
        break;
!     case SIM_SH_MACL_REGNUM:
        val = MACL;
        break;
!     case SIM_SH_SR_REGNUM:
        val = GET_SR ();
        break;
!     case SIM_SH_FPUL_REGNUM:
        val = FPUL;
        break;
!     case SIM_SH_FPSCR_REGNUM:
        val = GET_FPSCR ();
        break;
!     case SIM_SH_FR0_REGNUM: case SIM_SH_FR1_REGNUM: case SIM_SH_FR2_REGNUM:
!     case SIM_SH_FR3_REGNUM: case SIM_SH_FR4_REGNUM: case SIM_SH_FR5_REGNUM:
!     case SIM_SH_FR6_REGNUM: case SIM_SH_FR7_REGNUM: case SIM_SH_FR8_REGNUM:
!     case SIM_SH_FR9_REGNUM: case SIM_SH_FR10_REGNUM: case SIM_SH_FR11_REGNUM:
!     case SIM_SH_FR12_REGNUM: case SIM_SH_FR13_REGNUM: case SIM_SH_FR14_REGNUM:
!     case SIM_SH_FR15_REGNUM:
!       val = FI (rn - SIM_SH_FR0_REGNUM);
        break;
!     case SIM_SH_DSR_REGNUM:
!       val = DSR;
        break;
!     case SIM_SH_A0G_REGNUM:
!       val = SEXT (A0G);
        break;
!     case SIM_SH_A0_REGNUM:
!       val = A0;
        break;
!     case SIM_SH_A1G_REGNUM:
!       val = SEXT (A1G);
        break;
!     case SIM_SH_A1_REGNUM:
!       val = A1;
        break;
!     case SIM_SH_M0_REGNUM:
!       val = M0;
        break;
!     case SIM_SH_M1_REGNUM:
!       val = M1;
        break;
!     case SIM_SH_X0_REGNUM:
!       val = X0;
        break;
!     case SIM_SH_X1_REGNUM:
!       val = X1;
!       break;
!     case SIM_SH_Y0_REGNUM:
!       val = Y0;
!       break;
!     case SIM_SH_Y1_REGNUM:
!       val = Y1;
!       break;
!     case SIM_SH_MOD_REGNUM:
!       val = MOD;
        break;
!     case SIM_SH_RS_REGNUM:
!       val = RS;
        break;
!     case SIM_SH_RE_REGNUM:
!       val = RE;
        break;
!     case SIM_SH_SSR_REGNUM:
        val = SSR;
        break;
!     case SIM_SH_SPC_REGNUM:
        val = SPC;
        break;
      /* The rn_bank idiosyncracies are not due to hardware differences, but to
         a weird aliasing naming scheme for sh3 / sh3e / sh4.  */
!     case SIM_SH_R0_BANK0_REGNUM: case SIM_SH_R1_BANK0_REGNUM:
!     case SIM_SH_R2_BANK0_REGNUM: case SIM_SH_R3_BANK0_REGNUM:
!     case SIM_SH_R4_BANK0_REGNUM: case SIM_SH_R5_BANK0_REGNUM:
!     case SIM_SH_R6_BANK0_REGNUM: case SIM_SH_R7_BANK0_REGNUM:
!       val = (SR_MD && SR_RB
! 	     ? Rn_BANK (rn - SIM_SH_R0_BANK0_REGNUM)
! 	     : saved_state.asregs.regs[rn - SIM_SH_R0_BANK0_REGNUM]);
!       break;
!     case SIM_SH_R0_BANK1_REGNUM: case SIM_SH_R1_BANK1_REGNUM:
!     case SIM_SH_R2_BANK1_REGNUM: case SIM_SH_R3_BANK1_REGNUM:
!     case SIM_SH_R4_BANK1_REGNUM: case SIM_SH_R5_BANK1_REGNUM:
!     case SIM_SH_R6_BANK1_REGNUM: case SIM_SH_R7_BANK1_REGNUM:
!       val = (! SR_MD || ! SR_RB
! 	     ? Rn_BANK (rn - SIM_SH_R0_BANK1_REGNUM)
! 	     : saved_state.asregs.regs[rn - SIM_SH_R0_BANK1_REGNUM]);
!       break;
!     case SIM_SH_R0_BANK_REGNUM: case SIM_SH_R1_BANK_REGNUM:
!     case SIM_SH_R2_BANK_REGNUM: case SIM_SH_R3_BANK_REGNUM:
!     case SIM_SH_R4_BANK_REGNUM: case SIM_SH_R5_BANK_REGNUM:
!     case SIM_SH_R6_BANK_REGNUM: case SIM_SH_R7_BANK_REGNUM:
!       val = Rn_BANK (rn - SIM_SH_R0_BANK_REGNUM);
        break;
      default:
        return 0;
Index: gdb/sh-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/sh-tdep.c,v
retrieving revision 1.65
diff -p -r1.65 sh-tdep.c
*** gdb/sh-tdep.c	26 Jun 2002 15:28:46 -0000	1.65
--- gdb/sh-tdep.c	3 Jul 2002 20:32:49 -0000
***************
*** 54,59 ****
--- 54,61 ----
  void (*sh_show_regs) (void);
  CORE_ADDR (*skip_prologue_hard_way) (CORE_ADDR);
  void (*do_pseudo_register) (int);
+ static int sh_dsp_register_sim_regno (int);
+ 
  
  #define SH_DEFAULT_NUM_REGS 59
  
*************** sh_gdbarch_init (struct gdbarch_info inf
*** 4280,4285 ****
--- 4282,4288 ----
    set_gdbarch_deprecated_extract_struct_value_address (gdbarch, sh_extract_struct_value_address);
    set_gdbarch_pop_frame (gdbarch, sh_pop_frame);
    set_gdbarch_print_insn (gdbarch, gdb_print_insn_sh);
+   set_gdbarch_register_sim_regno (gdbarch, legacy_register_sim_regno);
    skip_prologue_hard_way = sh_skip_prologue_hard_way;
    do_pseudo_register = sh_do_pseudo_register;
  
*************** sh_gdbarch_init (struct gdbarch_info inf
*** 4314,4319 ****
--- 4317,4323 ----
        set_gdbarch_register_raw_size (gdbarch, sh_default_register_raw_size);
        set_gdbarch_register_virtual_size (gdbarch, sh_default_register_raw_size);
        set_gdbarch_register_byte (gdbarch, sh_default_register_byte);
+       set_gdbarch_register_sim_regno (gdbarch, sh_dsp_register_sim_regno);
        tdep->DSR_REGNUM = 24;
        tdep->A0G_REGNUM = 25;
        tdep->A0_REGNUM = 26;
*************** _initialize_sh_tdep (void)
*** 4600,4603 ****
--- 4604,4648 ----
    gdbarch_register (bfd_arch_sh, sh_gdbarch_init, sh_dump_tdep);
  
    add_com ("regs", class_vars, sh_show_regs_command, "Print all registers");
+ }
+ 
+ enum
+ {
+    DSP_DSR_REGNUM = 24,
+    DSP_A0G_REGNUM,
+    DSP_A0_REGNUM,
+    DSP_A1G_REGNUM,
+    DSP_A1_REGNUM,
+    DSP_M0_REGNUM,
+    DSP_M1_REGNUM,
+    DSP_X0_REGNUM,
+    DSP_X1_REGNUM,
+    DSP_Y0_REGNUM,
+    DSP_Y1_REGNUM,
+  
+    DSP_MOD_REGNUM = 40,
+  
+    DSP_RS_REGNUM = 43,
+    DSP_RE_REGNUM,
+  
+    DSP_R0_BANK_REGNUM = 51,
+    DSP_R7_BANK_REGNUM = DSP_R0_BANK_REGNUM + 7
+ };
+ 
+ static int
+ sh_dsp_register_sim_regno (int nr)
+ {
+   if (legacy_register_sim_regno (nr) < 0)
+     return legacy_register_sim_regno (nr);
+   if (nr >= DSP_DSR_REGNUM && nr < DSP_Y1_REGNUM)
+     return nr - DSP_DSR_REGNUM + SIM_SH_DSR_REGNUM;
+   if (nr == DSP_MOD_REGNUM)
+     return SIM_SH_MOD_REGNUM;
+   if (nr == DSP_RS_REGNUM)
+     return SIM_SH_RS_REGNUM;
+   if (nr == DSP_RE_REGNUM)
+     return SIM_SH_RE_REGNUM;
+   if (nr >= DSP_R0_BANK_REGNUM && nr <= DSP_R7_BANK_REGNUM)
+     return nr - DSP_R0_BANK_REGNUM + SIM_SH_R0_BANK_REGNUM;
+   return nr;
  }

Re: Unreviewed patches

[Elena Zannoni]

Joern Rennecke writes:
 > Elena Zannoni wrote:
 > > Hmm, I don't think we ever worried about that. We have broken
 > > compatibility even between gcc and gdb, sometimes. And here the chance
 > > is smaller, given that sim and gdb come from the same repo and are
 > > built and linked together.
 > 
 > All right, I have appended a patch that uses separate numbers for the sh-dsp
 > registers.  I've left the other numbers unchanged.
 > 
 > regression tested on i686-px-linux-gnu X sh-elf; the tally before and
 > after the patch is the same:
 > # of expected passes            6681
 > # of unexpected failures        322
 > # of expected failures          60
 > # of unresolved testcases       29
 > # of unsupported tests          7
 >  
 > > Oh, yes, I see them. I guess they don't reply with 'accepted' in the
 > > cgen world.
 > 
 > Actually, he did reply, but it was a personal email.
 > 
 > > But I cannot approve changes to the sh64 sim.
 > 
 > So is the sh64 gdb currently without a maintainer?
 > 	

I am maintaining the sh64 gdb part only. I am not sure about the
status of the sim part.

The sim-sh.h change is approved.
The interp.c changes look ok too.
For the gdb sh-tdep.c part, I have a few comments.
There is no need for a prototype. I removed all of those a few months back.
You should just move the new function to before the gdbarch_init one.
Instead of adding another enum, you should use

  struct gdbarch_tdep *tdep = gdbarch_tdep (current_gdbarch); 
 and then
  tdep-><...>_REGNUM

If you need new register numbers for the dsp case, just add
them to the tdep structure.

Thanks for cleaning this up.
Elena


 > -- 
 > --------------------------
 > SuperH
 > 2430 Aztec West / Almondsbury / BRISTOL / BS32 4AQ
 > T:+44 1454 462330Wed Jul  3 21:33:14 2002  J"orn Rennecke <joern.rennecke@superh.com>
 > 
 > include/gdb:
 > 	* sim-sh.h: Add enum constants for sh[1-4], sh3e, sh3?-dsp,
 > 	renumbering the sh-dsp registers to use distinct numbers.
 > sim/sh:
 > 	* Makefile (interp.o): Depend on $(srcroot)/include/gdb/sim-sh.h.
 > 	* interp.c: Include "gdb/sim-sh.h".
 > 	(sim_store_register, sim_fetch_register): Use constants defined there.
 > gdb:
 > 	* sh-tydep.c (sh_dsp_register_sim_regno): New function.
 > 	(sh_gdbarch_init): Use it for sh-dsp.
 > 
 > Index: include/gdb/sim-sh.h
 > ===================================================================
 > RCS file: /cvs/src/src/include/gdb/sim-sh.h,v
 > retrieving revision 1.1
 > diff -p -r1.1 sim-sh.h
 > *** include/gdb/sim-sh.h	10 May 2002 22:53:56 -0000	1.1
 > --- include/gdb/sim-sh.h	3 Jul 2002 19:46:29 -0000
 > ***************
 > *** 1,5 ****
 >   /* This file defines the interface between the sh simulator and gdb.
 > !    Copyright (C) 2002 Free Software Foundation, Inc.
 >   
 >   This file is part of GDB.
 >   
 > --- 1,5 ----
 >   /* This file defines the interface between the sh simulator and gdb.
 > !    Copyright (C) 2000, 2002 Free Software Foundation, Inc.
 >   
 >   This file is part of GDB.
 >   
 > *************** extern "C" { // }
 > *** 27,51 ****
 >   /* The simulator makes use of the following register information. */
 >   
 >   enum
 > !   {
 > !     SIM_SH64_R0_REGNUM = 0,
 > !     SIM_SH64_SP_REGNUM = 15,
 > !     SIM_SH64_PC_REGNUM = 64,
 > !     SIM_SH64_SR_REGNUM = 65,
 > !     SIM_SH64_SSR_REGNUM = 66,
 > !     SIM_SH64_SPC_REGNUM = 67,
 > !     SIM_SH64_TR0_REGNUM = 68,
 > !     SIM_SH64_FPCSR_REGNUM = 76,
 > !     SIM_SH64_FR0_REGNUM = 77
 > !   };
 >   
 >   enum
 > !   {
 > !     SIM_SH64_NR_REGS = 141,  /* total number of architectural registers */
 > !     SIM_SH64_NR_R_REGS = 64, /* number of general registers */
 > !     SIM_SH64_NR_TR_REGS = 8, /* number of target registers */
 > !     SIM_SH64_NR_FP_REGS = 64 /* number of floating point registers */
 > !   };
 >   
 >   #ifdef __cplusplus
 >   }
 > --- 27,158 ----
 >   /* The simulator makes use of the following register information. */
 >   
 >   enum
 > ! {
 > !   SIM_SH_R0_REGNUM = 0,
 > !   SIM_SH_R1_REGNUM,
 > !   SIM_SH_R2_REGNUM,
 > !   SIM_SH_R3_REGNUM,
 > !   SIM_SH_R4_REGNUM,
 > !   SIM_SH_R5_REGNUM,
 > !   SIM_SH_R6_REGNUM,
 > !   SIM_SH_R7_REGNUM,
 > !   SIM_SH_R8_REGNUM,
 > !   SIM_SH_R9_REGNUM,
 > !   SIM_SH_R10_REGNUM,
 > !   SIM_SH_R11_REGNUM,
 > !   SIM_SH_R12_REGNUM,
 > !   SIM_SH_R13_REGNUM,
 > !   SIM_SH_R14_REGNUM,
 > !   SIM_SH_R15_REGNUM,
 > !   SIM_SH_PC_REGNUM,
 > !   SIM_SH_PR_REGNUM,
 > !   SIM_SH_GBR_REGNUM,
 > !   SIM_SH_VBR_REGNUM,
 > !   SIM_SH_MACH_REGNUM,
 > !   SIM_SH_MACL_REGNUM,
 > !   SIM_SH_SR_REGNUM,
 > !   SIM_SH_FPUL_REGNUM,
 > !   SIM_SH_FPSCR_REGNUM,
 > !   SIM_SH_FR0_REGNUM, /* FRn registers: sh3e / sh4 */
 > !   SIM_SH_FR1_REGNUM,
 > !   SIM_SH_FR2_REGNUM,
 > !   SIM_SH_FR3_REGNUM,
 > !   SIM_SH_FR4_REGNUM,
 > !   SIM_SH_FR5_REGNUM,
 > !   SIM_SH_FR6_REGNUM,
 > !   SIM_SH_FR7_REGNUM,
 > !   SIM_SH_FR8_REGNUM,
 > !   SIM_SH_FR9_REGNUM,
 > !   SIM_SH_FR10_REGNUM,
 > !   SIM_SH_FR11_REGNUM,
 > !   SIM_SH_FR12_REGNUM,
 > !   SIM_SH_FR13_REGNUM,
 > !   SIM_SH_FR14_REGNUM,
 > !   SIM_SH_FR15_REGNUM,
 > !   SIM_SH_SSR_REGNUM, /* sh3{,e,-dsp}, sh4 */
 > !   SIM_SH_SPC_REGNUM, /* sh3{,e,-dsp}, sh4 */
 > !   SIM_SH_R0_BANK0_REGNUM, /* SIM_SH_Rn_BANKm_REGNUM: sh3[e] / sh4 */
 > !   SIM_SH_R1_BANK0_REGNUM,
 > !   SIM_SH_R2_BANK0_REGNUM,
 > !   SIM_SH_R3_BANK0_REGNUM,
 > !   SIM_SH_R4_BANK0_REGNUM,
 > !   SIM_SH_R5_BANK0_REGNUM,
 > !   SIM_SH_R6_BANK0_REGNUM,
 > !   SIM_SH_R7_BANK0_REGNUM,
 > !   SIM_SH_R0_BANK1_REGNUM,
 > !   SIM_SH_R1_BANK1_REGNUM,
 > !   SIM_SH_R2_BANK1_REGNUM,
 > !   SIM_SH_R3_BANK1_REGNUM,
 > !   SIM_SH_R4_BANK1_REGNUM,
 > !   SIM_SH_R5_BANK1_REGNUM,
 > !   SIM_SH_R6_BANK1_REGNUM,
 > !   SIM_SH_R7_BANK1_REGNUM,
 > !   SIM_SH_XF0_REGNUM,
 > !   SIM_SH_XF1_REGNUM,
 > !   SIM_SH_XF2_REGNUM,
 > !   SIM_SH_XF3_REGNUM,
 > !   SIM_SH_XF4_REGNUM,
 > !   SIM_SH_XF5_REGNUM,
 > !   SIM_SH_XF6_REGNUM,
 > !   SIM_SH_XF7_REGNUM,
 > !   SIM_SH_XF8_REGNUM,
 > !   SIM_SH_XF9_REGNUM,
 > !   SIM_SH_XF10_REGNUM,
 > !   SIM_SH_XF11_REGNUM,
 > !   SIM_SH_XF12_REGNUM,
 > !   SIM_SH_XF13_REGNUM,
 > !   SIM_SH_XF14_REGNUM,
 > !   SIM_SH_XF15_REGNUM,
 > !   SIM_SH_SGR_REGNUM,
 > !   SIM_SH_DBR_REGNUM,
 > !   SIM_SH4_NUM_REGS, /* 77 */
 > ! 
 > !   /* sh[3]-dsp */
 > !   SIM_SH_DSR_REGNUM,
 > !   SIM_SH_A0G_REGNUM,
 > !   SIM_SH_A0_REGNUM,
 > !   SIM_SH_A1G_REGNUM,
 > !   SIM_SH_A1_REGNUM,
 > !   SIM_SH_M0_REGNUM,
 > !   SIM_SH_M1_REGNUM,
 > !   SIM_SH_X0_REGNUM,
 > !   SIM_SH_X1_REGNUM,
 > !   SIM_SH_Y0_REGNUM,
 > !   SIM_SH_Y1_REGNUM,
 > !   SIM_SH_MOD_REGNUM,
 > !   SIM_SH_RS_REGNUM,
 > !   SIM_SH_RE_REGNUM,
 > !   SIM_SH_R0_BANK_REGNUM,
 > !   SIM_SH_R1_BANK_REGNUM,
 > !   SIM_SH_R2_BANK_REGNUM,
 > !   SIM_SH_R3_BANK_REGNUM,
 > !   SIM_SH_R4_BANK_REGNUM,
 > !   SIM_SH_R5_BANK_REGNUM,
 > !   SIM_SH_R6_BANK_REGNUM,
 > !   SIM_SH_R7_BANK_REGNUM
 > !   /* 100..127: room for expansion.  */
 > ! };
 >   
 >   enum
 > ! {
 > !   SIM_SH64_R0_REGNUM = 0,
 > !   SIM_SH64_SP_REGNUM = 15,
 > !   SIM_SH64_PC_REGNUM = 64,
 > !   SIM_SH64_SR_REGNUM = 65,
 > !   SIM_SH64_SSR_REGNUM = 66,
 > !   SIM_SH64_SPC_REGNUM = 67,
 > !   SIM_SH64_TR0_REGNUM = 68,
 > !   SIM_SH64_FPCSR_REGNUM = 76,
 > !   SIM_SH64_FR0_REGNUM = 77
 > ! };
 > ! 
 > ! enum
 > ! {
 > !   SIM_SH64_NR_REGS = 141,  /* total number of architectural registers */
 > !   SIM_SH64_NR_R_REGS = 64, /* number of general registers */
 > !   SIM_SH64_NR_TR_REGS = 8, /* number of target registers */
 > !   SIM_SH64_NR_FP_REGS = 64 /* number of floating point registers */
 > ! };
 >   
 >   #ifdef __cplusplus
 >   }
 > Index: sim/sh/interp.c
 > ===================================================================
 > RCS file: /cvs/src/src/sim/sh/interp.c,v
 > retrieving revision 1.6
 > diff -p -r1.6 interp.c
 > *** sim/sh/interp.c	18 Jun 2002 15:54:44 -0000	1.6
 > --- sim/sh/interp.c	3 Jul 2002 19:46:29 -0000
 > ***************
 > *** 29,34 ****
 > --- 29,35 ----
 >   #include "bfd.h"
 >   #include "gdb/callback.h"
 >   #include "gdb/remote-sim.h"
 > + #include "gdb/sim-sh.h"
 >   
 >   /* This file is local - if newlib changes, then so should this.  */
 >   #include "syscall.h"
 > *************** sim_store_register (sd, rn, memory, leng
 > *** 1790,1887 ****
 >     val = swap (* (int *)memory);
 >     switch (rn)
 >       {
 > !     case  0: case  1: case  2: case  3: case  4: case  5: case  6: case  7:
 > !     case  8: case  9: case 10: case 11: case 12: case 13: case 14: case 15:
 >         saved_state.asregs.regs[rn] = val;
 >         break;
 > !     case 16:
 >         saved_state.asregs.pc = val;
 >         break;
 > !     case 17:
 >         PR = val;
 >         break;
 > !     case 18:
 >         GBR = val;
 >         break;
 > !     case 19:
 >         VBR = val;
 >         break;
 > !     case 20:
 >         MACH = val;
 >         break;
 > !     case 21:
 >         MACL = val;
 >         break;
 > !     case 22:
 >         SET_SR (val);
 >         break;
 > !     case 23:
 >         FPUL = val;
 >         break;
 > !     case 24:
 >         SET_FPSCR (val);
 >         break;
 > !     case 25:
 > !       if (target_dsp)
 > ! 	A0G = val;
 > !     else case 26:
 > !       if (target_dsp)
 > ! 	A0 = val;
 > !     else case 27:
 > !       if (target_dsp)
 > ! 	A1G = val;
 > !     else case 28:
 > !       if (target_dsp)
 > ! 	A1 = val;
 > !     else case 29:
 > !       if (target_dsp)
 > ! 	M0 = val;
 > !     else case 30:
 > !       if (target_dsp)
 > ! 	M1 = val;
 > !     else case 31:
 > !       if (target_dsp)
 > ! 	X0 = val;
 > !     else case 32:
 > !       if (target_dsp)
 > ! 	X1 = val;
 > !     else case 33:
 > !       if (target_dsp)
 > ! 	Y0 = val;
 > !     else case 34:
 > !       if (target_dsp)
 > ! 	Y1 = val;
 > !     else case 40:
 > !       if (target_dsp)
 > ! 	SET_MOD (val);
 > !     else case 35: case 36: case 37: case 38: case 39:
 > ! 	SET_FI (rn - 25, val);
 >         break;
 > !     case 41:
 >         SSR = val;
 >         break;
 > !     case 42:
 >         SPC = val;
 >         break;
 >       /* The rn_bank idiosyncracies are not due to hardware differences, but to
 >          a weird aliasing naming scheme for sh3 / sh3e / sh4.  */
 > !     case 43:
 > !       if (target_dsp)
 > ! 	RS = val;
 > !     else case 44:
 > !       if (target_dsp)
 > ! 	RE = val;
 > !     else case 45: case 46: case 47: case 48: case 49: case 50:
 >         if (SR_MD && SR_RB)
 > ! 	Rn_BANK (rn - 43) = val;
 >         else
 > ! 	saved_state.asregs.regs[rn - 43] = val;
 >         break;
 > !     case 51: case 52: case 53: case 54: case 55: case 56: case 57: case 58:
 > !       if (target_dsp || ! SR_MD || ! SR_RB)
 > ! 	SET_Rn_BANK (rn - 51, val);
 >         else
 > ! 	saved_state.asregs.regs[rn - 51] = val;
 >         break;
 >       default:
 >         return 0;
 > --- 1791,1912 ----
 >     val = swap (* (int *)memory);
 >     switch (rn)
 >       {
 > !     case SIM_SH_R0_REGNUM: case SIM_SH_R1_REGNUM: case SIM_SH_R2_REGNUM:
 > !     case SIM_SH_R3_REGNUM: case SIM_SH_R4_REGNUM: case SIM_SH_R5_REGNUM:
 > !     case SIM_SH_R6_REGNUM: case SIM_SH_R7_REGNUM: case SIM_SH_R8_REGNUM:
 > !     case SIM_SH_R9_REGNUM: case SIM_SH_R10_REGNUM: case SIM_SH_R11_REGNUM:
 > !     case SIM_SH_R12_REGNUM: case SIM_SH_R13_REGNUM: case SIM_SH_R14_REGNUM:
 > !     case SIM_SH_R15_REGNUM:
 >         saved_state.asregs.regs[rn] = val;
 >         break;
 > !     case SIM_SH_PC_REGNUM:
 >         saved_state.asregs.pc = val;
 >         break;
 > !     case SIM_SH_PR_REGNUM:
 >         PR = val;
 >         break;
 > !     case SIM_SH_GBR_REGNUM:
 >         GBR = val;
 >         break;
 > !     case SIM_SH_VBR_REGNUM:
 >         VBR = val;
 >         break;
 > !     case SIM_SH_MACH_REGNUM:
 >         MACH = val;
 >         break;
 > !     case SIM_SH_MACL_REGNUM:
 >         MACL = val;
 >         break;
 > !     case SIM_SH_SR_REGNUM:
 >         SET_SR (val);
 >         break;
 > !     case SIM_SH_FPUL_REGNUM:
 >         FPUL = val;
 >         break;
 > !     case SIM_SH_FPSCR_REGNUM:
 >         SET_FPSCR (val);
 >         break;
 > !     case SIM_SH_FR0_REGNUM: case SIM_SH_FR1_REGNUM: case SIM_SH_FR2_REGNUM:
 > !     case SIM_SH_FR3_REGNUM: case SIM_SH_FR4_REGNUM: case SIM_SH_FR5_REGNUM:
 > !     case SIM_SH_FR6_REGNUM: case SIM_SH_FR7_REGNUM: case SIM_SH_FR8_REGNUM:
 > !     case SIM_SH_FR9_REGNUM: case SIM_SH_FR10_REGNUM: case SIM_SH_FR11_REGNUM:
 > !     case SIM_SH_FR12_REGNUM: case SIM_SH_FR13_REGNUM: case SIM_SH_FR14_REGNUM:
 > !     case SIM_SH_FR15_REGNUM:
 > !       SET_FI (rn - SIM_SH_FR0_REGNUM, val);
 >         break;
 > !     case SIM_SH_DSR_REGNUM:
 > !       DSR = val;
 > !       break;
 > !     case SIM_SH_A0G_REGNUM:
 > !       A0G = val;
 > !       break;
 > !     case SIM_SH_A0_REGNUM:
 > !       A0 = val;
 > !       break;
 > !     case SIM_SH_A1G_REGNUM:
 > !       A1G = val;
 > !       break;
 > !     case SIM_SH_A1_REGNUM:
 > !       A1 = val;
 > !       break;
 > !     case SIM_SH_M0_REGNUM:
 > !       M0 = val;
 > !       break;
 > !     case SIM_SH_M1_REGNUM:
 > !       M1 = val;
 > !       break;
 > !     case SIM_SH_X0_REGNUM:
 > !       X0 = val;
 > !       break;
 > !     case SIM_SH_X1_REGNUM:
 > !       X1 = val;
 > !       break;
 > !     case SIM_SH_Y0_REGNUM:
 > !       Y0 = val;
 > !       break;
 > !     case SIM_SH_Y1_REGNUM:
 > !       Y1 = val;
 > !       break;
 > !     case SIM_SH_MOD_REGNUM:
 > !       SET_MOD (val);
 > !       break;
 > !     case SIM_SH_RS_REGNUM:
 > !       RS = val;
 > !       break;
 > !     case SIM_SH_RE_REGNUM:
 > !       RE = val;
 > !       break;
 > !     case SIM_SH_SSR_REGNUM:
 >         SSR = val;
 >         break;
 > !     case SIM_SH_SPC_REGNUM:
 >         SPC = val;
 >         break;
 >       /* The rn_bank idiosyncracies are not due to hardware differences, but to
 >          a weird aliasing naming scheme for sh3 / sh3e / sh4.  */
 > !     case SIM_SH_R0_BANK0_REGNUM: case SIM_SH_R1_BANK0_REGNUM:
 > !     case SIM_SH_R2_BANK0_REGNUM: case SIM_SH_R3_BANK0_REGNUM:
 > !     case SIM_SH_R4_BANK0_REGNUM: case SIM_SH_R5_BANK0_REGNUM:
 > !     case SIM_SH_R6_BANK0_REGNUM: case SIM_SH_R7_BANK0_REGNUM:
 >         if (SR_MD && SR_RB)
 > ! 	Rn_BANK (rn - SIM_SH_R0_BANK0_REGNUM) = val;
 >         else
 > ! 	saved_state.asregs.regs[rn - SIM_SH_R0_BANK0_REGNUM] = val;
 >         break;
 > !     case SIM_SH_R0_BANK1_REGNUM: case SIM_SH_R1_BANK1_REGNUM:
 > !     case SIM_SH_R2_BANK1_REGNUM: case SIM_SH_R3_BANK1_REGNUM:
 > !     case SIM_SH_R4_BANK1_REGNUM: case SIM_SH_R5_BANK1_REGNUM:
 > !     case SIM_SH_R6_BANK1_REGNUM: case SIM_SH_R7_BANK1_REGNUM:
 > !       if (SR_MD && SR_RB)
 > ! 	saved_state.asregs.regs[rn - SIM_SH_R0_BANK1_REGNUM] = val;
 >         else
 > ! 	Rn_BANK (rn - SIM_SH_R0_BANK1_REGNUM) = val;
 > !       break;
 > !     case SIM_SH_R0_BANK_REGNUM: case SIM_SH_R1_BANK_REGNUM:
 > !     case SIM_SH_R2_BANK_REGNUM: case SIM_SH_R3_BANK_REGNUM:
 > !     case SIM_SH_R4_BANK_REGNUM: case SIM_SH_R5_BANK_REGNUM:
 > !     case SIM_SH_R6_BANK_REGNUM: case SIM_SH_R7_BANK_REGNUM:
 > !       SET_Rn_BANK (rn - SIM_SH_R0_BANK_REGNUM, val);
 >         break;
 >       default:
 >         return 0;
 > *************** sim_fetch_register (sd, rn, memory, leng
 > *** 1901,1996 ****
 >     init_pointers ();
 >     switch (rn)
 >       {
 > !     case  0: case  1: case  2: case  3: case  4: case  5: case  6: case  7:
 > !     case  8: case  9: case 10: case 11: case 12: case 13: case 14: case 15:
 >         val = saved_state.asregs.regs[rn];
 >         break;
 > !     case 16:
 >         val = saved_state.asregs.pc;
 >         break;
 > !     case 17:
 >         val = PR;
 >         break;
 > !     case 18:
 >         val = GBR;
 >         break;
 > !     case 19:
 >         val = VBR;
 >         break;
 > !     case 20:
 >         val = MACH;
 >         break;
 > !     case 21:
 >         val = MACL;
 >         break;
 > !     case 22:
 >         val = GET_SR ();
 >         break;
 > !     case 23:
 >         val = FPUL;
 >         break;
 > !     case 24:
 >         val = GET_FPSCR ();
 >         break;
 > !     case 25:
 > !       val = target_dsp ? SEXT (A0G) : FI (0);
 >         break;
 > !     case 26:
 > !       val = target_dsp ? A0 : FI (1);
 >         break;
 > !     case 27:
 > !       val = target_dsp ? SEXT (A1G) : FI (2);
 >         break;
 > !     case 28:
 > !       val = target_dsp ? A1 : FI (3);
 >         break;
 > !     case 29:
 > !       val = target_dsp ? M0 : FI (4);
 >         break;
 > !     case 30:
 > !       val = target_dsp ? M1 : FI (5);
 >         break;
 > !     case 31:
 > !       val = target_dsp ? X0 : FI (6);
 >         break;
 > !     case 32:
 > !       val = target_dsp ? X1 : FI (7);
 >         break;
 > !     case 33:
 > !       val = target_dsp ? Y0 : FI (8);
 >         break;
 > !     case 34:
 > !       val = target_dsp ? Y1 : FI (9);
 >         break;
 > !     case 35: case 36: case 37: case 38: case 39:
 > !       val = FI (rn - 25);
 >         break;
 > !     case 40:
 > !       val = target_dsp ? MOD : FI (15);
 >         break;
 > !     case 41:
 >         val = SSR;
 >         break;
 > !     case 42:
 >         val = SPC;
 >         break;
 >       /* The rn_bank idiosyncracies are not due to hardware differences, but to
 >          a weird aliasing naming scheme for sh3 / sh3e / sh4.  */
 > !     case 43:
 > !       if (target_dsp)
 > ! 	val = RS;
 > !     else case 44:
 > !       if (target_dsp)
 > ! 	val = RE;
 > !     else case 45: case 46: case 47: case 48: case 49: case 50:
 > ! 	val = (SR_MD && SR_RB
 > ! 	       ? Rn_BANK (rn - 43)
 > ! 	       : saved_state.asregs.regs[rn - 43]);
 > !       break;
 > !     case 51: case 52: case 53: case 54: case 55: case 56: case 57: case 58:
 > !       val = (target_dsp || ! SR_MD || ! SR_RB
 > ! 	     ? Rn_BANK (rn - 51)
 > ! 	     : saved_state.asregs.regs[rn - 51]);
 >         break;
 >       default:
 >         return 0;
 > --- 1926,2045 ----
 >     init_pointers ();
 >     switch (rn)
 >       {
 > !     case SIM_SH_R0_REGNUM: case SIM_SH_R1_REGNUM: case SIM_SH_R2_REGNUM:
 > !     case SIM_SH_R3_REGNUM: case SIM_SH_R4_REGNUM: case SIM_SH_R5_REGNUM:
 > !     case SIM_SH_R6_REGNUM: case SIM_SH_R7_REGNUM: case SIM_SH_R8_REGNUM:
 > !     case SIM_SH_R9_REGNUM: case SIM_SH_R10_REGNUM: case SIM_SH_R11_REGNUM:
 > !     case SIM_SH_R12_REGNUM: case SIM_SH_R13_REGNUM: case SIM_SH_R14_REGNUM:
 > !     case SIM_SH_R15_REGNUM:
 >         val = saved_state.asregs.regs[rn];
 >         break;
 > !     case SIM_SH_PC_REGNUM:
 >         val = saved_state.asregs.pc;
 >         break;
 > !     case SIM_SH_PR_REGNUM:
 >         val = PR;
 >         break;
 > !     case SIM_SH_GBR_REGNUM:
 >         val = GBR;
 >         break;
 > !     case SIM_SH_VBR_REGNUM:
 >         val = VBR;
 >         break;
 > !     case SIM_SH_MACH_REGNUM:
 >         val = MACH;
 >         break;
 > !     case SIM_SH_MACL_REGNUM:
 >         val = MACL;
 >         break;
 > !     case SIM_SH_SR_REGNUM:
 >         val = GET_SR ();
 >         break;
 > !     case SIM_SH_FPUL_REGNUM:
 >         val = FPUL;
 >         break;
 > !     case SIM_SH_FPSCR_REGNUM:
 >         val = GET_FPSCR ();
 >         break;
 > !     case SIM_SH_FR0_REGNUM: case SIM_SH_FR1_REGNUM: case SIM_SH_FR2_REGNUM:
 > !     case SIM_SH_FR3_REGNUM: case SIM_SH_FR4_REGNUM: case SIM_SH_FR5_REGNUM:
 > !     case SIM_SH_FR6_REGNUM: case SIM_SH_FR7_REGNUM: case SIM_SH_FR8_REGNUM:
 > !     case SIM_SH_FR9_REGNUM: case SIM_SH_FR10_REGNUM: case SIM_SH_FR11_REGNUM:
 > !     case SIM_SH_FR12_REGNUM: case SIM_SH_FR13_REGNUM: case SIM_SH_FR14_REGNUM:
 > !     case SIM_SH_FR15_REGNUM:
 > !       val = FI (rn - SIM_SH_FR0_REGNUM);
 >         break;
 > !     case SIM_SH_DSR_REGNUM:
 > !       val = DSR;
 >         break;
 > !     case SIM_SH_A0G_REGNUM:
 > !       val = SEXT (A0G);
 >         break;
 > !     case SIM_SH_A0_REGNUM:
 > !       val = A0;
 >         break;
 > !     case SIM_SH_A1G_REGNUM:
 > !       val = SEXT (A1G);
 >         break;
 > !     case SIM_SH_A1_REGNUM:
 > !       val = A1;
 >         break;
 > !     case SIM_SH_M0_REGNUM:
 > !       val = M0;
 >         break;
 > !     case SIM_SH_M1_REGNUM:
 > !       val = M1;
 >         break;
 > !     case SIM_SH_X0_REGNUM:
 > !       val = X0;
 >         break;
 > !     case SIM_SH_X1_REGNUM:
 > !       val = X1;
 > !       break;
 > !     case SIM_SH_Y0_REGNUM:
 > !       val = Y0;
 > !       break;
 > !     case SIM_SH_Y1_REGNUM:
 > !       val = Y1;
 > !       break;
 > !     case SIM_SH_MOD_REGNUM:
 > !       val = MOD;
 >         break;
 > !     case SIM_SH_RS_REGNUM:
 > !       val = RS;
 >         break;
 > !     case SIM_SH_RE_REGNUM:
 > !       val = RE;
 >         break;
 > !     case SIM_SH_SSR_REGNUM:
 >         val = SSR;
 >         break;
 > !     case SIM_SH_SPC_REGNUM:
 >         val = SPC;
 >         break;
 >       /* The rn_bank idiosyncracies are not due to hardware differences, but to
 >          a weird aliasing naming scheme for sh3 / sh3e / sh4.  */
 > !     case SIM_SH_R0_BANK0_REGNUM: case SIM_SH_R1_BANK0_REGNUM:
 > !     case SIM_SH_R2_BANK0_REGNUM: case SIM_SH_R3_BANK0_REGNUM:
 > !     case SIM_SH_R4_BANK0_REGNUM: case SIM_SH_R5_BANK0_REGNUM:
 > !     case SIM_SH_R6_BANK0_REGNUM: case SIM_SH_R7_BANK0_REGNUM:
 > !       val = (SR_MD && SR_RB
 > ! 	     ? Rn_BANK (rn - SIM_SH_R0_BANK0_REGNUM)
 > ! 	     : saved_state.asregs.regs[rn - SIM_SH_R0_BANK0_REGNUM]);
 > !       break;
 > !     case SIM_SH_R0_BANK1_REGNUM: case SIM_SH_R1_BANK1_REGNUM:
 > !     case SIM_SH_R2_BANK1_REGNUM: case SIM_SH_R3_BANK1_REGNUM:
 > !     case SIM_SH_R4_BANK1_REGNUM: case SIM_SH_R5_BANK1_REGNUM:
 > !     case SIM_SH_R6_BANK1_REGNUM: case SIM_SH_R7_BANK1_REGNUM:
 > !       val = (! SR_MD || ! SR_RB
 > ! 	     ? Rn_BANK (rn - SIM_SH_R0_BANK1_REGNUM)
 > ! 	     : saved_state.asregs.regs[rn - SIM_SH_R0_BANK1_REGNUM]);
 > !       break;
 > !     case SIM_SH_R0_BANK_REGNUM: case SIM_SH_R1_BANK_REGNUM:
 > !     case SIM_SH_R2_BANK_REGNUM: case SIM_SH_R3_BANK_REGNUM:
 > !     case SIM_SH_R4_BANK_REGNUM: case SIM_SH_R5_BANK_REGNUM:
 > !     case SIM_SH_R6_BANK_REGNUM: case SIM_SH_R7_BANK_REGNUM:
 > !       val = Rn_BANK (rn - SIM_SH_R0_BANK_REGNUM);
 >         break;
 >       default:
 >         return 0;
 > Index: gdb/sh-tdep.c
 > ===================================================================
 > RCS file: /cvs/src/src/gdb/sh-tdep.c,v
 > retrieving revision 1.65
 > diff -p -r1.65 sh-tdep.c
 > *** gdb/sh-tdep.c	26 Jun 2002 15:28:46 -0000	1.65
 > --- gdb/sh-tdep.c	3 Jul 2002 20:32:49 -0000
 > ***************
 > *** 54,59 ****
 > --- 54,61 ----
 >   void (*sh_show_regs) (void);
 >   CORE_ADDR (*skip_prologue_hard_way) (CORE_ADDR);
 >   void (*do_pseudo_register) (int);
 > + static int sh_dsp_register_sim_regno (int);
 > + 
 >   
 >   #define SH_DEFAULT_NUM_REGS 59
 >   
 > *************** sh_gdbarch_init (struct gdbarch_info inf
 > *** 4280,4285 ****
 > --- 4282,4288 ----
 >     set_gdbarch_deprecated_extract_struct_value_address (gdbarch, sh_extract_struct_value_address);
 >     set_gdbarch_pop_frame (gdbarch, sh_pop_frame);
 >     set_gdbarch_print_insn (gdbarch, gdb_print_insn_sh);
 > +   set_gdbarch_register_sim_regno (gdbarch, legacy_register_sim_regno);
 >     skip_prologue_hard_way = sh_skip_prologue_hard_way;
 >     do_pseudo_register = sh_do_pseudo_register;
 >   
 > *************** sh_gdbarch_init (struct gdbarch_info inf
 > *** 4314,4319 ****
 > --- 4317,4323 ----
 >         set_gdbarch_register_raw_size (gdbarch, sh_default_register_raw_size);
 >         set_gdbarch_register_virtual_size (gdbarch, sh_default_register_raw_size);
 >         set_gdbarch_register_byte (gdbarch, sh_default_register_byte);
 > +       set_gdbarch_register_sim_regno (gdbarch, sh_dsp_register_sim_regno);
 >         tdep->DSR_REGNUM = 24;
 >         tdep->A0G_REGNUM = 25;
 >         tdep->A0_REGNUM = 26;
 > *************** _initialize_sh_tdep (void)
 > *** 4600,4603 ****
 > --- 4604,4648 ----
 >     gdbarch_register (bfd_arch_sh, sh_gdbarch_init, sh_dump_tdep);
 >   
 >     add_com ("regs", class_vars, sh_show_regs_command, "Print all registers");
 > + }
 > + 
 > + enum
 > + {
 > +    DSP_DSR_REGNUM = 24,
 > +    DSP_A0G_REGNUM,
 > +    DSP_A0_REGNUM,
 > +    DSP_A1G_REGNUM,
 > +    DSP_A1_REGNUM,
 > +    DSP_M0_REGNUM,
 > +    DSP_M1_REGNUM,
 > +    DSP_X0_REGNUM,
 > +    DSP_X1_REGNUM,
 > +    DSP_Y0_REGNUM,
 > +    DSP_Y1_REGNUM,
 > +  
 > +    DSP_MOD_REGNUM = 40,
 > +  
 > +    DSP_RS_REGNUM = 43,
 > +    DSP_RE_REGNUM,
 > +  
 > +    DSP_R0_BANK_REGNUM = 51,
 > +    DSP_R7_BANK_REGNUM = DSP_R0_BANK_REGNUM + 7
 > + };
 > + 
 > + static int
 > + sh_dsp_register_sim_regno (int nr)
 > + {
 > +   if (legacy_register_sim_regno (nr) < 0)
 > +     return legacy_register_sim_regno (nr);
 > +   if (nr >= DSP_DSR_REGNUM && nr < DSP_Y1_REGNUM)
 > +     return nr - DSP_DSR_REGNUM + SIM_SH_DSR_REGNUM;
 > +   if (nr == DSP_MOD_REGNUM)
 > +     return SIM_SH_MOD_REGNUM;
 > +   if (nr == DSP_RS_REGNUM)
 > +     return SIM_SH_RS_REGNUM;
 > +   if (nr == DSP_RE_REGNUM)
 > +     return SIM_SH_RE_REGNUM;
 > +   if (nr >= DSP_R0_BANK_REGNUM && nr <= DSP_R7_BANK_REGNUM)
 > +     return nr - DSP_R0_BANK_REGNUM + SIM_SH_R0_BANK_REGNUM;
 > +   return nr;
 >   }

Re: Unreviewed patches

[Joern Rennecke]

Elena Zannoni wrote:
> For the gdb sh-tdep.c part, I have a few comments.
> There is no need for a prototype. I removed all of those a few months back.

Ok, I'll place the new function so that it needs no prototype.
But you have to keep in mind that that makes the call graph more
rigid, unless you like to move code around a lot.

> You should just move the new function to before the gdbarch_init one.
> Instead of adding another enum, you should use
> 
>   struct gdbarch_tdep *tdep = gdbarch_tdep (current_gdbarch);
>  and then
>   tdep-><...>_REGNUM
> 
> If you need new register numbers for the dsp case, just add
> them to the tdep structure.

I think the stashing of constants into the tdep structure is basically
wrong.  You separate the register names arrays from the literals
that describe their positions, and you replicate the literals
up to four times.  The tdep structure and the sh_gdbarch_init
function are so large that you have lost track of the things that
really belong in tdep, like sh_show_regs, skip_prologue_hard_way,
and do_pseudo_register.  If you look at other gdb ports, you'll
see that they put only variable stuff in tdep, and use enums
for constants.  The sh gdb register naming scheme also doesn't
scale well, the names are again duplicated multiple times.
	
I have a scheme that names each register just once, and enumerates
it only once - in an enum, of course.  But I thought we should first
sort out the simulator <-> gdb and gcc -> gdb interfaces before we
start on the gdb internals.
	
-- 
--------------------------
SuperH
2430 Aztec West / Almondsbury / BRISTOL / BS32 4AQ
T:+44 1454 462330

Re: Unreviewed patches

[Andrew Cagney]

> 
> I think the stashing of constants into the tdep structure is basically
> wrong.  You separate the register names arrays from the literals
> that describe their positions, and you replicate the literals
> up to four times.  The tdep structure and the sh_gdbarch_init
> function are so large that you have lost track of the things that
> really belong in tdep, like sh_show_regs, skip_prologue_hard_way,
> and do_pseudo_register.  If you look at other gdb ports, you'll
> see that they put only variable stuff in tdep, and use enums
> for constants.  The sh gdb register naming scheme also doesn't
> scale well, the names are again duplicated multiple times.

Can i suggest comparing the SH with the MIPS or RS6000.

Andrew

Re: Unreviewed patches

[Joern Rennecke]

Andrew Cagney wrote:
> 
> >
> > I think the stashing of constants into the tdep structure is basically
> > wrong.  You separate the register names arrays from the literals
> > that describe their positions, and you replicate the literals
> > up to four times.  The tdep structure and the sh_gdbarch_init
> > function are so large that you have lost track of the things that
> > really belong in tdep, like sh_show_regs, skip_prologue_hard_way,
> > and do_pseudo_register.  If you look at other gdb ports, you'll
> > see that they put only variable stuff in tdep, and use enums
> > for constants.  The sh gdb register naming scheme also doesn't
> > scale well, the names are again duplicated multiple times.
> 
> Can i suggest comparing the SH with the MIPS or RS6000.

MIPS and RS6000 use varying register numbers for hardware registers
with identical name and function.  I suppose that is due to historical
accident?

On the SH, it makes sense to consider the floating point register
start number as variable; however, there are a lot more register
numbers that are constant:
the privileged mode registers of the SH1..SH4 are not replicated
in the SH5; the SH5 has other registers of its own for privileged
mode.  All the sh-dsp specific register are, well, sh-dsp specific,
and hence only the sh-dsp numbering applies.
	
-- 
--------------------------
SuperH
2430 Aztec West / Almondsbury / BRISTOL / BS32 4AQ
T:+44 1454 462330

Re: Unreviewed patches

[Andrew Cagney]

> Andrew Cagney wrote:
> 
>> 
> 
>> >
>> > I think the stashing of constants into the tdep structure is basically
>> > wrong.  You separate the register names arrays from the literals
>> > that describe their positions, and you replicate the literals
>> > up to four times.  The tdep structure and the sh_gdbarch_init
>> > function are so large that you have lost track of the things that
>> > really belong in tdep, like sh_show_regs, skip_prologue_hard_way,
>> > and do_pseudo_register.  If you look at other gdb ports, you'll
>> > see that they put only variable stuff in tdep, and use enums
>> > for constants.  The sh gdb register naming scheme also doesn't
>> > scale well, the names are again duplicated multiple times.
> 
>> 
>> Can i suggest comparing the SH with the MIPS or RS6000.


> MIPS and RS6000 use varying register numbers for hardware registers
> with identical name and function.  I suppose that is due to historical
> accident?

If it happened once it would be an accident.

In the good old days, GDB was built for a specific CPU.  The register 
layout (along with the G packet) was hard wired using constants from the 
tm*.h files.  Each CPU variant had a different register layout (for 
efficiency reasons).

Targets like the SH would then add a limited form of CPU variant support 
using patches like:

Mon May 15 21:27:27 2000  J"orn Rennecke <amylaar@cygnus.co.uk>

         * sh-tdep.c (sh_dsp_reg_names, sh3_dsp_reg_names): New arrays.
         (sh_processor_type_table): Add entries for bfd_mach_sh_dsp and
          bfd_mach_sh3_dsp.

It was implemented by modifying the register name tables (along with a 
few other structures) directly.

In the mean time, people have been replacing that code with code using 
the multi-arch framework.  In doing this conversion, the technique of 
replacing hardwired constants with variables has proven most effective. 
  This is because it lets the developer complete their multi-arch task 
independant of any other longer term changes that GDB may require.

enjoy,
Andrew

Re: Unreviewed patches

[Elena Zannoni]

Joern, I noticed that you checked in the patch anyway, even though
there was disagreement about adding another hardwired enumeration in
sh-tdep.c vs. using the tdep structure, like arm, ppc, etc do. I
apologize if my latest message to this thread wasn't clear enough, and
you thought it was an approval. I think the patch is basically fine,
except for the enum. Could you please fix it?

thanks
Elena


Joern Rennecke writes:
 > Andrew Cagney wrote:
 > > 
 > > >
 > > > I think the stashing of constants into the tdep structure is basically
 > > > wrong.  You separate the register names arrays from the literals
 > > > that describe their positions, and you replicate the literals
 > > > up to four times.  The tdep structure and the sh_gdbarch_init
 > > > function are so large that you have lost track of the things that
 > > > really belong in tdep, like sh_show_regs, skip_prologue_hard_way,
 > > > and do_pseudo_register.  If you look at other gdb ports, you'll
 > > > see that they put only variable stuff in tdep, and use enums
 > > > for constants.  The sh gdb register naming scheme also doesn't
 > > > scale well, the names are again duplicated multiple times.
 > > 
 > > Can i suggest comparing the SH with the MIPS or RS6000.
 > 
 > MIPS and RS6000 use varying register numbers for hardware registers
 > with identical name and function.  I suppose that is due to historical
 > accident?
 > 
 > On the SH, it makes sense to consider the floating point register
 > start number as variable; however, there are a lot more register
 > numbers that are constant:
 > the privileged mode registers of the SH1..SH4 are not replicated
 > in the SH5; the SH5 has other registers of its own for privileged
 > mode.  All the sh-dsp specific register are, well, sh-dsp specific,
 > and hence only the sh-dsp numbering applies.
 > 	
 > -- 
 > --------------------------
 > SuperH
 > 2430 Aztec West / Almondsbury / BRISTOL / BS32 4AQ
 > T:+44 1454 462330

Re: Unreviewed patches

[Elena Zannoni]

Joern Rennecke writes:
 > Elena Zannoni wrote:
 > > For the gdb sh-tdep.c part, I have a few comments.
 > > There is no need for a prototype. I removed all of those a few months back.
 > 
 > Ok, I'll place the new function so that it needs no prototype.
 > But you have to keep in mind that that makes the call graph more
 > rigid, unless you like to move code around a lot.
 > 

Thanks. I did a major clean up recently, and things should be ok for a while.
If I find out that I have to introduce a prototype in the future because
there is no other reasonable choice, I'll revisit the decision then.

 > > You should just move the new function to before the gdbarch_init one.
 > > Instead of adding another enum, you should use
 > > 
 > >   struct gdbarch_tdep *tdep = gdbarch_tdep (current_gdbarch);
 > >  and then
 > >   tdep-><...>_REGNUM
 > > 
 > > If you need new register numbers for the dsp case, just add
 > > them to the tdep structure.
 > 
 > I think the stashing of constants into the tdep structure is basically
 > wrong.  You separate the register names arrays from the literals
 > that describe their positions, and you replicate the literals
 > up to four times.  The tdep structure and the sh_gdbarch_init
 > function are so large that you have lost track of the things that
 > really belong in tdep, like sh_show_regs, skip_prologue_hard_way,
 > and do_pseudo_register.  If you look at other gdb ports, you'll
 > see that they put only variable stuff in tdep, and use enums
 > for constants.  The sh gdb register naming scheme also doesn't
 > scale well, the names are again duplicated multiple times.
 > 	

Tdep contains the things that change between different flavors of the
architecture, when you say 'set architecture blah'. The register
numbers belong to that category. ppc does the same.

 > I have a scheme that names each register just once, and enumerates
 > it only once - in an enum, of course.  But I thought we should first
 > sort out the simulator <-> gdb and gcc -> gdb interfaces before we
 > start on the gdb internals.
 > 	

Definitely, focussing on the interface issues is time better spent
right now.

Elena


 > -- 
 > --------------------------
 > SuperH
 > 2430 Aztec West / Almondsbury / BRISTOL / BS32 4AQ
 > T:+44 1454 462330

Unreviewed patches

[Joern Rennecke]

http://sources.redhat.com/ml/gdb-patches/2002-06/msg00185.html
http://sources.redhat.com/ml/gdb-patches/2002-06/msg00234.html

-- 
--------------------------
SuperH
2430 Aztec West / Almondsbury / BRISTOL / BS32 4AQ
T:+44 1454 462330