[PATCH] Do not print empty-group regs when printing general ones

[PATCH] Do not print empty-group regs when printing general ones

[Shahab Vahedi]

From: Shahab Vahedi <shahab@synopsys.com>

When the command "info registers" (same as "info registers general"),
is issued, _all_ the registers from a tdesc XML are printed. This
includes the registers with empty register groups (set as "") which
are supposed to be only printed by "info registers all" (or "info
all-registers").

This bug got introduced after all the overhauls that the
tdesc_register_in_reggroup_p() went through. You can see that the
logic of tdesc_register_in_reggroup_p() did NOT remain the same after
all those changes:

  git difftool c9c895b9666..HEAD -- gdb/target-descriptions.c

With the current implementation, when the reg->group is an empty
string, this function returns -1, while in the working revision
(c9c895b9666), it returned 0. This patch makes sure that the 0 is
returned again.

The old implementation of tdesc_register_in_reggroup_p() returned
-1 when "reggroup" was set to "all_reggroups" at line 4 below:

1  tdesc_register_reggroup_p (...)
2  {
3   ...
4   ret = tdesc_register_in_reggroup_p (gdbarch, regno, reggroup);
5   if (ret != -1)
6     return ret;
7
8   return default_register_reggroup_p (gdbarch, regno, reggroup);
9  }

As a result, the execution continued at line 8 and the
default_register_reggroup_p(..., reggroup=all_reggroups) would
return 1. However, with the current implementation of
tdesc_register_in_reggroup_p() that allows checking against any
arbitrary group name, it returns 0 when comparing the "reg->group"
against the string "all" which is the group name for "all_reggroups".
I have added a special check to cover this case and
"info all-registers" works as expected.

gdb/ChangeLog:
2020-01-20  Shahab Vahedi  <shahab@synopsys.com>

	* target-descriptions.c (tdesc_register_in_reggroup_p):
	Return 0 when reg->group is empty and reggroup is not.
---
 gdb/target-descriptions.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/gdb/target-descriptions.c b/gdb/target-descriptions.c
index 04711ba2fa5..937210cf25e 100644
--- a/gdb/target-descriptions.c
+++ b/gdb/target-descriptions.c
@@ -977,13 +977,15 @@ 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 (reggroup)))
+  if (reg != NULL)
+    {
+      if (reggroup == all_reggroup)
 	return 1;
-
-  if (reg != NULL
-      && (reggroup == save_reggroup || reggroup == restore_reggroup))
-    return reg->save_restore;
+      else if (reggroup == save_reggroup || reggroup == restore_reggroup)
+	return reg->save_restore;
+      else
+	return (int) (reg->group == reggroup_name (reggroup));
+    }
 
   return -1;
 }
-- 
2.25.0

Re: [PATCH] Do not print empty-group regs when printing general ones

[Andrew Burgess]

* Shahab Vahedi <shahab.vahedi@gmail.com> [2020-01-20 16:53:15 +0100]:

> From: Shahab Vahedi <shahab@synopsys.com>
> 
> When the command "info registers" (same as "info registers general"),
> is issued, _all_ the registers from a tdesc XML are printed. This
> includes the registers with empty register groups (set as "") which
> are supposed to be only printed by "info registers all" (or "info
> all-registers").
> 
> This bug got introduced after all the overhauls that the
> tdesc_register_in_reggroup_p() went through. You can see that the
> logic of tdesc_register_in_reggroup_p() did NOT remain the same after
> all those changes:
> 
>   git difftool c9c895b9666..HEAD -- gdb/target-descriptions.c
> 
> With the current implementation, when the reg->group is an empty
> string, this function returns -1, while in the working revision
> (c9c895b9666), it returned 0. This patch makes sure that the 0 is
> returned again.
> 
> The old implementation of tdesc_register_in_reggroup_p() returned
> -1 when "reggroup" was set to "all_reggroups" at line 4 below:
> 
> 1  tdesc_register_reggroup_p (...)
> 2  {
> 3   ...
> 4   ret = tdesc_register_in_reggroup_p (gdbarch, regno, reggroup);
> 5   if (ret != -1)
> 6     return ret;
> 7
> 8   return default_register_reggroup_p (gdbarch, regno, reggroup);
> 9  }
> 
> As a result, the execution continued at line 8 and the
> default_register_reggroup_p(..., reggroup=all_reggroups) would
> return 1. However, with the current implementation of
> tdesc_register_in_reggroup_p() that allows checking against any
> arbitrary group name, it returns 0 when comparing the "reg->group"
> against the string "all" which is the group name for "all_reggroups".
> I have added a special check to cover this case and
> "info all-registers" works as expected.
> 
> gdb/ChangeLog:
> 2020-01-20  Shahab Vahedi  <shahab@synopsys.com>
> 
> 	* target-descriptions.c (tdesc_register_in_reggroup_p):
> 	Return 0 when reg->group is empty and reggroup is not.

Looks good to me.

Thanks,
Andrew



> ---
>  gdb/target-descriptions.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/gdb/target-descriptions.c b/gdb/target-descriptions.c
> index 04711ba2fa5..937210cf25e 100644
> --- a/gdb/target-descriptions.c
> +++ b/gdb/target-descriptions.c
> @@ -977,13 +977,15 @@ 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 (reggroup)))
> +  if (reg != NULL)
> +    {
> +      if (reggroup == all_reggroup)
>  	return 1;
> -
> -  if (reg != NULL
> -      && (reggroup == save_reggroup || reggroup == restore_reggroup))
> -    return reg->save_restore;
> +      else if (reggroup == save_reggroup || reggroup == restore_reggroup)
> +	return reg->save_restore;
> +      else
> +	return (int) (reg->group == reggroup_name (reggroup));
> +    }
>  
>    return -1;
>  }
> -- 
> 2.25.0
> 

