Message ID | 20200120155315.30333-1-shahab.vahedi@gmail.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 113365 invoked by alias); 20 Jan 2020 15:53:51 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 113345 invoked by uid 89); 20 Jan 2020 15:53:50 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, FREEMAIL_FROM, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.1 spammy=continued, H*m:gmail, HContent-Transfer-Encoding:8bit X-HELO: mail-lj1-f195.google.com Received: from mail-lj1-f195.google.com (HELO mail-lj1-f195.google.com) (209.85.208.195) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 20 Jan 2020 15:53:39 +0000 Received: by mail-lj1-f195.google.com with SMTP id h23so34320337ljc.8 for <gdb-patches@sourceware.org>; Mon, 20 Jan 2020 07:53:39 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:date:message-id:mime-version :content-transfer-encoding; bh=ATgE3vkQIKPLAPMC1t38fbwHffzw5fYm5Ro8lwCFN1Y=; b=qE6V4fOU3os+2BdBQnGCz7ljPVgyH2cfNftprpjBoe92sJNRn4CF0VbtlOzLZD8Sxu a2wyD4bfCxhR/azz7ulXWs01rcGVfUwZ3Wp83I6w45lFvV7l2MQnD4q3AcqCh0Lymtm9 jCqcc5ycGi4/r1FI3eF67CzKeieaW27JnCzuOGIEEFUTdPVzgMolySLzkrGcD0U9/9EH wlpdD6kQdJS69GvbX9Bj5VovrKgd3HlHGNdH5uNmV7dlY+WyCoWEV1l31RWIoTD5Bw3f EISWRIslhkV3Yquz/5quhzTrQenhZoSSTpO7kOPMt8VSOSUGfgIaokjffIlRhCXIr2g+ NGuQ== Return-Path: <shahab.vahedi@gmail.com> Received: from archie.internal.synopsys.com ([2a03:1b20:6:f011::2d]) by smtp.gmail.com with ESMTPSA id f11sm4230600lfm.12.2020.01.20.07.53.35 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 20 Jan 2020 07:53:36 -0800 (PST) From: Shahab Vahedi <shahab.vahedi@gmail.com> To: gdb-patches@sourceware.org Cc: Shahab Vahedi <shahab@synopsys.com>, Claudiu Zissulescu <claziss@synopsys.com>, Francois Bedard <fbedard@synopsys.com> Subject: [PATCH] Do not print empty-group regs when printing general ones Date: Mon, 20 Jan 2020 16:53:15 +0100 Message-Id: <20200120155315.30333-1-shahab.vahedi@gmail.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit |
Commit Message
Shahab Vahedi
Jan. 20, 2020, 3:53 p.m. UTC
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(-)
Comments
* 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 >
This patch was reviewed once (as OK): https://sourceware.org/ml/gdb-patches/2020-01/msg00613.html Could someone review/merge it? Cheers, Shahab
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.
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
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.
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
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.
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
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
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
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?
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
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.
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; }