From: Luis Machado <luis.machado@linaro.org>
To: Christian Biesinger <cbiesinger@google.com>
Cc: gdb-patches <gdb-patches@sourceware.org>
Subject: Re: [PATCH] Explicitly put registers in the general group for ARM
Date: Wed, 04 Mar 2020 22:01:00 -0000 [thread overview]
Message-ID: <CAD2_up-jrytKJyT1xoYGM0WnjvYHXi6HWBarHBiRTWK3_t+_ow@mail.gmail.com> (raw)
In-Reply-To: <CAPTJ0XHBweyg=5m80-Kk+7kdH4EiCtjLhWfirfZBrzEt-r4AbA@mail.gmail.com>
On Wed, Mar 4, 2020, 18:03 Christian Biesinger <cbiesinger@google.com>
wrote:
> On Mon, Mar 2, 2020 at 1:07 PM Luis Machado <luis.machado@linaro.org>
> wrote:
> >
> > Hi,
> >
> > On 3/2/20 2:59 PM, Christian Biesinger via gdb-patches wrote:
> > > This seems like the right thing per the documentation:
> > >
> https://sourceware.org/gdb/current/onlinedocs/gdb/Target-Description-Format.html#Target-Description-Format
> > > "If no group is specified, GDB will not display the register in
> > > info registers."
> > > but should also help with this issue:
> > > https://sourceware.org/ml/gdb-patches/2020-02/msg01038.html]
> >
> > I don't have a problem with the patch, but i think this should be done
> > for all the targets that currently do not explicitly set the group name
> > and fall into the "general" category based on the previous heuristic. Of
> > course, this is if we want to honor the existing documentation.
>
> Yeah, I agree. But I don't have the bandwidth to do this all targets,
> it is a good amount of manual work.
>
Just to be clear, i'm not expecting you to fix all the cases. I could do it
as well, but i want to make sure we agree on a reasonable way forward,
given the existing behavior, so we don't have to redo the work later. Or do
work that won't be useful.
I'll raise a thread about this and we can share some thoughts then.
> > There are also other target description producers out there that will
> > keep generating wrong group information, and there is little we can do
> > about it. So it makes me wonder if it is worth honoring the
> > documentation or if we should just update the documentation with the
> > existing behavior, since we'll have to keep it for a while.
>
> Yeah, but I still think it is good to be explicit about which
> registers are in the general group? If y'all disagree, I can drop this
> patch.
>
> > Users can always define some arbitrary group name that will not get
> > displayed by "info registers" anyway, right?
>
> That is true.
>
> > I'm thinking the offending patch will still need to be reverted or fixed
> > up to bring back the old behavior.
> >
> > Shahab was working on an alternative patch, but it hasn't been submitted
> > yet.
>
> Yeah, I think this is independent of that.
>
> Christian
>
> > > Note that "set tdesc filename features/arm/arm-core.xml; maint print
> c-tdesc"
> > > prints an empty target description both before and after this change,
> > > so I just manually edited the .c file to specify the group.
> > >
> > > gdb/ChangeLog:
> > >
> > > 2020-03-02 Christian Biesinger <cbiesinger@google.com>
> > >
> > > * features/aarch64-core.c (create_feature_aarch64_core): Put all
> > > registers in the "general" group.
> > > * features/aarch64-core.xml: Likewise.
> > > * features/arm/arm-core.c (create_feature_arm_arm_core):
> Likewise.
> > > * features/arm/arm-core.xml: Likewise.
> > > ---
> > > gdb/features/aarch64-core.c | 68
> +++++++++++++++++------------------
> > > gdb/features/aarch64-core.xml | 68
> +++++++++++++++++------------------
> > > gdb/features/arm/arm-core.c | 34 +++++++++---------
> > > gdb/features/arm/arm-core.xml | 34 +++++++++---------
> > > 4 files changed, 102 insertions(+), 102 deletions(-)
> > >
> > > diff --git a/gdb/features/aarch64-core.c b/gdb/features/aarch64-core.c
> > > index a080a641c4..f929b2cbf9 100644
> > > --- a/gdb/features/aarch64-core.c
> > > +++ b/gdb/features/aarch64-core.c
> > > @@ -30,39 +30,39 @@ create_feature_aarch64_core (struct target_desc
> *result, long regnum)
> > > tdesc_add_flag (type_with_fields, 30, "Z");
> > > tdesc_add_flag (type_with_fields, 31, "N");
> > >
> > > - tdesc_create_reg (feature, "x0", regnum++, 1, NULL, 64, "int");
> > > - tdesc_create_reg (feature, "x1", regnum++, 1, NULL, 64, "int");
> > > - tdesc_create_reg (feature, "x2", regnum++, 1, NULL, 64, "int");
> > > - tdesc_create_reg (feature, "x3", regnum++, 1, NULL, 64, "int");
> > > - tdesc_create_reg (feature, "x4", regnum++, 1, NULL, 64, "int");
> > > - tdesc_create_reg (feature, "x5", regnum++, 1, NULL, 64, "int");
> > > - tdesc_create_reg (feature, "x6", regnum++, 1, NULL, 64, "int");
> > > - tdesc_create_reg (feature, "x7", regnum++, 1, NULL, 64, "int");
> > > - tdesc_create_reg (feature, "x8", regnum++, 1, NULL, 64, "int");
> > > - tdesc_create_reg (feature, "x9", regnum++, 1, NULL, 64, "int");
> > > - tdesc_create_reg (feature, "x10", regnum++, 1, NULL, 64, "int");
> > > - tdesc_create_reg (feature, "x11", regnum++, 1, NULL, 64, "int");
> > > - tdesc_create_reg (feature, "x12", regnum++, 1, NULL, 64, "int");
> > > - tdesc_create_reg (feature, "x13", regnum++, 1, NULL, 64, "int");
> > > - tdesc_create_reg (feature, "x14", regnum++, 1, NULL, 64, "int");
> > > - tdesc_create_reg (feature, "x15", regnum++, 1, NULL, 64, "int");
> > > - tdesc_create_reg (feature, "x16", regnum++, 1, NULL, 64, "int");
> > > - tdesc_create_reg (feature, "x17", regnum++, 1, NULL, 64, "int");
> > > - tdesc_create_reg (feature, "x18", regnum++, 1, NULL, 64, "int");
> > > - tdesc_create_reg (feature, "x19", regnum++, 1, NULL, 64, "int");
> > > - tdesc_create_reg (feature, "x20", regnum++, 1, NULL, 64, "int");
> > > - tdesc_create_reg (feature, "x21", regnum++, 1, NULL, 64, "int");
> > > - tdesc_create_reg (feature, "x22", regnum++, 1, NULL, 64, "int");
> > > - tdesc_create_reg (feature, "x23", regnum++, 1, NULL, 64, "int");
> > > - tdesc_create_reg (feature, "x24", regnum++, 1, NULL, 64, "int");
> > > - tdesc_create_reg (feature, "x25", regnum++, 1, NULL, 64, "int");
> > > - tdesc_create_reg (feature, "x26", regnum++, 1, NULL, 64, "int");
> > > - tdesc_create_reg (feature, "x27", regnum++, 1, NULL, 64, "int");
> > > - tdesc_create_reg (feature, "x28", regnum++, 1, NULL, 64, "int");
> > > - tdesc_create_reg (feature, "x29", regnum++, 1, NULL, 64, "int");
> > > - tdesc_create_reg (feature, "x30", regnum++, 1, NULL, 64, "int");
> > > - tdesc_create_reg (feature, "sp", regnum++, 1, NULL, 64, "data_ptr");
> > > - tdesc_create_reg (feature, "pc", regnum++, 1, NULL, 64, "code_ptr");
> > > - tdesc_create_reg (feature, "cpsr", regnum++, 1, NULL, 32,
> "cpsr_flags");
> > > + tdesc_create_reg (feature, "x0", regnum++, 1, "general", 64, "int");
> > > + tdesc_create_reg (feature, "x1", regnum++, 1, "general", 64, "int");
> > > + tdesc_create_reg (feature, "x2", regnum++, 1, "general", 64, "int");
> > > + tdesc_create_reg (feature, "x3", regnum++, 1, "general", 64, "int");
> > > + tdesc_create_reg (feature, "x4", regnum++, 1, "general", 64, "int");
> > > + tdesc_create_reg (feature, "x5", regnum++, 1, "general", 64, "int");
> > > + tdesc_create_reg (feature, "x6", regnum++, 1, "general", 64, "int");
> > > + tdesc_create_reg (feature, "x7", regnum++, 1, "general", 64, "int");
> > > + tdesc_create_reg (feature, "x8", regnum++, 1, "general", 64, "int");
> > > + tdesc_create_reg (feature, "x9", regnum++, 1, "general", 64, "int");
> > > + tdesc_create_reg (feature, "x10", regnum++, 1, "general", 64,
> "int");
> > > + tdesc_create_reg (feature, "x11", regnum++, 1, "general", 64,
> "int");
> > > + tdesc_create_reg (feature, "x12", regnum++, 1, "general", 64,
> "int");
> > > + tdesc_create_reg (feature, "x13", regnum++, 1, "general", 64,
> "int");
> > > + tdesc_create_reg (feature, "x14", regnum++, 1, "general", 64,
> "int");
> > > + tdesc_create_reg (feature, "x15", regnum++, 1, "general", 64,
> "int");
> > > + tdesc_create_reg (feature, "x16", regnum++, 1, "general", 64,
> "int");
> > > + tdesc_create_reg (feature, "x17", regnum++, 1, "general", 64,
> "int");
> > > + tdesc_create_reg (feature, "x18", regnum++, 1, "general", 64,
> "int");
> > > + tdesc_create_reg (feature, "x19", regnum++, 1, "general", 64,
> "int");
> > > + tdesc_create_reg (feature, "x20", regnum++, 1, "general", 64,
> "int");
> > > + tdesc_create_reg (feature, "x21", regnum++, 1, "general", 64,
> "int");
> > > + tdesc_create_reg (feature, "x22", regnum++, 1, "general", 64,
> "int");
> > > + tdesc_create_reg (feature, "x23", regnum++, 1, "general", 64,
> "int");
> > > + tdesc_create_reg (feature, "x24", regnum++, 1, "general", 64,
> "int");
> > > + tdesc_create_reg (feature, "x25", regnum++, 1, "general", 64,
> "int");
> > > + tdesc_create_reg (feature, "x26", regnum++, 1, "general", 64,
> "int");
> > > + tdesc_create_reg (feature, "x27", regnum++, 1, "general", 64,
> "int");
> > > + tdesc_create_reg (feature, "x28", regnum++, 1, "general", 64,
> "int");
> > > + tdesc_create_reg (feature, "x29", regnum++, 1, "general", 64,
> "int");
> > > + tdesc_create_reg (feature, "x30", regnum++, 1, "general", 64,
> "int");
> > > + tdesc_create_reg (feature, "sp", regnum++, 1, "general", 64,
> "data_ptr");
> > > + tdesc_create_reg (feature, "pc", regnum++, 1, "general", 64,
> "code_ptr");
> > > + tdesc_create_reg (feature, "cpsr", regnum++, 1, "general", 32,
> "cpsr_flags");
> > > return regnum;
> > > }
> > > diff --git a/gdb/features/aarch64-core.xml
> b/gdb/features/aarch64-core.xml
> > > index ee6a3a6dfa..2f66088c6d 100644
> > > --- a/gdb/features/aarch64-core.xml
> > > +++ b/gdb/features/aarch64-core.xml
> > > @@ -8,40 +8,40 @@
> > >
> > > <!DOCTYPE feature SYSTEM "gdb-target.dtd">
> > > <feature name="org.gnu.gdb.aarch64.core">
> > > - <reg name="x0" bitsize="64"/>
> > > - <reg name="x1" bitsize="64"/>
> > > - <reg name="x2" bitsize="64"/>
> > > - <reg name="x3" bitsize="64"/>
> > > - <reg name="x4" bitsize="64"/>
> > > - <reg name="x5" bitsize="64"/>
> > > - <reg name="x6" bitsize="64"/>
> > > - <reg name="x7" bitsize="64"/>
> > > - <reg name="x8" bitsize="64"/>
> > > - <reg name="x9" bitsize="64"/>
> > > - <reg name="x10" bitsize="64"/>
> > > - <reg name="x11" bitsize="64"/>
> > > - <reg name="x12" bitsize="64"/>
> > > - <reg name="x13" bitsize="64"/>
> > > - <reg name="x14" bitsize="64"/>
> > > - <reg name="x15" bitsize="64"/>
> > > - <reg name="x16" bitsize="64"/>
> > > - <reg name="x17" bitsize="64"/>
> > > - <reg name="x18" bitsize="64"/>
> > > - <reg name="x19" bitsize="64"/>
> > > - <reg name="x20" bitsize="64"/>
> > > - <reg name="x21" bitsize="64"/>
> > > - <reg name="x22" bitsize="64"/>
> > > - <reg name="x23" bitsize="64"/>
> > > - <reg name="x24" bitsize="64"/>
> > > - <reg name="x25" bitsize="64"/>
> > > - <reg name="x26" bitsize="64"/>
> > > - <reg name="x27" bitsize="64"/>
> > > - <reg name="x28" bitsize="64"/>
> > > - <reg name="x29" bitsize="64"/>
> > > - <reg name="x30" bitsize="64"/>
> > > - <reg name="sp" bitsize="64" type="data_ptr"/>
> > > + <reg name="x0" bitsize="64" group="general"/>
> > > + <reg name="x1" bitsize="64" group="general"/>
> > > + <reg name="x2" bitsize="64" group="general"/>
> > > + <reg name="x3" bitsize="64" group="general"/>
> > > + <reg name="x4" bitsize="64" group="general"/>
> > > + <reg name="x5" bitsize="64" group="general"/>
> > > + <reg name="x6" bitsize="64" group="general"/>
> > > + <reg name="x7" bitsize="64" group="general"/>
> > > + <reg name="x8" bitsize="64" group="general"/>
> > > + <reg name="x9" bitsize="64" group="general"/>
> > > + <reg name="x10" bitsize="64" group="general"/>
> > > + <reg name="x11" bitsize="64" group="general"/>
> > > + <reg name="x12" bitsize="64" group="general"/>
> > > + <reg name="x13" bitsize="64" group="general"/>
> > > + <reg name="x14" bitsize="64" group="general"/>
> > > + <reg name="x15" bitsize="64" group="general"/>
> > > + <reg name="x16" bitsize="64" group="general"/>
> > > + <reg name="x17" bitsize="64" group="general"/>
> > > + <reg name="x18" bitsize="64" group="general"/>
> > > + <reg name="x19" bitsize="64" group="general"/>
> > > + <reg name="x20" bitsize="64" group="general"/>
> > > + <reg name="x21" bitsize="64" group="general"/>
> > > + <reg name="x22" bitsize="64" group="general"/>
> > > + <reg name="x23" bitsize="64" group="general"/>
> > > + <reg name="x24" bitsize="64" group="general"/>
> > > + <reg name="x25" bitsize="64" group="general"/>
> > > + <reg name="x26" bitsize="64" group="general"/>
> > > + <reg name="x27" bitsize="64" group="general"/>
> > > + <reg name="x28" bitsize="64" group="general"/>
> > > + <reg name="x29" bitsize="64" group="general"/>
> > > + <reg name="x30" bitsize="64" group="general"/>
> > > + <reg name="sp" bitsize="64" type="data_ptr" group="general"/>
> > >
> > > - <reg name="pc" bitsize="64" type="code_ptr"/>
> > > + <reg name="pc" bitsize="64" type="code_ptr" group="general"/>
> > >
> > > <flags id="cpsr_flags" size="4">
> > > <!-- Stack Pointer. -->
> > > @@ -86,6 +86,6 @@
> > > <!-- Negative Condition flag. -->
> > > <field name="N" start="31" end="31"/>
> > > </flags>
> > > - <reg name="cpsr" bitsize="32" type="cpsr_flags"/>
> > > + <reg name="cpsr" bitsize="32" type="cpsr_flags" group="general"/>
> > >
> > > </feature>
> > > diff --git a/gdb/features/arm/arm-core.c b/gdb/features/arm/arm-core.c
> > > index e401411fc5..4519752975 100644
> > > --- a/gdb/features/arm/arm-core.c
> > > +++ b/gdb/features/arm/arm-core.c
> > > @@ -9,23 +9,23 @@ create_feature_arm_arm_core (struct target_desc
> *result, long regnum)
> > > struct tdesc_feature *feature;
> > >
> > > feature = tdesc_create_feature (result, "org.gnu.gdb.arm.core");
> > > - tdesc_create_reg (feature, "r0", regnum++, 1, NULL, 32, "uint32");
> > > - tdesc_create_reg (feature, "r1", regnum++, 1, NULL, 32, "uint32");
> > > - tdesc_create_reg (feature, "r2", regnum++, 1, NULL, 32, "uint32");
> > > - tdesc_create_reg (feature, "r3", regnum++, 1, NULL, 32, "uint32");
> > > - tdesc_create_reg (feature, "r4", regnum++, 1, NULL, 32, "uint32");
> > > - tdesc_create_reg (feature, "r5", regnum++, 1, NULL, 32, "uint32");
> > > - tdesc_create_reg (feature, "r6", regnum++, 1, NULL, 32, "uint32");
> > > - tdesc_create_reg (feature, "r7", regnum++, 1, NULL, 32, "uint32");
> > > - tdesc_create_reg (feature, "r8", regnum++, 1, NULL, 32, "uint32");
> > > - tdesc_create_reg (feature, "r9", regnum++, 1, NULL, 32, "uint32");
> > > - tdesc_create_reg (feature, "r10", regnum++, 1, NULL, 32, "uint32");
> > > - tdesc_create_reg (feature, "r11", regnum++, 1, NULL, 32, "uint32");
> > > - tdesc_create_reg (feature, "r12", regnum++, 1, NULL, 32, "uint32");
> > > - tdesc_create_reg (feature, "sp", regnum++, 1, NULL, 32, "data_ptr");
> > > - tdesc_create_reg (feature, "lr", regnum++, 1, NULL, 32, "int");
> > > - tdesc_create_reg (feature, "pc", regnum++, 1, NULL, 32, "code_ptr");
> > > + tdesc_create_reg (feature, "r0", regnum++, 1, "general", 32,
> "uint32");
> > > + tdesc_create_reg (feature, "r1", regnum++, 1, "general", 32,
> "uint32");
> > > + tdesc_create_reg (feature, "r2", regnum++, 1, "general", 32,
> "uint32");
> > > + tdesc_create_reg (feature, "r3", regnum++, 1, "general", 32,
> "uint32");
> > > + tdesc_create_reg (feature, "r4", regnum++, 1, "general", 32,
> "uint32");
> > > + tdesc_create_reg (feature, "r5", regnum++, 1, "general", 32,
> "uint32");
> > > + tdesc_create_reg (feature, "r6", regnum++, 1, "general", 32,
> "uint32");
> > > + tdesc_create_reg (feature, "r7", regnum++, 1, "general", 32,
> "uint32");
> > > + tdesc_create_reg (feature, "r8", regnum++, 1, "general", 32,
> "uint32");
> > > + tdesc_create_reg (feature, "r9", regnum++, 1, "general", 32,
> "uint32");
> > > + tdesc_create_reg (feature, "r10", regnum++, 1, "general", 32,
> "uint32");
> > > + tdesc_create_reg (feature, "r11", regnum++, 1, "general", 32,
> "uint32");
> > > + tdesc_create_reg (feature, "r12", regnum++, 1, "general", 32,
> "uint32");
> > > + tdesc_create_reg (feature, "sp", regnum++, 1, "general", 32,
> "data_ptr");
> > > + tdesc_create_reg (feature, "lr", regnum++, 1, "general", 32, "int");
> > > + tdesc_create_reg (feature, "pc", regnum++, 1, "general", 32,
> "code_ptr");
> > > regnum = 25;
> > > - tdesc_create_reg (feature, "cpsr", regnum++, 1, NULL, 32, "int");
> > > + tdesc_create_reg (feature, "cpsr", regnum++, 1, "general", 32,
> "int");
> > > return regnum;
> > > }
> > > diff --git a/gdb/features/arm/arm-core.xml
> b/gdb/features/arm/arm-core.xml
> > > index 144fc4638a..d506004fe1 100644
> > > --- a/gdb/features/arm/arm-core.xml
> > > +++ b/gdb/features/arm/arm-core.xml
> > > @@ -7,25 +7,25 @@
> > >
> > > <!DOCTYPE feature SYSTEM "gdb-target.dtd">
> > > <feature name="org.gnu.gdb.arm.core">
> > > - <reg name="r0" bitsize="32" type="uint32"/>
> > > - <reg name="r1" bitsize="32" type="uint32"/>
> > > - <reg name="r2" bitsize="32" type="uint32"/>
> > > - <reg name="r3" bitsize="32" type="uint32"/>
> > > - <reg name="r4" bitsize="32" type="uint32"/>
> > > - <reg name="r5" bitsize="32" type="uint32"/>
> > > - <reg name="r6" bitsize="32" type="uint32"/>
> > > - <reg name="r7" bitsize="32" type="uint32"/>
> > > - <reg name="r8" bitsize="32" type="uint32"/>
> > > - <reg name="r9" bitsize="32" type="uint32"/>
> > > - <reg name="r10" bitsize="32" type="uint32"/>
> > > - <reg name="r11" bitsize="32" type="uint32"/>
> > > - <reg name="r12" bitsize="32" type="uint32"/>
> > > - <reg name="sp" bitsize="32" type="data_ptr"/>
> > > - <reg name="lr" bitsize="32"/>
> > > - <reg name="pc" bitsize="32" type="code_ptr"/>
> > > + <reg name="r0" bitsize="32" type="uint32" group="general"/>
> > > + <reg name="r1" bitsize="32" type="uint32" group="general"/>
> > > + <reg name="r2" bitsize="32" type="uint32" group="general"/>
> > > + <reg name="r3" bitsize="32" type="uint32" group="general"/>
> > > + <reg name="r4" bitsize="32" type="uint32" group="general"/>
> > > + <reg name="r5" bitsize="32" type="uint32" group="general"/>
> > > + <reg name="r6" bitsize="32" type="uint32" group="general"/>
> > > + <reg name="r7" bitsize="32" type="uint32" group="general"/>
> > > + <reg name="r8" bitsize="32" type="uint32" group="general"/>
> > > + <reg name="r9" bitsize="32" type="uint32" group="general"/>
> > > + <reg name="r10" bitsize="32" type="uint32" group="general"/>
> > > + <reg name="r11" bitsize="32" type="uint32" group="general"/>
> > > + <reg name="r12" bitsize="32" type="uint32" group="general"/>
> > > + <reg name="sp" bitsize="32" type="data_ptr" group="general"/>
> > > + <reg name="lr" bitsize="32" group="general"/>
> > > + <reg name="pc" bitsize="32" type="code_ptr"/ group="general">
> > >
> > > <!-- The CPSR is register 25, rather than register 16, because
> > > the FPA registers historically were placed between the PC
> > > and the CPSR in the "g" packet. -->
> > > - <reg name="cpsr" bitsize="32" regnum="25"/>
> > > + <reg name="cpsr" bitsize="32" regnum="25" group="general"/>
> > > </feature>
> > >
>
prev parent reply other threads:[~2020-03-04 22:01 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-02 17:59 Christian Biesinger via gdb-patches
2020-03-02 19:07 ` Luis Machado
2020-03-04 21:05 ` Christian Biesinger via gdb-patches
2020-03-04 22:01 ` Luis Machado [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAD2_up-jrytKJyT1xoYGM0WnjvYHXi6HWBarHBiRTWK3_t+_ow@mail.gmail.com \
--to=luis.machado@linaro.org \
--cc=cbiesinger@google.com \
--cc=gdb-patches@sourceware.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).