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

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

[Ciaran Woodward]

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;
 }
 
-- 
2.25.1

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

[Luis Machado]

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.

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

[Andrew Burgess]

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 wonder if the right solution would be to work around existing "broken"
stubs in the *-tdep.c code?

We could add a function to target-descriptions.c which sets a register's
group name ONLY when the group name is empty.

Then from the *-tdep.c code we can fill in groups which for important
registers, which are known to possibly be empty.

Or we could extend tdesc_numbered_register and maybe
tdesc_unnumbered_register to take an optional "default" group, the
default would be applied if the register is found, and doesn't already
have a group set.

Thanks,
Andrew

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

[Ciaran Woodward]

Hi,

Luis:
> 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.

The most severely affected target I believe is the s390, which puts
many registers into 'lower' and 'upper' groups in the tdesc. This
seemingly distinguishes the upper and lower 32-bits of 64-bit registers. 

However, the s390 already implements proper 64 bit versions of
these registers as 'pseudo' registers, which are added to the general set.
With the current master behaviour, 'info registers' will print the
pseudo (64-bit) versions, the upper half and the lower half separately,
which is not particularly useful. After this patch, only the pseudo
versions will be printed - which I feel is better behaviour.

I would be interested to hear if anyone thinks the new behaviour is a
'harmful' regression, because we could avoid the regression, its just
more work, and provides arguably worse user experience.

(I did a sweep of other potentially affected platforms below)

Andrew:
> I wonder if the right solution would be to work around existing "broken"
> stubs in the *-tdep.c code?
> 
> We could add a function to target-descriptions.c which sets a register's
> group name ONLY when the group name is empty.
> 
> Then from the *-tdep.c code we can fill in groups which for important
> registers, which are known to possibly be empty.
> 
> Or we could extend tdesc_numbered_register and maybe
> tdesc_unnumbered_register to take an optional "default" group, the
> default would be applied if the register is found, and doesn't already
> have a group set.

Unless I'm misunderstanding you, the patch as currently shown will
already work as you specify and fall back to the default group handling
(adding to general, vector or float depending on type - using
default_register_reggroup_p). It will do this for all cases where the
tdesc group is empty.

I assumed there is no reason to actually be able to add a register to
'no group' - since tdesc can specify an arbitrary group name, it is
always possible to specify a nonstandard group to put excess registers
that shouldn't be shown under normal 'info registers'.

The legacy behaviour we might want to replicate was that registers were
added to multiple groups: the one they were added to in the tdesc, and
the one they get added to by type. So the 'fpscr' register Luis mentioned
would be displayed in *both* 'info registers general' and 'info registers float'.

If we do want to maintain the legacy behaviour, I see two ways to
re-implement this for stubs which send legacy target descriptions:

1. A tdep-override approach (similar to what you suggest), implementing
a gdbarch_register_reggroup_p for the target which calls into
tdesc_register_in_reggroup_p but ignores 0 (NOT_IN_GROUP) returns
for known-bad registers. This would be a reasonable approach if we
think we know the full set of affected registers, and it would avoid the
argument/attribute extra bloat that option 2 would entail.

2. A different tdep-override, which just uses exactly the legacy
behaviour for the architecture, by calling a different version of
tdesc_use_registers.

3. Add some boolean attribute to the register to trigger the 'new'
behaviour, and default it to the old behaviour if not specified. Something
like a defaultGroups='false' in the XML, which would exclude the register
from the legacy behaviour, and exclusively add it to the groups specified
by 'groups' and 'save-restore'. Would need an equivilent argument to the
tdesc_create_reg function.

Personally I would lean towards 1, since the number of places where this
is a harmful regression are probably minimal. In most cases, I think the
new behaviour is preferable, such as showing integer float/vector registers
alongside their 'info registers float/vector' friends, and not in "general"
at all. I identified affected registers by checking 'tdesc_create_reg'
calls:

