This is the mail archive of the gdb-patches@sources.redhat.com mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: RFA: PowerPC: add segment register numbers


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.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]