[PING] [PATCH] Do not print empty-group regs when printing general ones

[Shahab Vahedi]

This patch was reviewed once (as OK):
https://sourceware.org/ml/gdb-patches/2020-01/msg00613.html

Could someone review/merge it?


Cheers,
Shahab

[Regression] [PATCH] Do not print empty-group regs when printing general ones

[Luis Machado]

On 1/31/20 7:34 AM, Shahab Vahedi wrote:
> This patch was reviewed once (as OK):
> https://sourceware.org/ml/gdb-patches/2020-01/msg00613.html
> 
> Could someone review/merge it?
> 
> 
> Cheers,
> Shahab
> 

FTR, this has broken general register printing for ARM/AArch64. Now 
"info reg" shows nothing.

Given there are already remote stubs, probes and gdbservers running out 
there, this is an undesirable change to have.

I had an IRC chat with Christian and he pointed me at some documentation 
stating empty-group registers should not be printed, but i think this is 
a case where the implementation has diverged from the documentation.

https://sourceware.org/gdb/current/onlinedocs/gdb/Target-Description-Format.html#Target-Description-Format

We could probably patch up any non-standard target description XML's 
from now on, but the existing behavior may have to be preserved.

I haven't investigated this in depth yet to determine what can/should 
change.

Re: [Regression] [PATCH] Do not print empty-group regs when printing general ones

[Christian Biesinger via gdb-patches]

On Fri, Feb 28, 2020 at 7:08 AM Luis Machado <luis.machado@linaro.org> wrote:
>
> On 1/31/20 7:34 AM, Shahab Vahedi wrote:
> > This patch was reviewed once (as OK):
> > https://sourceware.org/ml/gdb-patches/2020-01/msg00613.html
> >
> > Could someone review/merge it?
> >
> >
> > Cheers,
> > Shahab
> >
>
> FTR, this has broken general register printing for ARM/AArch64. Now
> "info reg" shows nothing.
>
> Given there are already remote stubs, probes and gdbservers running out
> there, this is an undesirable change to have.
>
> I had an IRC chat with Christian and he pointed me at some documentation
> stating empty-group registers should not be printed, but i think this is
> a case where the implementation has diverged from the documentation.
>
> https://sourceware.org/gdb/current/onlinedocs/gdb/Target-Description-Format.html#Target-Description-Format
>
> We could probably patch up any non-standard target description XML's
> from now on, but the existing behavior may have to be preserved.

Most targets under features/ do not specify group="general" in their
XML files for anything (only S/390 does); it seems like that should
maybe be fixed either way?

Christian

Re: [Regression] [PATCH] Do not print empty-group regs when printing general ones

[Luis Machado]