s390, these would have been "general" also:
pswm, pswa: "psw"
acr0-15: "access"
fpc: "float"
orig_r2, last_break, system_call: "system"
tdb0, tac, tct, atia, tr0-15: "tdb"
gds, gddm, gsepla, bc_gsd, bc_gssm, bc_gsepla: "gs"
("upper" & "lower" groups would also have been shown in general, but shouldn't be imo)

mips, these would have been "general" also:
fcsr, fir: "float"
restart: "system"

aarch64, this would have been "general" also:
tag_ctl: "system"

rs6000, this would have been "general" also:
fpscr: "float"

powerpc, these would have been "general" also:
(NOTE: I don't actually see this architecture calling tdesc_use_registers, so does it even use the tdesc functionality?)
fpscr, cfpscr: "float"
vscr, vrsave, cvsr, cvrsave: "vector"

loongarch, these would have been "general" also:
fcc0-7, fcsr: "float"

i386, these would be "general" also:
fctrl, fstat, ftag, fiseg, fioff, foseg, fooff, fop: "float"
mxcsr: "vector"

arm:
fpscr: "float"
wCSSF, wCASF, wCGR0-3: "vector"

Thanks,
Ciaran

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

[Andrew Burgess]

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

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?

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);
  }

Thanks,
Andrew

Re: [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
> 

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

[Andrew Burgess]

Ciaran Woodward <ciaranwoodward@xmos.com> writes:

> 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;
>  }

I haven't tested this, but looking at the series you referenced, I don't
think the above change can be correct.  If reggroup is all_reggroup, and
a register has a group set, then this is going to return 0, right?

If we then look at tdesc_register_reggroup_p, which calls the above
function, we can see that it relies on the function you changed to
return -1 in this case, at which point we fall through to
default_register_reggroup_p, where all_reggroup is handled.

Of course, this all depends on which target you're using.  Some targets
implement their own set_gdbarch_register_reggroup_p callback, and handle
all_reggroup before calling tdesc_register_in_reggroup_p, but others
(e.g. MIPS) don't do it this way, so I think your change will break
them.

Thanks,
Andrew

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

[Ciaran Woodward]

Hi Andrew,
 
> I haven't tested this, but looking at the series you referenced, I don't
> think the above change can be correct.  If reggroup is all_reggroup, and
> a register has a group set, then this is going to return 0, right?
> 
> If we then look at tdesc_register_reggroup_p, which calls the above
> function, we can see that it relies on the function you changed to
> return -1 in this case, at which point we fall through to
> default_register_reggroup_p, where all_reggroup is handled.
> 
> Of course, this all depends on which target you're using.  Some targets
> implement their own set_gdbarch_register_reggroup_p callback, and handle
> all_reggroup before calling tdesc_register_in_reggroup_p, but others
> (e.g. MIPS) don't do it this way, so I think your change will break
> them.

Yes, you're right - good catch. I suppose the best route in a v2 patch
would be to return -1 if the group is reggroup_all, so that the caller
can handle it as it does currently.

Thanks,
Ciaran

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

[Andrew Burgess]

Ciaran Woodward <ciaranwoodward@xmos.com> writes:

> Hi Andrew,
>  
>> I haven't tested this, but looking at the series you referenced, I don't
>> think the above change can be correct.  If reggroup is all_reggroup, and
>> a register has a group set, then this is going to return 0, right?
>> 
>> If we then look at tdesc_register_reggroup_p, which calls the above
>> function, we can see that it relies on the function you changed to
>> return -1 in this case, at which point we fall through to
>> default_register_reggroup_p, where all_reggroup is handled.
>> 
>> Of course, this all depends on which target you're using.  Some targets
>> implement their own set_gdbarch_register_reggroup_p callback, and handle
>> all_reggroup before calling tdesc_register_in_reggroup_p, but others
>> (e.g. MIPS) don't do it this way, so I think your change will break
>> them.
>
> Yes, you're right - good catch. I suppose the best route in a v2 patch
> would be to return -1 if the group is reggroup_all, so that the caller
> can handle it as it does currently.

