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

[Luis Machado <luis.machado@linaro.org>]

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?