register_offset_hack() vs REGISTER_BYTE()

register_offset_hack() vs REGISTER_BYTE()

[Kevin Buettner]

I'm seeing the following internal error:

    .../frame.c:591: internal-error: Failed to compute the register number
    corresponding to 0x296

This is happening because the *addrp value in the following loop (which
is in frame_register() in frame.c)...

          for (regnum = 0; regnum < NUM_REGS + NUM_PSEUDO_REGS; regnum++)
            {
              if (*addrp == register_offset_hack (current_gdbarch, regnum))
                {
                  *realnump = regnum;
                  return;
                }
            }

...is set using a value obtained from REGISTER_BYTE().  (See
sentinel_frame_prev_register in sentinel-frame.c.)  But the
value obtained from register_offset_hack() was computed by using the
register's virtual type.  For this particular architecture (64-bit MIPS),
there are some registers whose virtual size is 4, but which are stored
in an 8-byte container.  Hence the discrepancy.

It's not clear to me that the values being returned by register_offset_hack()
are all that useful.  These are offsets which would occur if you squeezed
all of the "unused" space out of the registers array.

IMO, the call to register_offset_hack() should be replaced with a call
to REGISTER_BYTE().

Opinions?

Kevin

Re: register_offset_hack() vs REGISTER_BYTE()

[Andrew Cagney]

> I'm seeing the following internal error:
> 
>     .../frame.c:591: internal-error: Failed to compute the register number
>     corresponding to 0x296
> 
> This is happening because the *addrp value in the following loop (which
> is in frame_register() in frame.c)...
> 
>           for (regnum = 0; regnum < NUM_REGS + NUM_PSEUDO_REGS; regnum++)
>             {
>               if (*addrp == register_offset_hack (current_gdbarch, regnum))
>                 {
>                   *realnump = regnum;
>                   return;
>                 }
>             }
> 
> ...is set using a value obtained from REGISTER_BYTE().  (See
> sentinel_frame_prev_register in sentinel-frame.c.)  But the
> value obtained from register_offset_hack() was computed by using the
> register's virtual type.

Er, for a legacy architecture, register_offset_hack(i) (aka 
current_regcache->descr->register_offset) should be REGISTER_BYTE(i). 
See init_legacy_regcache_descr.

Sounds like the MIPS is sneeking past:

   /* If an old style architecture, fill in the remainder of the
      register cache descriptor using the register macros.  */
   if (!gdbarch_pseudo_register_read_p (gdbarch)
       && !gdbarch_pseudo_register_write_p (gdbarch)
       && !gdbarch_register_type_p (gdbarch))
     {
       descr->legacy_p = 1;
       init_legacy_regcache_descr (gdbarch, descr);
       return descr;
     }

Andrew

Re: register_offset_hack() vs REGISTER_BYTE()

[Kevin Buettner]

On May 1,  8:58pm, Andrew Cagney wrote:

> > I'm seeing the following internal error:
> > 
> >     .../frame.c:591: internal-error: Failed to compute the register number
> >     corresponding to 0x296
> > 
> > This is happening because the *addrp value in the following loop (which
> > is in frame_register() in frame.c)...
> > 
> >           for (regnum = 0; regnum < NUM_REGS + NUM_PSEUDO_REGS; regnum++)
> >             {
> >               if (*addrp == register_offset_hack (current_gdbarch, regnum))
> >                 {
> >                   *realnump = regnum;
> >                   return;
> >                 }
> >             }
> > 
> > ...is set using a value obtained from REGISTER_BYTE().  (See
> > sentinel_frame_prev_register in sentinel-frame.c.)  But the
> > value obtained from register_offset_hack() was computed by using the
> > register's virtual type.
> 
> Er, for a legacy architecture, register_offset_hack(i) (aka 
> current_regcache->descr->register_offset) should be REGISTER_BYTE(i). 
> See init_legacy_regcache_descr.
> 
> Sounds like the MIPS is sneeking past:
> 
>    /* If an old style architecture, fill in the remainder of the
>       register cache descriptor using the register macros.  */
>    if (!gdbarch_pseudo_register_read_p (gdbarch)
>        && !gdbarch_pseudo_register_write_p (gdbarch)
>        && !gdbarch_register_type_p (gdbarch))
>      {
>        descr->legacy_p = 1;
>        init_legacy_regcache_descr (gdbarch, descr);
>        return descr;
>      }

Hmm, yes.  I just added some pseudo registers...

Kevin

Re: register_offset_hack() vs REGISTER_BYTE()

[Andrew Cagney]

>> Er, for a legacy architecture, register_offset_hack(i) (aka 
>> current_regcache->descr->register_offset) should be REGISTER_BYTE(i). 
>> See init_legacy_regcache_descr.
>> 
>> Sounds like the MIPS is sneeking past:
>> 
>>    /* If an old style architecture, fill in the remainder of the
>>       register cache descriptor using the register macros.  */
>>    if (!gdbarch_pseudo_register_read_p (gdbarch)
>>        && !gdbarch_pseudo_register_write_p (gdbarch)
>>        && !gdbarch_register_type_p (gdbarch))
>>      {
>>        descr->legacy_p = 1;
>>        init_legacy_regcache_descr (gdbarch, descr);
>>        return descr;
>>      }
> 
> 
> Hmm, yes.  I just added some pseudo registers...