That would be fine, but you could also just return 1.  The original
patch that you referenced (aa66aac47b4dd38) changed the function
tdesc_register_in_reggroup_p to this:

  int
  tdesc_register_in_reggroup_p (struct gdbarch *gdbarch, int regno,
  			      const struct reggroup *reggroup)
  {
    struct tdesc_reg *reg = tdesc_find_register (gdbarch, regno);
  
    if (reg != NULL)
      {
        if (reggroup == all_reggroup)
          return 1;
        else if (reggroup == save_reggroup || reggroup == restore_reggroup)
          return reg->save_restore;
        else
          return (int) (reg->group == reggroup->name ());
      }
  
    return -1;
  }

I think the only mistake here is the final 'else', which should have
been:

        else if (!reg->group.empty ())
          return (int) (reg->group == reggroup->name ());

With that fixed, I think this is pretty much what your new patch is
going to look like.

From there, I agree with your conclusions in [1], any problems where
targets want registers to show up in extra groups, or to follow
non-standard rules, should be solved by overriding the
gdbarch_register_reggroup_p hook.

[1] https://inbox.sourceware.org/gdb-patches/PAXPR09MB55834D4B7236002FCFCD4197B9F2A@PAXPR09MB5583.eurprd09.prod.outlook.com/T/#m6575b4be85f8f15751e7d419e0c3df9148cbb53a

Thanks,
Andrew

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

[Ciaran Woodward]

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.
---
 gdb/target-descriptions.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/gdb/target-descriptions.c b/gdb/target-descriptions.c
index cdedf88c793..6eb626b9c5a 100644
--- a/gdb/target-descriptions.c
+++ b/gdb/target-descriptions.c
@@ -954,13 +954,17 @@ 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 ()))
+  if (reg != NULL)
+    {
+      if (reggroup == all_reggroup)
 	return 1;
 
-  if (reg != NULL
-      && (reggroup == save_reggroup || reggroup == restore_reggroup))
-    return reg->save_restore;
+      if (reggroup == save_reggroup || reggroup == restore_reggroup)
+	return reg->save_restore;
+
+      if (reg != NULL && !reg->group.empty ())
+	return (reg->group == reggroup->name ());
+    }
 
   return -1;
 }
-- 
2.25.1

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

[Ciaran Woodward]

Ping

Discussion of the V1 can be found here: https://patchwork.sourceware.org/project/gdb/patch/20230907170752.28639-1-ciaranwoodward@xmos.com/

From what I see, this change is complete & useful as-is, but opens the door for further useful work in future - to improve the tdesc XMLs.

> -----Original Message-----
> From: Ciaran Woodward
> Sent: Wednesday, September 13, 2023 12:42 PM
> To: gdb-patches@sourceware.org
> Cc: luis.machado@arm.com; aburgess@redhat.com; Ciaran Woodward
> <ciaranwoodward@xmos.com>
> Subject: [PATCH v2] gdb/tdesc: Don't assign custom-group tdesc registers
> to 'general'
> 
> 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.
> ---
>  gdb/target-descriptions.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/gdb/target-descriptions.c b/gdb/target-descriptions.c
> index cdedf88c793..6eb626b9c5a 100644
> --- a/gdb/target-descriptions.c
> +++ b/gdb/target-descriptions.c
> @@ -954,13 +954,17 @@ 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 ()))
> +  if (reg != NULL)
> +    {
> +      if (reggroup == all_reggroup)
>  	return 1;
> 
> -  if (reg != NULL
> -      && (reggroup == save_reggroup || reggroup == restore_reggroup))
> -    return reg->save_restore;
> +      if (reggroup == save_reggroup || reggroup == restore_reggroup)
> +	return reg->save_restore;
> +
> +      if (reg != NULL && !reg->group.empty ())
> +	return (reg->group == reggroup->name ());
> +    }
> 
>    return -1;
>  }
> --
> 2.25.1

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

[Luis Machado]

On 11/20/23 13:25, Ciaran Woodward wrote:
> Ping
> 
> Discussion of the V1 can be found here: https://patchwork.sourceware.org/project/gdb/patch/20230907170752.28639-1-ciaranwoodward@xmos.com/
> 
> From what I see, this change is complete & useful as-is, but opens the door for further useful work in future - to improve the tdesc XMLs.
> 