On 2/28/20 10:22 AM, Christian Biesinger wrote:
> On Fri, Feb 28, 2020 at 7:08 AM Luis Machado <luis.machado@linaro.org> wrote:
>>
>> On 1/31/20 7:34 AM, Shahab Vahedi wrote:
>>> This patch was reviewed once (as OK):
>>> https://sourceware.org/ml/gdb-patches/2020-01/msg00613.html
>>>
>>> Could someone review/merge it?
>>>
>>>
>>> Cheers,
>>> Shahab
>>>
>>
>> FTR, this has broken general register printing for ARM/AArch64. Now
>> "info reg" shows nothing.
>>
>> Given there are already remote stubs, probes and gdbservers running out
>> there, this is an undesirable change to have.
>>
>> I had an IRC chat with Christian and he pointed me at some documentation
>> stating empty-group registers should not be printed, but i think this is
>> a case where the implementation has diverged from the documentation.
>>
>> https://sourceware.org/gdb/current/onlinedocs/gdb/Target-Description-Format.html#Target-Description-Format
>>
>> We could probably patch up any non-standard target description XML's
>> from now on, but the existing behavior may have to be preserved.
> 
> Most targets under features/ do not specify group="general" in their
> XML files for anything (only S/390 does); it seems like that should
> maybe be fixed either way?

I agree.

Re: [Regression] [PATCH] Do not print empty-group regs when printing general ones

[Shahab Vahedi]

On Fri, Feb 28, 2020 at 10:31:26AM -0300, Luis Machado wrote:
> 
> 
> On 2/28/20 10:22 AM, Christian Biesinger wrote:
> > On Fri, Feb 28, 2020 at 7:08 AM Luis Machado <luis.machado@linaro.org> wrote:
> > > 
> > > On 1/31/20 7:34 AM, Shahab Vahedi wrote:
> > > > This patch was reviewed once (as OK):
> > > > https://sourceware.org/ml/gdb-patches/2020-01/msg00613.html
> > > > 
> > > > Could someone review/merge it?
> > > > 
> > > > 
> > > > Cheers,
> > > > Shahab
> > > > 
> > > 
> > > FTR, this has broken general register printing for ARM/AArch64. Now
> > > "info reg" shows nothing.
> > > 
> > > Given there are already remote stubs, probes and gdbservers running out
> > > there, this is an undesirable change to have.
> > > 
> > > I had an IRC chat with Christian and he pointed me at some documentation
> > > stating empty-group registers should not be printed, but i think this is
> > > a case where the implementation has diverged from the documentation.
> > > 
> > > https://sourceware.org/gdb/current/onlinedocs/gdb/Target-Description-Format.html#Target-Description-Format
> > > 
> > > We could probably patch up any non-standard target description XML's
> > > from now on, but the existing behavior may have to be preserved.
> > 
> > Most targets under features/ do not specify group="general" in their
> > XML files for anything (only S/390 does); it seems like that should
> > maybe be fixed either way?
> 
> I agree.

The documentation [1] says:
If no group is specified, GDB will not display the register in info registers

[1]
https://sourceware.org/gdb/onlinedocs/gdb/Target-Description-Format.html#Target-Description-Format

Re: [Regression] [PATCH] Do not print empty-group regs when printing general ones

[Luis Machado]

On 2/28/20 10:36 AM, Shahab Vahedi wrote:
> On Fri, Feb 28, 2020 at 10:31:26AM -0300, Luis Machado wrote:
>>
>>
>> On 2/28/20 10:22 AM, Christian Biesinger wrote:
>>> On Fri, Feb 28, 2020 at 7:08 AM Luis Machado <luis.machado@linaro.org> wrote:
>>>>
>>>> On 1/31/20 7:34 AM, Shahab Vahedi wrote:
>>>>> This patch was reviewed once (as OK):
>>>>> https://sourceware.org/ml/gdb-patches/2020-01/msg00613.html
>>>>>
>>>>> Could someone review/merge it?
>>>>>
>>>>>
>>>>> Cheers,
>>>>> Shahab
>>>>>
>>>>
>>>> FTR, this has broken general register printing for ARM/AArch64. Now
>>>> "info reg" shows nothing.
>>>>
>>>> Given there are already remote stubs, probes and gdbservers running out
>>>> there, this is an undesirable change to have.
>>>>
>>>> I had an IRC chat with Christian and he pointed me at some documentation
>>>> stating empty-group registers should not be printed, but i think this is
>>>> a case where the implementation has diverged from the documentation.
>>>>
>>>> https://sourceware.org/gdb/current/onlinedocs/gdb/Target-Description-Format.html#Target-Description-Format
>>>>
>>>> We could probably patch up any non-standard target description XML's
>>>> from now on, but the existing behavior may have to be preserved.
>>>
>>> Most targets under features/ do not specify group="general" in their
>>> XML files for anything (only S/390 does); it seems like that should
>>> maybe be fixed either way?
>>
>> I agree.
> 
> The documentation [1] says:
> If no group is specified, GDB will not display the register in info registers
> 
> [1]
> https://sourceware.org/gdb/onlinedocs/gdb/Target-Description-Format.html#Target-Description-Format
> 

