Re: RFA: PowerPC: add segment register numbers

Jim Blandy
2004-07-14T23:28:38Z

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.