Options 1 and 3 from the v1 discussion look reasonable to me. For 3 it would be nice to establish some kind of
versioning info so gdb can handle the xml the right way, and enforce group information. Say, if group information
is missing for a "v2" of the XML format, warn/error out.

For 1, I think it would be nice to cleanup gdb's target descriptions and add group information explicitly, to
prevent this from happening. in the future.


>> -----Original Message-----
>> From: Ciaran Woodward
>> Sent: Wednesday, September 13, 2023 12:42 PM
>> To: gdb-patches@sourceware.org
>> Cc: luis.machado@arm.com; aburgess@redhat.com; Ciaran Woodward
>> <ciaranwoodward@xmos.com>
>> Subject: [PATCH v2] gdb/tdesc: Don't assign custom-group tdesc registers
>> to 'general'
>>
>> 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.
>> ---
>>  gdb/target-descriptions.c | 14 +++++++++-----
>>  1 file changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/gdb/target-descriptions.c b/gdb/target-descriptions.c
>> index cdedf88c793..6eb626b9c5a 100644
>> --- a/gdb/target-descriptions.c
>> +++ b/gdb/target-descriptions.c
>> @@ -954,13 +954,17 @@ 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 ()))
>> +  if (reg != NULL)
>> +    {
>> +      if (reggroup == all_reggroup)
>>  	return 1;
>>
>> -  if (reg != NULL
>> -      && (reggroup == save_reggroup || reggroup == restore_reggroup))
>> -    return reg->save_restore;
>> +      if (reggroup == save_reggroup || reggroup == restore_reggroup)
>> +	return reg->save_restore;
>> +
>> +      if (reg != NULL && !reg->group.empty ())
>> +	return (reg->group == reggroup->name ());
>> +    }
>>
>>    return -1;
>>  }
>> --
>> 2.25.1
> 

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

[Ciaran Woodward]

Hi, Thanks for your reply

> > Discussion of the V1 can be found here:
> https://patchwork.sourceware.org/project/gdb/patch/20230907170752.28639-1-
> ciaranwoodward@xmos.com/
> >
> 
> Options 1 and 3 from the v1 discussion look reasonable to me. For 3 it
> would be nice to establish some kind of
> versioning info so gdb can handle the xml the right way, and enforce group
> information. Say, if group information
> is missing for a "v2" of the XML format, warn/error out.

I'm leaning towards 1, but it isn't clear to me which of the listed registers are
"quirks of the previous output which we don't care about" and which are "important
groupings we want to maintain".

Going back to Andrew's example, if we look only at ARM's fpscr, which is defined:
  <reg name="fpscr" bitsize="32" type="int" group="float"/>

Both pre- and post- patch, it would show in:
  info registers all
  info registers float

Pre-patch, but not post-patch, it would show in:
  info registers
  info registers general

To me, the post-patch behaviour seems like an improvement, but I'm not close enough
with the architecture to know that for sure.

There are a bunch of similar situations in other architectures (identified in
an earlier email).

If you think I should maintain the pre-patch behaviour, I'm happy to go add the
fix to all the individual architectures.
(it also raises the question of whether we should allow multi-grouping for
custom groups too)

> For 1, I think it would be nice to cleanup gdb's target descriptions and
> add group information explicitly, to
> prevent this from happening. in the future.

Post-patch, the behaviour can be expressed as:

  If the group is explicitly stated in the target-description, add the register
  to that group. If the group is not stated in the target-description, add the
  register to a group based on its type.

It's the "If the group is not stated in the target-description" which is added
by this patch.

Which I think already makes sense - the rationale being that there is never a
reason to add a register to no group, so having a default is reasonable. Therefore
I don't think any change to the existing XMLs is necessary.

Pre-patch, even when you _did_ specify an explicit group, gdb would add it to
the group based on type anyway.

Please let me know if I'm misunderstanding the rationale for your request.