That's valid, but unfortunately it doesn't change the fact the existing 
code is breaking backwards compatibility with the installed base.

As we discussed on IRC, this is code put together in early 2007 and 
hasn't been touched since, apart from a small change in 2017 to cope 
with arbitrary group strings. Plus we have plenty of existing target 
descriptions that do not honor explicitly setting a register group.

With that said, i think the documentation would have a lower priority in 
this regard. We should fix the existing target descriptions to be more 
strict with the group names, but the old behavior would have to be 
honored IMO.

Re: [Regression] [PATCH] Do not print empty-group regs when printing general ones

[Shahab Vahedi]

On Fri, Feb 28, 2020 at 10:50:54AM -0300, Luis Machado wrote:
> On 2/28/20 10:36 AM, Shahab Vahedi wrote:
> > On Fri, Feb 28, 2020 at 10:31:26AM -0300, Luis Machado wrote:
> > > 
> > > 
> > > On 2/28/20 10:22 AM, Christian Biesinger wrote:
> > > > On Fri, Feb 28, 2020 at 7:08 AM Luis Machado <luis.machado@linaro.org> wrote:
> > > > > 
> > > > > On 1/31/20 7:34 AM, Shahab Vahedi wrote:
> > > > > > This patch was reviewed once (as OK):
> > > > > > https://sourceware.org/ml/gdb-patches/2020-01/msg00613.html
> > > > > > 
> > > > > > Could someone review/merge it?
> > > > > > 
> > > > > > 
> > > > > > Cheers,
> > > > > > Shahab
> > > > > > 
> > > > > 
> > > > > FTR, this has broken general register printing for ARM/AArch64. Now
> > > > > "info reg" shows nothing.
> > > > > 
> > > > > Given there are already remote stubs, probes and gdbservers running out
> > > > > there, this is an undesirable change to have.
> > > > > 
> > > > > I had an IRC chat with Christian and he pointed me at some documentation
> > > > > stating empty-group registers should not be printed, but i think this is
> > > > > a case where the implementation has diverged from the documentation.
> > > > > 
> > > > > https://sourceware.org/gdb/current/onlinedocs/gdb/Target-Description-Format.html#Target-Description-Format
> > > > > 
> > > > > We could probably patch up any non-standard target description XML's
> > > > > from now on, but the existing behavior may have to be preserved.
> > > > 
> > > > Most targets under features/ do not specify group="general" in their
> > > > XML files for anything (only S/390 does); it seems like that should
> > > > maybe be fixed either way?
> > > 
> > > I agree.
> > 
> > The documentation [1] says:
> > If no group is specified, GDB will not display the register in info registers
> > 
> > [1]
> > https://sourceware.org/gdb/onlinedocs/gdb/Target-Description-Format.html#Target-Description-Format
> > 
> 
> That's valid, but unfortunately it doesn't change the fact the existing code
> is breaking backwards compatibility with the installed base.
> 
> As we discussed on IRC, this is code put together in early 2007 and hasn't
> been touched since, apart from a small change in 2017 to cope with arbitrary
> group strings. Plus we have plenty of existing target descriptions that do
> not honor explicitly setting a register group.
> 
> With that said, i think the documentation would have a lower priority in
> this regard. We should fix the existing target descriptions to be more
> strict with the group names, but the old behavior would have to be honored
> IMO.

I agree. The point in the patch was to make extra registers go away. However,
it apparently eliminated the output of "info registers" for other targets and
that is not OK. No matter sticking to the documentation or not. Feel free to
revert the patch.

Ideally I'd like a solution that:

1) "info registers": must not print the non-default (non-general) registers
   as it was the case with c9c895b9666

2) "info registers": should only print "general" group registers. This requires
   adding 'group="general"' to every XML features out there. So I don't know
   how realistic it is.


Cheers,
Shahab

Re: [Regression] [PATCH] Do not print empty-group regs when printing general ones