Can probably change REGISTER_BYTE to a pre-initialized method with 
predicate (see DEPRECATED_PC_IN_CALL_DUMMY for an example), and then add 
a predicate test for it to the above mix.

Sentinel frame should probably also be switched to using 
register_offset_hack(), instead of REGISTER_BYTE(), so that it is more 
consistent with the frame code.

Sounds like it's time to s/ REGISTER_BYTE / DEPRECATED_REGISTER_BYTE /. 
  It isn't yet possible to eliminate that macro from the MIPS since it 
is that hardwired mystery that defines the MIPS remote protocol :-(

Andrew

Re: register_offset_hack() vs REGISTER_BYTE()

[Andrew Cagney]

> Can probably change REGISTER_BYTE to a pre-initialized method with predicate (see DEPRECATED_PC_IN_CALL_DUMMY for an example), and then add a predicate test for it to the above mix.

On second thoughts, and having investigate things a bit further ... 
Doing it would revert any architecture [still] defining REGISTER_BYTE 
(almost all of them) back to using the legacy register cache. 
Consequently I'll instead just add sanity checks to ensure that the 
regcache and REGISTER_BYTE are consistent.

> Sentinel frame should probably also be switched to using register_offset_hack(), instead of REGISTER_BYTE(), so that it is more consistent with the frame code.

Appears to work.

> Sounds like it's time to s/ REGISTER_BYTE / DEPRECATED_REGISTER_BYTE /.  It isn't yet possible to eliminate that macro from the MIPS since it is that hardwired mystery that defines the MIPS remote protocol :-(

...

Andrew

Re: register_offset_hack() vs REGISTER_BYTE()

[Kevin Buettner]

On May 2,  6:26pm, Andrew Cagney wrote:

> > Can probably change REGISTER_BYTE to a pre-initialized method with
> > predicate (see DEPRECATED_PC_IN_CALL_DUMMY for an example), and
> > then add a predicate test for it to the above mix.
> 
> On second thoughts, and having investigate things a bit further ... 
> Doing it would revert any architecture [still] defining REGISTER_BYTE 
> (almost all of them) back to using the legacy register cache. 
> Consequently I'll instead just add sanity checks to ensure that the 
> regcache and REGISTER_BYTE are consistent.
> 
> > Sentinel frame should probably also be switched to using
> > register_offset_hack(), instead of REGISTER_BYTE(), so that it is
> > more consistent with the frame code.
> 
> Appears to work.
> 
> > Sounds like it's time to s/ REGISTER_BYTE /
> > DEPRECATED_REGISTER_BYTE /.  It isn't yet possible to eliminate
> > that macro from the MIPS since it is that hardwired mystery that
> > defines the MIPS remote protocol :-(
> 
> ...

For MIPS register numbers in [0, NUM_REGS), I'm going to make
mips_register_virtual_type() always return a type whose size is the same as
that currently returned by mips_register_raw_size().  I'll introduce pseudo
registers for those that actually need different virtual sizes.  (I've
turned mips o32 floating point registers into pseudo registers already and
it seems to work...)

This'll allow me to eliminate the MIPS REGISTER_BYTE and REGISTER_RAW_SIZE
definitions.  Plus some other stuff too.  E.g, I've already gotten rid of
mips_register_convert_to_type() and mips_register_convert_from_type().  I
think mips_register_convert_to_virtual() and mips_register_convert_to_raw()
will end up going away too.

Kevin

Re: register_offset_hack() vs REGISTER_BYTE()

[Andrew Cagney]

> For MIPS register numbers in [0, NUM_REGS), I'm going to make
> mips_register_virtual_type() always return a type whose size is the same as
> that currently returned by mips_register_raw_size().

Yes!!!  That should have always been that way.

>  I'll introduce pseudo
> registers for those that actually need different virtual sizes.  (I've
> turned mips o32 floating point registers into pseudo registers already and
> it seems to work...)

Yep.

> This'll allow me to eliminate the MIPS REGISTER_BYTE and REGISTER_RAW_SIZE
> definitions.  Plus some other stuff too.  E.g, I've already gotten rid of
> mips_register_convert_to_type() and mips_register_convert_from_type().  I
> think mips_register_convert_to_virtual() and mips_register_convert_to_raw()
> will end up going away too.

Don't stop there.  It should be possible to trim it down to something like:
	- register_type
	- num_regs
	- num_pseudo_regs
	- register_name
(looking at the d10v I've a few more to eliminate).

This means that a huge chunk of nastiness can finally be expunged from 
GDB.  Ya!

Andrew