[PATCH] gdb/tdesc: Don't assign custom-group tdesc registers to 'general'

[Luis Machado]

On 9/12/23 14:48, Andrew Burgess wrote:
> Luis Machado via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
>> Hi,
>>
>> On 9/7/23 18:07, Ciaran Woodward wrote:
>>> The 'Target Description' mechanism in GDB enables the target to
>>> supply the full set of registers available on the system to gdb
>>> in an XML format.
>>>
>>> This format enables setting the 'group' of each register, such
>>> that they can be queried using the 'info registers <group>'
>>> mechanism.
>>>
>>> However prior to this change, even if a register was explicitly
>>> assigned to a group, it would still show up in the
>>> 'info registers general' report. This is unexpected, and also
>>> disagrees with the comment above the tdesc_register_in_reggroup_p
>>> function, which says that '-1' should be returned if the register
>>> group is not-known, not the register group is known, but differs.
>>>
>>> There was a previous change that did address this issue in
>>> aa66aac47b4dd38f9524ddb5546c08cc09930d37
>>> but it also caused registers with *no* group in the target
>>> description to be removed from 'general', so it was reverted in
>>> 440cf44eb0f70830b8d8ac35289f84129c7a35c1
>>> as that behaviour was used by some targets.
>>>
>>> The change in this commit enhances the usefulness of the tdesc
>>> 'group' attribute for adding system configuration registers,
>>> of which there may be hundreds - very inconvenient to request
>>> and print on every 'info registers' call.
>>> ---
>>>
>>> Email archive link to discussion of previously referenced patch:
>>> https://inbox.sourceware.org/gdb-patches/20200120155315.30333-1-shahab.vahedi@gmail.com/T/#m0ce2983153b2f482b66461ed6b97c4b287b09a89
>>>
>>>  gdb/target-descriptions.c | 7 +++----
>>>  1 file changed, 3 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/gdb/target-descriptions.c b/gdb/target-descriptions.c
>>> index cdedf88c793..c1bb03c3adf 100644
>>> --- a/gdb/target-descriptions.c
>>> +++ b/gdb/target-descriptions.c
>>> @@ -954,14 +954,13 @@ tdesc_register_in_reggroup_p (struct gdbarch *gdbarch, int regno,
>>>  {
>>>    struct tdesc_reg *reg = tdesc_find_register (gdbarch, regno);
>>>  
>>> -  if (reg != NULL && !reg->group.empty ()
>>> -      && (reg->group == reggroup->name ()))
>>> -	return 1;
>>> -
>>>    if (reg != NULL
>>>        && (reggroup == save_reggroup || reggroup == restore_reggroup))
>>>      return reg->save_restore;
>>>  
>>> +  if (reg != NULL && !reg->group.empty ())
>>> +    return (reg->group == reggroup->name ());
>>> +
>>>    return -1;
>>>  }
>>>  
>>
>> Yeah, this is a hard one. Unfortunately the outcome then was that we
>> have a number of debugging stubs out in the open that advertise xml's
>> with less-than-correct group information.
>>
>> I think even gdb, to this day, is guilty of that.
>>
>> Though the patch doesn't regress things as much as before, it still
>> makes the 32-bit Arm gdb drop the fpscr register from the general
>> display (info registers).
>>
>> Depending on what others think, and the benefit of not having a bunch
>> of system registers show up every time, we could cope with this
>> particular regression.
> 
> I took another look at this today.  When you talk about the 'fpscr'
> register, in features/arm/arm-vfpv{2,3}.xml the register of type 'int',
> so would be in the general group, but is placed into the 'float' group
> with a line like:
> 
>   <reg name="fpscr" bitsize="32" type="int" group="float"/>
> 
> So, to me, fpscr should show up for:
> 
>   info registers all
>   info registers float
> 
> But not for:
> 
>   info registers
>   info registers general
> 

Yeah. I'm not entirely sure how that came to be, but I suppose it gets displayed in "info registers" because of its type, even
though it is in the float group. In any case, I don't think it is a big deal, and we could change/fix this.

> When you mention existing stubs in the wild that "advertise xml's with
> less-than-correct group information." -- are these stubs sending
> something different to the above?  Or are they just copying that above?
> 

For 32-bit Arm I think there are both cases (the same as gdb and others somewhat different). But it is difficult to get our hands into all of those examples.

> If we did want to retain the above functionality, then we can just
> set_gdbarch_register_reggroup_p to point to this (untested) function:
> 
>   static int
>   arm_register_reggroup_p (struct gdbarch  *gdbarch, int regnum,
>                            const struct reggroup *reggroup)
>   {
>     if (regnum == ARM_FPSCR_REGNUM && reggroup == general_reggroup)
>       reggroup = float_reggroup;
>   
>     int ret = tdesc_register_in_reggroup_p (gdbarch, regnum, reggroup);
>     if (ret != -1)
>       return ret;
>     return default_register_reggroup_p (gdbarch, regno, reggroup);
>   }

I suppose that'd work. To me, the fact fpscr is showing up in the general display looks like an oversight. This might also be a good opportunity to make the register group information in the xml's more accurate.

> 
> Thanks,
> Andrew
>