[Christian Biesinger via gdb-patches]

On Fri, Feb 28, 2020 at 8:08 AM Shahab Vahedi <shahab.vahedi@gmail.com> wrote:
>
> On Fri, Feb 28, 2020 at 10:50:54AM -0300, Luis Machado wrote:
> > On 2/28/20 10:36 AM, Shahab Vahedi wrote:
> > > On Fri, Feb 28, 2020 at 10:31:26AM -0300, Luis Machado wrote:
> > > >
> > > >
> > > > On 2/28/20 10:22 AM, Christian Biesinger wrote:
> > > > > On Fri, Feb 28, 2020 at 7:08 AM Luis Machado <luis.machado@linaro.org> wrote:
> > > > > >
> > > > > > On 1/31/20 7:34 AM, Shahab Vahedi wrote:
> > > > > > > This patch was reviewed once (as OK):
> > > > > > > https://sourceware.org/ml/gdb-patches/2020-01/msg00613.html
> > > > > > >
> > > > > > > Could someone review/merge it?
> > > > > > >
> > > > > > >
> > > > > > > Cheers,
> > > > > > > Shahab
> > > > > > >
> > > > > >
> > > > > > FTR, this has broken general register printing for ARM/AArch64. Now
> > > > > > "info reg" shows nothing.
> > > > > >
> > > > > > Given there are already remote stubs, probes and gdbservers running out
> > > > > > there, this is an undesirable change to have.
> > > > > >
> > > > > > I had an IRC chat with Christian and he pointed me at some documentation
> > > > > > stating empty-group registers should not be printed, but i think this is
> > > > > > a case where the implementation has diverged from the documentation.
> > > > > >
> > > > > > https://sourceware.org/gdb/current/onlinedocs/gdb/Target-Description-Format.html#Target-Description-Format
> > > > > >
> > > > > > We could probably patch up any non-standard target description XML's
> > > > > > from now on, but the existing behavior may have to be preserved.
> > > > >
> > > > > Most targets under features/ do not specify group="general" in their
> > > > > XML files for anything (only S/390 does); it seems like that should
> > > > > maybe be fixed either way?
> > > >
> > > > I agree.
> > >
> > > The documentation [1] says:
> > > If no group is specified, GDB will not display the register in info registers
> > >
> > > [1]
> > > https://sourceware.org/gdb/onlinedocs/gdb/Target-Description-Format.html#Target-Description-Format
> > >
> >
> > That's valid, but unfortunately it doesn't change the fact the existing code
> > is breaking backwards compatibility with the installed base.
> >
> > As we discussed on IRC, this is code put together in early 2007 and hasn't
> > been touched since, apart from a small change in 2017 to cope with arbitrary
> > group strings. Plus we have plenty of existing target descriptions that do
> > not honor explicitly setting a register group.
> >
> > With that said, i think the documentation would have a lower priority in
> > this regard. We should fix the existing target descriptions to be more
> > strict with the group names, but the old behavior would have to be honored
> > IMO.
>
> I agree. The point in the patch was to make extra registers go away. However,
> it apparently eliminated the output of "info registers" for other targets and
> that is not OK. No matter sticking to the documentation or not. Feel free to
> revert the patch.
>
> Ideally I'd like a solution that:
>
> 1) "info registers": must not print the non-default (non-general) registers
>    as it was the case with c9c895b9666
>
> 2) "info registers": should only print "general" group registers. This requires
>    adding 'group="general"' to every XML features out there. So I don't know
>    how realistic it is.

Maybe we can do something like: if there is no register with
group=general, print all registers w/o group. Otherwise, only print
registers with group=general.

Christian

Re: [Regression] [PATCH] Do not print empty-group regs when printing general ones

[Shahab Vahedi]

