Re: RFA: PowerPC: add segment register numbers

[Kevin Buettner <kevinb at redhat dot com>]

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