RFA: PowerPC: add segment register numbers

RFA: PowerPC: add segment register numbers

[Jim Blandy]

2004-07-12  Jim Blandy  <jimb@redhat.com>

	* ppc-tdep.h (struct gdbarch_tdep): New member: ppc_sr0_regnum.
	* rs6000-tdep.c (rs6000_gdbarch_init): Initialize it.

diff -crp -x '*~' -x '.#*' -x CVS src.01/gdb/ppc-tdep.h src.02/gdb/ppc-tdep.h
*** gdb/ppc-tdep.h	2004-07-12 16:56:48.000000000 -0500
--- gdb/ppc-tdep.h	2004-07-12 17:00:45.000000000 -0500
*************** struct gdbarch_tdep
*** 158,163 ****
--- 158,167 ----
      int ppc_fpscr_regnum;	/* Floating point status and condition
      				   register */
  
+     int ppc_sr0_regnum;         /* segment register 0, or -1 on
+                                    variants that have no segment
+                                    registers.  */
+ 
      int ppc_mq_regnum;		/* Multiply/Divide extension register */
      int ppc_vr0_regnum;		/* First AltiVec register */
      int ppc_vrsave_regnum;	/* Last AltiVec register */
diff -crp -x '*~' -x '.#*' -x CVS src.01/gdb/rs6000-tdep.c src.02/gdb/rs6000-tdep.c
*** gdb/rs6000-tdep.c	2004-07-08 18:43:02.000000000 -0500
--- gdb/rs6000-tdep.c	2004-07-12 17:02:44.000000000 -0500
*************** rs6000_gdbarch_init (struct gdbarch_info
*** 2879,2884 ****
--- 2879,2885 ----
      tdep->ppc_mq_regnum = -1;
    tdep->ppc_fp0_regnum = 32;
    tdep->ppc_fpscr_regnum = power ? 71 : 70;
+   tdep->ppc_sr0_regnum = 71;
    tdep->ppc_vr0_regnum = -1;
    tdep->ppc_vrsave_regnum = -1;
    tdep->ppc_ev0_regnum = -1;
*************** rs6000_gdbarch_init (struct gdbarch_info
*** 2907,2913 ****
    else
      tdep->lr_frame_offset = 8;
  
!   if (v->arch == bfd_arch_powerpc)
      switch (v->mach)
        {
        case bfd_mach_ppc: 
--- 2908,2916 ----
    else
      tdep->lr_frame_offset = 8;
  
!   if (v->arch == bfd_arch_rs6000)
!     tdep->ppc_sr0_regnum = -1;
!   else if (v->arch == bfd_arch_powerpc)
      switch (v->mach)
        {
        case bfd_mach_ppc: 
*************** rs6000_gdbarch_init (struct gdbarch_info
*** 2931,2936 ****
--- 2934,2940 ----
  	tdep->ppc_ev31_regnum = 38;
          tdep->ppc_fp0_regnum = -1;
          tdep->ppc_fpscr_regnum = -1;
+         tdep->ppc_sr0_regnum = -1;
          tdep->ppc_acc_regnum = 39;
          tdep->ppc_spefscr_regnum = 40;
          set_gdbarch_pc_regnum (gdbarch, 0);
*************** rs6000_gdbarch_init (struct gdbarch_info
*** 2939,2945 ****
--- 2943,2961 ----
          set_gdbarch_pseudo_register_read (gdbarch, e500_pseudo_register_read);
          set_gdbarch_pseudo_register_write (gdbarch, e500_pseudo_register_write);
  	break;
+ 
+       case bfd_mach_ppc64:
+       case bfd_mach_ppc_620:
+       case bfd_mach_ppc_630:
+       case bfd_mach_ppc_a35:
+       case bfd_mach_ppc_rs64ii:
+       case bfd_mach_ppc_rs64iii:
+         /* These processor's register sets don't have segment registers.  */
+         tdep->ppc_sr0_regnum = -1;
+         break;
        }   
+   else
+     gdb_assert (0);
  
    /* Sanity check on registers.  */
    gdb_assert (strcmp (tdep->regs[tdep->ppc_gp0_regnum].name, "r0") == 0);

Re: RFA: PowerPC: add segment register numbers

[Kevin Buettner]

On 12 Jul 2004 17:08:11 -0500
Jim Blandy <jimb@redhat.com> wrote:

> 	* ppc-tdep.h (struct gdbarch_tdep): New member: ppc_sr0_regnum.
> 	* rs6000-tdep.c (rs6000_gdbarch_init): Initialize it.

I have a question and a comment...

First the question: Regarding this portion of the patch...

*************** rs6000_gdbarch_init (struct gdbarch_info
*** 2879,2884 ****
--- 2879,2885 ----
      tdep->ppc_mq_regnum = -1;
    tdep->ppc_fp0_regnum = 32;
    tdep->ppc_fpscr_regnum = power ? 71 : 70;
+   tdep->ppc_sr0_regnum = 71;

...what about the case when ``power'' is non-zero?  Do we want
ppc_fpscr_regnum and ppc_sr0_regnum to both be equal to 71?

Now the comment: Regarding the following portion of the patch...

> *************** rs6000_gdbarch_init (struct gdbarch_info
> *** 2939,2945 ****
> --- 2943,2961 ----
>           set_gdbarch_pseudo_register_read (gdbarch, e500_pseudo_register_read);
>           set_gdbarch_pseudo_register_write (gdbarch, e500_pseudo_register_write);
>   	break;
> + 
> +       case bfd_mach_ppc64:
> +       case bfd_mach_ppc_620:
> +       case bfd_mach_ppc_630:
> +       case bfd_mach_ppc_a35:
> +       case bfd_mach_ppc_rs64ii:
> +       case bfd_mach_ppc_rs64iii:
> +         /* These processor's register sets don't have segment registers.  */
> +         tdep->ppc_sr0_regnum = -1;
> +         break;
>         }   
> +   else
> +     gdb_assert (0);

...I don't really like the gdb_assert (0).

With this patch, the code is structured as follows:

  if (v->arch == bfd_arch_powerpc)
    switch (v->mach)
      {
        ...
      }   
  else
    gdb_assert (0);

Could you revise your patch so that the above is instead structured
like this...?

  gdb_assert(v->arch == bfd_arch_powerpc);
  switch (v->mach)
    {
      ...
    }

BTW, the reason I don't like the assert (0) is that I like seeing a
somewhat meaningful condition when an assert is triggered.  Seeing 0
as the failed condition doesn't give much of a clue about what's going
on.

Kevin

Re: RFA: PowerPC: add segment register numbers

[Jim Blandy]

Kevin Buettner <kevinb@redhat.com> writes:
> *************** rs6000_gdbarch_init (struct gdbarch_info
> *** 2879,2884 ****
> --- 2879,2885 ----
>       tdep->ppc_mq_regnum = -1;
>     tdep->ppc_fp0_regnum = 32;
>     tdep->ppc_fpscr_regnum = power ? 71 : 70;
> +   tdep->ppc_sr0_regnum = 71;
> 
> ...what about the case when ``power'' is non-zero?  Do we want
> ppc_fpscr_regnum and ppc_sr0_regnum to both be equal to 71?

'power' is initialized like this:

  power = arch == bfd_arch_rs6000;

So ppc_sr0_regnum gets cleared below, whenever 'power' is true.

*************** rs6000_gdbarch_init (struct gdbarch_info
*** 2907,2913 ****
    else
      tdep->lr_frame_offset = 8;
  
!   if (v->arch == bfd_arch_powerpc)
      switch (v->mach)
        {
        case bfd_mach_ppc: 
--- 2908,2916 ----
    else
      tdep->lr_frame_offset = 8;
  
!   if (v->arch == bfd_arch_rs6000)
!     tdep->ppc_sr0_regnum = -1;
!   else if (v->arch == bfd_arch_powerpc)
      switch (v->mach)
        {
        case bfd_mach_ppc: 

I'd be happy to tweak rs6000_gdbarch_init to consistently use 'power'
or 'arch == bfd_arch_rs6000' if you think either would be clearer.  (I
would tend to prefer the latter: that would be one less step readers
would need to follow back through, and the code doesn't get too much
mileage out of the 'power' abbreviation anyway.)


> Now the comment: Regarding the following portion of the patch...
> 
> > *************** rs6000_gdbarch_init (struct gdbarch_info
> > *** 2939,2945 ****
> > --- 2943,2961 ----
> >           set_gdbarch_pseudo_register_read (gdbarch, e500_pseudo_register_read);
> >           set_gdbarch_pseudo_register_write (gdbarch, e500_pseudo_register_write);
> >   	break;
> > + 
> > +       case bfd_mach_ppc64:
> > +       case bfd_mach_ppc_620:
> > +       case bfd_mach_ppc_630:
> > +       case bfd_mach_ppc_a35:
> > +       case bfd_mach_ppc_rs64ii:
> > +       case bfd_mach_ppc_rs64iii:
> > +         /* These processor's register sets don't have segment registers.  */
> > +         tdep->ppc_sr0_regnum = -1;
> > +         break;
> >         }   
> > +   else
> > +     gdb_assert (0);
> 
> ...I don't really like the gdb_assert (0).
> 
> With this patch, the code is structured as follows:
> 
>   if (v->arch == bfd_arch_powerpc)
>     switch (v->mach)
>       {
>         ...
>       }   
>   else
>     gdb_assert (0);
> 
> Could you revise your patch so that the above is instead structured
> like this...?
> 
>   gdb_assert(v->arch == bfd_arch_powerpc);
>   switch (v->mach)
>     {
>       ...
>     }
> 
> BTW, the reason I don't like the assert (0) is that I like seeing a
> somewhat meaningful condition when an assert is triggered.  Seeing 0
> as the failed condition doesn't give much of a clue about what's going
> on.

That's true.  The problem with writing the condition out in the assert
is that one repeats the series of conditions that go before it.  So
you're checking for one potential inconsistency, but introducing a new
one.

As we discussed on the phone, calling internal_error and providing a
suitable error message explicitly, addresses both concerns.  I'll
revise the patch.

Re: RFA: PowerPC: add segment register numbers

[Jim Blandy]

How's this?

2004-07-12  Jim Blandy  <jimb@redhat.com>

	* ppc-tdep.h (struct gdbarch_tdep): New member: ppc_sr0_regnum.
	* rs6000-tdep.c (rs6000_gdbarch_init): Initialize it.

Index: gdb/ppc-tdep.h
===================================================================
RCS file: /cvs/src/src/gdb/ppc-tdep.h,v
retrieving revision 1.41
diff -c -p -r1.41 ppc-tdep.h
*** gdb/ppc-tdep.h	14 Jul 2004 23:18:09 -0000	1.41
--- gdb/ppc-tdep.h	14 Jul 2004 23:34:10 -0000
*************** struct gdbarch_tdep
*** 158,163 ****
--- 158,167 ----
      int ppc_fpscr_regnum;	/* Floating point status and condition
      				   register */
  
+     int ppc_sr0_regnum;         /* segment register 0, or -1 on
+                                    variants that have no segment
+                                    registers.  */
+ 
      int ppc_mq_regnum;		/* Multiply/Divide extension register */
      int ppc_vr0_regnum;		/* First AltiVec register */
      int ppc_vrsave_regnum;	/* Last AltiVec register */
Index: gdb/rs6000-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/rs6000-tdep.c,v
retrieving revision 1.217
diff -c -p -r1.217 rs6000-tdep.c
*** gdb/rs6000-tdep.c	14 Jul 2004 23:00:59 -0000	1.217
--- gdb/rs6000-tdep.c	14 Jul 2004 23:34:12 -0000
*************** rs6000_gdbarch_init (struct gdbarch_info
*** 2879,2884 ****
--- 2879,2885 ----
      tdep->ppc_mq_regnum = -1;
    tdep->ppc_fp0_regnum = 32;
    tdep->ppc_fpscr_regnum = power ? 71 : 70;
+   tdep->ppc_sr0_regnum = 71;
    tdep->ppc_vr0_regnum = -1;
    tdep->ppc_vrsave_regnum = -1;
    tdep->ppc_ev0_regnum = -1;
*************** rs6000_gdbarch_init (struct gdbarch_info
*** 2907,2913 ****
    else
      tdep->lr_frame_offset = 8;
  
!   if (v->arch == bfd_arch_powerpc)
      switch (v->mach)
        {
        case bfd_mach_ppc: 
--- 2908,2916 ----
    else
      tdep->lr_frame_offset = 8;
  
!   if (v->arch == bfd_arch_rs6000)
!     tdep->ppc_sr0_regnum = -1;
!   else if (v->arch == bfd_arch_powerpc)
      switch (v->mach)
        {
        case bfd_mach_ppc: 
*************** rs6000_gdbarch_init (struct gdbarch_info
*** 2931,2936 ****
--- 2934,2940 ----
  	tdep->ppc_ev31_regnum = 38;
          tdep->ppc_fp0_regnum = -1;
          tdep->ppc_fpscr_regnum = -1;
+         tdep->ppc_sr0_regnum = -1;
          tdep->ppc_acc_regnum = 39;
          tdep->ppc_spefscr_regnum = 40;
          set_gdbarch_pc_regnum (gdbarch, 0);
*************** rs6000_gdbarch_init (struct gdbarch_info
*** 2939,2945 ****
--- 2943,2963 ----
          set_gdbarch_pseudo_register_read (gdbarch, e500_pseudo_register_read);
          set_gdbarch_pseudo_register_write (gdbarch, e500_pseudo_register_write);
  	break;
+ 
+       case bfd_mach_ppc64:
+       case bfd_mach_ppc_620:
+       case bfd_mach_ppc_630:
+       case bfd_mach_ppc_a35:
+       case bfd_mach_ppc_rs64ii:
+       case bfd_mach_ppc_rs64iii:
+         /* These processor's register sets don't have segment registers.  */
+         tdep->ppc_sr0_regnum = -1;
+         break;
        }   
+   else
+     internal_error (__FILE__, __LINE__,
+                     "rs6000_gdbarch_init: "
+                     "received unexpected BFD 'arch' value");
  
    /* Sanity check on registers.  */
    gdb_assert (strcmp (tdep->regs[tdep->ppc_gp0_regnum].name, "r0") == 0);

Re: RFA: PowerPC: add segment register numbers

[Kevin Buettner]

On 14 Jul 2004 18:23:30 -0500
Jim Blandy <jimb@redhat.com> wrote:

> 
> Kevin Buettner <kevinb@redhat.com> writes:
> > *************** rs6000_gdbarch_init (struct gdbarch_info
> > *** 2879,2884 ****
> > --- 2879,2885 ----
> >       tdep->ppc_mq_regnum = -1;
> >     tdep->ppc_fp0_regnum = 32;
> >     tdep->ppc_fpscr_regnum = power ? 71 : 70;
> > +   tdep->ppc_sr0_regnum = 71;
> > 
> > ...what about the case when ``power'' is non-zero?  Do we want
> > ppc_fpscr_regnum and ppc_sr0_regnum to both be equal to 71?
> 
> 'power' is initialized like this:
> 
>   power = arch == bfd_arch_rs6000;
> 
> So ppc_sr0_regnum gets cleared below, whenever 'power' is true.

I see.  Thanks for the explanation.

> *************** rs6000_gdbarch_init (struct gdbarch_info
> *** 2907,2913 ****
>     else
>       tdep->lr_frame_offset = 8;
>   
> !   if (v->arch == bfd_arch_powerpc)
>       switch (v->mach)
>         {
>         case bfd_mach_ppc: 
> --- 2908,2916 ----
>     else
>       tdep->lr_frame_offset = 8;
>   
> !   if (v->arch == bfd_arch_rs6000)
> !     tdep->ppc_sr0_regnum = -1;
> !   else if (v->arch == bfd_arch_powerpc)
>       switch (v->mach)
>         {
>         case bfd_mach_ppc: 
> 
> I'd be happy to tweak rs6000_gdbarch_init to consistently use 'power'
> or 'arch == bfd_arch_rs6000' if you think either would be clearer.  (I
> would tend to prefer the latter: that would be one less step readers
> would need to follow back through, and the code doesn't get too much
> mileage out of the 'power' abbreviation anyway.)

A patch which does the latter would be welcome.  Consider it preapproved.

> > Now the comment: Regarding the following portion of the patch...
> > 
> > > *************** rs6000_gdbarch_init (struct gdbarch_info
> > > *** 2939,2945 ****
> > > --- 2943,2961 ----
> > >           set_gdbarch_pseudo_register_read (gdbarch, e500_pseudo_register_read);
> > >           set_gdbarch_pseudo_register_write (gdbarch, e500_pseudo_register_write);
> > >   	break;
> > > + 
> > > +       case bfd_mach_ppc64:
> > > +       case bfd_mach_ppc_620:
> > > +       case bfd_mach_ppc_630:
> > > +       case bfd_mach_ppc_a35:
> > > +       case bfd_mach_ppc_rs64ii:
> > > +       case bfd_mach_ppc_rs64iii:
> > > +         /* These processor's register sets don't have segment registers.  */
> > > +         tdep->ppc_sr0_regnum = -1;
> > > +         break;
> > >         }   
> > > +   else
> > > +     gdb_assert (0);
> > 
> > ...I don't really like the gdb_assert (0).
> > 
> > With this patch, the code is structured as follows:
> > 
> >   if (v->arch == bfd_arch_powerpc)
> >     switch (v->mach)
> >       {
> >         ...
> >       }   
> >   else
> >     gdb_assert (0);
> > 
> > Could you revise your patch so that the above is instead structured
> > like this...?
> > 
> >   gdb_assert(v->arch == bfd_arch_powerpc);
> >   switch (v->mach)
> >     {
> >       ...
> >     }
> > 
> > BTW, the reason I don't like the assert (0) is that I like seeing a
> > somewhat meaningful condition when an assert is triggered.  Seeing 0
> > as the failed condition doesn't give much of a clue about what's going
> > on.
> 
> That's true.  The problem with writing the condition out in the assert
> is that one repeats the series of conditions that go before it.  So
> you're checking for one potential inconsistency, but introducing a new
> one.
> 
> As we discussed on the phone, calling internal_error and providing a
> suitable error message explicitly, addresses both concerns.  I'll
> revise the patch.

Thanks!

Kevin

Re: RFA: PowerPC: add segment register numbers

[Kevin Buettner]

On 14 Jul 2004 18:32:38 -0500
Jim Blandy <jimb@redhat.com> wrote:

> How's this?
> 
> 2004-07-12  Jim Blandy  <jimb@redhat.com>
> 
> 	* ppc-tdep.h (struct gdbarch_tdep): New member: ppc_sr0_regnum.
> 	* rs6000-tdep.c (rs6000_gdbarch_init): Initialize it.

Looks good.  Thanks for revising it.

Kevin

Re: RFA: PowerPC: add segment register numbers

[Jim Blandy]

Kevin Buettner <kevinb@redhat.com> writes:

> On 14 Jul 2004 18:32:38 -0500
> Jim Blandy <jimb@redhat.com> wrote:
> 
> > How's this?
> > 
> > 2004-07-12  Jim Blandy  <jimb@redhat.com>
> > 
> > 	* ppc-tdep.h (struct gdbarch_tdep): New member: ppc_sr0_regnum.
> > 	* rs6000-tdep.c (rs6000_gdbarch_init): Initialize it.
> 
> Looks good.  Thanks for revising it.

Committed --- thanks.

Re: RFA: PowerPC: add segment register numbers

[Jim Blandy]

Kevin Buettner <kevinb@redhat.com> writes:
> A patch which does the latter would be welcome.  Consider it preapproved.

I was writing this patch and noticed some odd code in rs6000_gdbarch_init:

  /* Select instruction printer.  */
  if (arch == power)
    set_gdbarch_print_insn (gdbarch, print_insn_rs6000);
  else
    set_gdbarch_print_insn (gdbarch, gdb_print_insn_powerpc);

Now, 'arch' is an enum, and 'power' is a boolean.  So we've been using
gdb_print_insn_powerpc on rs6000 machines for some time --- the
incorrect code was introduced in r1.49, in Apr 2002.

So I'd like to actually try to test this patch on an RS6000 before
committing it.  :)