On Fri, Feb 28, 2020 at 08:10:35AM -0600, Christian Biesinger wrote:
> On Fri, Feb 28, 2020 at 8:08 AM Shahab Vahedi <shahab.vahedi@gmail.com> wrote:
> >
> > On Fri, Feb 28, 2020 at 10:50:54AM -0300, Luis Machado wrote:
> > > On 2/28/20 10:36 AM, Shahab Vahedi wrote:
> > > > On Fri, Feb 28, 2020 at 10:31:26AM -0300, Luis Machado wrote:
> > > > >
> > > > >
> > > > > On 2/28/20 10:22 AM, Christian Biesinger wrote:
> > > > > > On Fri, Feb 28, 2020 at 7:08 AM Luis Machado <luis.machado@linaro.org> wrote:
> > > > > > >
> > > > > > > On 1/31/20 7:34 AM, Shahab Vahedi wrote:
> > > > > > > > This patch was reviewed once (as OK):
> > > > > > > > https://sourceware.org/ml/gdb-patches/2020-01/msg00613.html
> > > > > > > >
> > > > > > > > Could someone review/merge it?
> > > > > > > >
> > > > > > > >
> > > > > > > > Cheers,
> > > > > > > > Shahab
> > > > > > > >
> > > > > > >
> > > > > > > FTR, this has broken general register printing for ARM/AArch64. Now
> > > > > > > "info reg" shows nothing.
> > > > > > >
> > > > > > > Given there are already remote stubs, probes and gdbservers running out
> > > > > > > there, this is an undesirable change to have.
> > > > > > >
> > > > > > > I had an IRC chat with Christian and he pointed me at some documentation
> > > > > > > stating empty-group registers should not be printed, but i think this is
> > > > > > > a case where the implementation has diverged from the documentation.
> > > > > > >
> > > > > > > https://sourceware.org/gdb/current/onlinedocs/gdb/Target-Description-Format.html#Target-Description-Format
> > > > > > >
> > > > > > > We could probably patch up any non-standard target description XML's
> > > > > > > from now on, but the existing behavior may have to be preserved.
> > > > > >
> > > > > > Most targets under features/ do not specify group="general" in their
> > > > > > XML files for anything (only S/390 does); it seems like that should
> > > > > > maybe be fixed either way?
> > > > >
> > > > > I agree.
> > > >
> > > > The documentation [1] says:
> > > > If no group is specified, GDB will not display the register in info registers
> > > >
> > > > [1]
> > > > https://sourceware.org/gdb/onlinedocs/gdb/Target-Description-Format.html#Target-Description-Format
> > > >
> > >
> > > That's valid, but unfortunately it doesn't change the fact the existing code
> > > is breaking backwards compatibility with the installed base.
> > >
> > > As we discussed on IRC, this is code put together in early 2007 and hasn't
> > > been touched since, apart from a small change in 2017 to cope with arbitrary
> > > group strings. Plus we have plenty of existing target descriptions that do
> > > not honor explicitly setting a register group.
> > >
> > > With that said, i think the documentation would have a lower priority in
> > > this regard. We should fix the existing target descriptions to be more
> > > strict with the group names, but the old behavior would have to be honored
> > > IMO.
> >
> > I agree. The point in the patch was to make extra registers go away. However,
> > it apparently eliminated the output of "info registers" for other targets and
> > that is not OK. No matter sticking to the documentation or not. Feel free to
> > revert the patch.
> >
> > Ideally I'd like a solution that:
> >
> > 1) "info registers": must not print the non-default (non-general) registers
> >    as it was the case with c9c895b9666
> >
> > 2) "info registers": should only print "general" group registers. This requires
> >    adding 'group="general"' to every XML features out there. So I don't know
> >    how realistic it is.
> 
> Maybe we can do something like: if there is no register with
> group=general, print all registers w/o group. Otherwise, only print
> registers with group=general.
> 
> Christian

I like that. We must update the documentation to something like:

* no gorup=... mentioned is the same as group="general" --> default
* group="" explicitly means that the register does not belong to any group.

And make the code act accordingly.


Cheers,
Shahab

Re: [Regression] [PATCH] Do not print empty-group regs when printing general ones

[Luis Machado]

