Re: RFA: PowerPC: add segment register numbers

Kevin Buettner
2004-07-15T00:22:45Z

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