On 2/28/20 11:15 AM, Shahab Vahedi wrote:
> On Fri, Feb 28, 2020 at 08:10:35AM -0600, Christian Biesinger wrote:
>> On Fri, Feb 28, 2020 at 8:08 AM Shahab Vahedi <shahab.vahedi@gmail.com> wrote:
>>>
>>> On Fri, Feb 28, 2020 at 10:50:54AM -0300, Luis Machado wrote:
>>>> On 2/28/20 10:36 AM, Shahab Vahedi wrote:
>>>>> On Fri, Feb 28, 2020 at 10:31:26AM -0300, Luis Machado wrote:
>>>>>>
>>>>>>
>>>>>> On 2/28/20 10:22 AM, Christian Biesinger wrote:
>>>>>>> On Fri, Feb 28, 2020 at 7:08 AM Luis Machado <luis.machado@linaro.org> wrote:
>>>>>>>>
>>>>>>>> On 1/31/20 7:34 AM, Shahab Vahedi wrote:
>>>>>>>>> This patch was reviewed once (as OK):
>>>>>>>>> https://sourceware.org/ml/gdb-patches/2020-01/msg00613.html
>>>>>>>>>
>>>>>>>>> Could someone review/merge it?
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Cheers,
>>>>>>>>> Shahab
>>>>>>>>>
>>>>>>>>
>>>>>>>> FTR, this has broken general register printing for ARM/AArch64. Now
>>>>>>>> "info reg" shows nothing.
>>>>>>>>
>>>>>>>> Given there are already remote stubs, probes and gdbservers running out
>>>>>>>> there, this is an undesirable change to have.
>>>>>>>>
>>>>>>>> I had an IRC chat with Christian and he pointed me at some documentation
>>>>>>>> stating empty-group registers should not be printed, but i think this is
>>>>>>>> a case where the implementation has diverged from the documentation.
>>>>>>>>
>>>>>>>> https://sourceware.org/gdb/current/onlinedocs/gdb/Target-Description-Format.html#Target-Description-Format
>>>>>>>>
>>>>>>>> We could probably patch up any non-standard target description XML's
>>>>>>>> from now on, but the existing behavior may have to be preserved.
>>>>>>>
>>>>>>> Most targets under features/ do not specify group="general" in their
>>>>>>> XML files for anything (only S/390 does); it seems like that should
>>>>>>> maybe be fixed either way?
>>>>>>
>>>>>> I agree.
>>>>>
>>>>> The documentation [1] says:
>>>>> If no group is specified, GDB will not display the register in info registers
>>>>>
>>>>> [1]
>>>>> https://sourceware.org/gdb/onlinedocs/gdb/Target-Description-Format.html#Target-Description-Format
>>>>>
>>>>
>>>> That's valid, but unfortunately it doesn't change the fact the existing code
>>>> is breaking backwards compatibility with the installed base.
>>>>
>>>> As we discussed on IRC, this is code put together in early 2007 and hasn't
>>>> been touched since, apart from a small change in 2017 to cope with arbitrary
>>>> group strings. Plus we have plenty of existing target descriptions that do
>>>> not honor explicitly setting a register group.
>>>>
>>>> With that said, i think the documentation would have a lower priority in
>>>> this regard. We should fix the existing target descriptions to be more
>>>> strict with the group names, but the old behavior would have to be honored
>>>> IMO.
>>>
>>> I agree. The point in the patch was to make extra registers go away. However,
>>> it apparently eliminated the output of "info registers" for other targets and
>>> that is not OK. No matter sticking to the documentation or not. Feel free to
>>> revert the patch.
>>>
>>> Ideally I'd like a solution that:
>>>
>>> 1) "info registers": must not print the non-default (non-general) registers
>>>     as it was the case with c9c895b9666
>>>
>>> 2) "info registers": should only print "general" group registers. This requires
>>>     adding 'group="general"' to every XML features out there. So I don't know
>>>     how realistic it is.
>>
>> Maybe we can do something like: if there is no register with
>> group=general, print all registers w/o group. Otherwise, only print
>> registers with group=general.
>>
>> Christian
> 
> I like that. We must update the documentation to something like:
> 
> * no gorup=... mentioned is the same as group="general" --> default
> * group="" explicitly means that the register does not belong to any group.
> 
> And make the code act accordingly.
> 
> 
> Cheers,
> Shahab
> 

Should we revert this for now then, and address this problem with an 
upcoming patch?

Re: [Regression] [PATCH] Do not print empty-group regs when printing general ones

[Shahab Vahedi]

Hi Luis,

On 3/4/20 2:16 PM, Luis Machado wrote:
> Should we revert this for now then, and address this problem with an upcoming patch?

Since this breaks the backward compatibility, I agree. Thank you for looking into this.


Cheers,
-- 
Shahab

Re: [Regression] [PATCH] Do not print empty-group regs when printing general ones

[Luis Machado]

Hi Shahab,

On 3/4/20 12:19 PM, Shahab Vahedi wrote:
> Hi Luis,
> 
> On 3/4/20 2:16 PM, Luis Machado wrote:
>> Should we revert this for now then, and address this problem with an upcoming patch?
> 
> Since this breaks the backward compatibility, I agree. Thank you for looking into this.
I've reverted this now, until we have a working fix.