[3/3] tdesc: handle arbitrary strings in tdesc_register_in_reggroup_p
Commit Message
tdesc_register_in_reggroup_p in now able to handle arbitrary
groups. This is useful when groups are created while the
target descriptor file is received from the remote.
This can be the case of a soft core target processor where
registers/groups can change.
gdb/ChangeLog:
2017-06-06 Franck Jullien <franck.jullien@gmail.com>
Stafford Horne <shorne@gmail.com>
* target-descriptions.c (tdesc_register_in_reggroup_p): Support
arbitrary strings.
* NEWS (Changes since GDB 8.0): Announce that GDB supports
arbitrary reggroups.
gdb/testsuite/ChangeLog:
2017-06-06 Stafford Horne <shorne@gmail.com>
* gdb.xml/extra-regs.xml: Add example foo reggroup.
* gdb.xml/tdesc-regs.exp: Add test to check for foo reggroup.
---
gdb/NEWS | 4 ++
gdb/target-descriptions.c | 73 ++++++++++++++++++------------------
gdb/testsuite/gdb.xml/extra-regs.xml | 1 +
gdb/testsuite/gdb.xml/tdesc-regs.exp | 3 ++
4 files changed, 44 insertions(+), 37 deletions(-)
Comments
> From: Stafford Horne <shorne@gmail.com>
> Cc: Stafford Horne <shorne@gmail.com>
> Date: Thu, 8 Jun 2017 07:15:47 +0900
>
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -3,6 +3,10 @@
>
> *** Changes since GDB 8.0
>
> +* GDB now supports dynamically creating arbitrary register groups specified
> + in xml target descriptors. This allows for finer grain grouping of
> + registers on systems with a large amount of registers.
> +
I'm confused: in the commentary for the patch to the manual you said
the feature existed, but wasn't documented. So why is it mentioned in
NEWS as a new feature? Should we have more documentation changes, if
it's new?
Thanks.
On Thu, Jun 08, 2017 at 05:40:34AM +0300, Eli Zaretskii wrote:
> > From: Stafford Horne <shorne@gmail.com>
> > Cc: Stafford Horne <shorne@gmail.com>
> > Date: Thu, 8 Jun 2017 07:15:47 +0900
> >
> > --- a/gdb/NEWS
> > +++ b/gdb/NEWS
> > @@ -3,6 +3,10 @@
> >
> > *** Changes since GDB 8.0
> >
> > +* GDB now supports dynamically creating arbitrary register groups specified
> > + in xml target descriptors. This allows for finer grain grouping of
> > + registers on systems with a large amount of registers.
> > +
>
> I'm confused: in the commentary for the patch to the manual you said
> the feature existed, but wasn't documented. So why is it mentioned in
> NEWS as a new feature? Should we have more documentation changes, if
> it's new?
Thanks for reviewing.
Sorry, you are right its a bit confusing and this second part probably
needs some more document changes.
There are 2 things going on in those 2 patches.
1. Add docs for the existing feature (listing registers by group name)
2. Add feature to be able to defined more groups based on the XML target
description.
Ill add docs for (2) and try to make it a bit more clear in my next cover
letter.
Is the single NEWS entry ok in the end?
-Stafford
> Date: Thu, 8 Jun 2017 14:01:51 +0900
> From: Stafford Horne <shorne@gmail.com>
> Cc: gdb-patches@sourceware.org
>
> There are 2 things going on in those 2 patches.
> 1. Add docs for the existing feature (listing registers by group name)
> 2. Add feature to be able to defined more groups based on the XML target
> description.
>
> Ill add docs for (2) and try to make it a bit more clear in my next cover
> letter.
Thanks.
> Is the single NEWS entry ok in the end?
Yes.
Hi Stafford,
On 2017-06-08 00:15, Stafford Horne wrote:
> @@ -1081,17 +1080,13 @@ tdesc_remote_register_number (struct gdbarch
> *gdbarch, int regno)
> }
>
> /* Check whether REGNUM is a member of REGGROUP. Registers from the
> - target description may be classified as general, float, or vector.
> - Unlike a gdbarch register_reggroup_p method, this function will
> - return -1 if it does not know; the caller should handle registers
> - with no specified group.
> -
> - Arbitrary strings (other than "general", "float", and "vector")
> - from the description are not used; they cause the register to be
> - displayed in "info all-registers" but excluded from "info
> - registers" et al. The names of containing features are also not
> - used. This might be extended to display registers in some more
> - useful groupings.
> + target description may be classified as general, float, vector or
> other
> + register groups registerd with reggroup_add(). Unlike a gdbarch
s/registerd/registered/
> @@ -1231,6 +1209,27 @@ tdesc_use_registers (struct gdbarch *gdbarch,
> void **slot = htab_find_slot (reg_hash, reg, INSERT);
>
> *slot = reg;
> + /* Add reggroup if its new. */
> + if (reg->group != NULL)
> + {
> + struct reggroup *group;
> + bool group_exists = false;
> +
> + for (group = reggroup_next (gdbarch, NULL);
> + group != NULL;
> + group = reggroup_next (gdbarch, group))
> + {
> + if (strcmp (reg->group, reggroup_name (group)) == 0)
> + {
> + group_exists = true;
> + break;
> + }
> + }
> +
> + if (!group_exists)
> + reggroup_add (gdbarch, reggroup_new (reg->group,
> + USER_REGGROUP));
Is the memory allocated by reggroup_new ever freed?
Simon
On Thu, Jun 08, 2017 at 10:52:01PM +0200, Simon Marchi wrote:
> Hi Stafford,
>
> On 2017-06-08 00:15, Stafford Horne wrote:
> > @@ -1081,17 +1080,13 @@ tdesc_remote_register_number (struct gdbarch
> > *gdbarch, int regno)
> > }
> >
> > /* Check whether REGNUM is a member of REGGROUP. Registers from the
> > - target description may be classified as general, float, or vector.
> > - Unlike a gdbarch register_reggroup_p method, this function will
> > - return -1 if it does not know; the caller should handle registers
> > - with no specified group.
> > -
> > - Arbitrary strings (other than "general", "float", and "vector")
> > - from the description are not used; they cause the register to be
> > - displayed in "info all-registers" but excluded from "info
> > - registers" et al. The names of containing features are also not
> > - used. This might be extended to display registers in some more
> > - useful groupings.
> > + target description may be classified as general, float, vector or
> > other
> > + register groups registerd with reggroup_add(). Unlike a gdbarch
>
> s/registerd/registered/
Doh, I seem to never be able to spell register.
> > @@ -1231,6 +1209,27 @@ tdesc_use_registers (struct gdbarch *gdbarch,
> > void **slot = htab_find_slot (reg_hash, reg, INSERT);
> >
> > *slot = reg;
> > + /* Add reggroup if its new. */
> > + if (reg->group != NULL)
> > + {
> > + struct reggroup *group;
> > + bool group_exists = false;
> > +
> > + for (group = reggroup_next (gdbarch, NULL);
> > + group != NULL;
> > + group = reggroup_next (gdbarch, group))
> > + {
> > + if (strcmp (reg->group, reggroup_name (group)) == 0)
> > + {
> > + group_exists = true;
> > + break;
> > + }
> > + }
> > +
> > + if (!group_exists)
> > + reggroup_add (gdbarch, reggroup_new (reg->group,
> > + USER_REGGROUP));
>
> Is the memory allocated by reggroup_new ever freed?
It is not, and its a bit tricky. I looked through gdb, it seems the
reggroup objects are never freed, anywhere. The list itself and list
elements (reggroup_el) are all allocated on obstack, but not reggroup.
It could get a bit messy to try to do something about it like refcounts or
tracking reggroups per target description.
Any suggestions?
-Stafford
On 2017-06-09 13:45, Stafford Horne wrote:
>> Is the memory allocated by reggroup_new ever freed?
>
> It is not, and its a bit tricky. I looked through gdb, it seems the
> reggroup objects are never freed, anywhere. The list itself and list
> elements (reggroup_el) are all allocated on obstack, but not reggroup.
>
> It could get a bit messy to try to do something about it like refcounts
> or
> tracking reggroups per target description.
>
> Any suggestions?
>
> -Stafford
What's tricky is that some reggroup objects are owned by tdep files and
are more or less permanent, whereas the groups provided by the tdesc are
more transient and owned by a gdbarch instance. A quick fix I think
would be to have another version of reggroup_new (or the same function
with additional parameters) that allocates the object on the gdbarch's
obstack instead of XNEW'ing it. The name would be obstack_strdup'ed
instead of xstrdup'ed. This should ensure that when the gdbarch is
freed, the reggroups it owns are freed as well.
I don't think reference counting is useful here, because there is always
a single entity to which the lifetime of the object is tied, so it
should be clear when to free it.
From what I can see, gdbarch's are never really freed right now, so it
doesn't really matter right now, but I think we should do it right
anyway, in case it ever changes.
Simon
On Fri, Jun 09, 2017 at 09:59:08PM +0200, Simon Marchi wrote:
> On 2017-06-09 13:45, Stafford Horne wrote:
> > > Is the memory allocated by reggroup_new ever freed?
> >
> > It is not, and its a bit tricky. I looked through gdb, it seems the
> > reggroup objects are never freed, anywhere. The list itself and list
> > elements (reggroup_el) are all allocated on obstack, but not reggroup.
> >
> > It could get a bit messy to try to do something about it like refcounts
> > or
> > tracking reggroups per target description.
> >
> > Any suggestions?
> >
> > -Stafford
>
> What's tricky is that some reggroup objects are owned by tdep files and are
> more or less permanent, whereas the groups provided by the tdesc are more
> transient and owned by a gdbarch instance. A quick fix I think would be to
> have another version of reggroup_new (or the same function with additional
> parameters) that allocates the object on the gdbarch's obstack instead of
> XNEW'ing it. The name would be obstack_strdup'ed instead of xstrdup'ed.
> This should ensure that when the gdbarch is freed, the reggroups it owns are
> freed as well.
>
> I don't think reference counting is useful here, because there is always a
> single entity to which the lifetime of the object is tied, so it should be
> clear when to free it.
>
> From what I can see, gdbarch's are never really freed right now, so it
> doesn't really matter right now, but I think we should do it right anyway,
> in case it ever changes.
Alright, creating a new reggroup_new() based on obstack is what I was
thinking as well. But I thought there might be an issue with users setting
and unsetting target descriptors. Ill revert back it I find any trouble.
-Stafford
On 06/07/2017 11:15 PM, Stafford Horne wrote:
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -3,6 +3,10 @@
>
> *** Changes since GDB 8.0
>
> +* GDB now supports dynamically creating arbitrary register groups specified
> + in xml target descriptors. This allows for finer grain grouping of
> + registers on systems with a large amount of registers.
s/xml/XML/
s/descriptors/descriptions/
Thanks,
Pedro Alves
@@ -3,6 +3,10 @@
*** Changes since GDB 8.0
+* GDB now supports dynamically creating arbitrary register groups specified
+ in xml target descriptors. This allows for finer grain grouping of
+ registers on systems with a large amount of registers.
+
*** Changes in GDB 8.0
* GDB now supports access to the PKU register on GNU/Linux. The register is
@@ -63,12 +63,11 @@ typedef struct tdesc_reg
int save_restore;
/* The name of the register group containing this register, or NULL
- if the group should be automatically determined from the
- register's type. If this is "general", "float", or "vector", the
- corresponding "info" command should display this register's
- value. It can be an arbitrary string, but should be limited to
- alphanumeric characters and internal hyphens. Currently other
- strings are ignored (treated as NULL). */
+ if the group should be automatically determined from the register's
+ type. This is traditionally "general", "float", "vector" but can
+ also be an arbitrary string. If defined the corresponding "info"
+ command should display this register's value. The string should be
+ limited to alphanumeric characters and internal hyphens. */
char *group;
/* The size of the register, in bits. */
@@ -1081,17 +1080,13 @@ tdesc_remote_register_number (struct gdbarch *gdbarch, int regno)
}
/* Check whether REGNUM is a member of REGGROUP. Registers from the
- target description may be classified as general, float, or vector.
- Unlike a gdbarch register_reggroup_p method, this function will
- return -1 if it does not know; the caller should handle registers
- with no specified group.
-
- Arbitrary strings (other than "general", "float", and "vector")
- from the description are not used; they cause the register to be
- displayed in "info all-registers" but excluded from "info
- registers" et al. The names of containing features are also not
- used. This might be extended to display registers in some more
- useful groupings.
+ target description may be classified as general, float, vector or other
+ register groups registerd with reggroup_add(). Unlike a gdbarch
+ register_reggroup_p method, this function will return -1 if it does not
+ know; the caller should handle registers with no specified group.
+
+ The names of containing features are not used. This might be extended
+ to display registers in some more useful groupings.
The save-restore flag is also implemented here. */
@@ -1101,26 +1096,9 @@ tdesc_register_in_reggroup_p (struct gdbarch *gdbarch, int regno,
{
struct tdesc_reg *reg = tdesc_find_register (gdbarch, regno);
- if (reg != NULL && reg->group != NULL)
- {
- int general_p = 0, float_p = 0, vector_p = 0;
-
- if (strcmp (reg->group, "general") == 0)
- general_p = 1;
- else if (strcmp (reg->group, "float") == 0)
- float_p = 1;
- else if (strcmp (reg->group, "vector") == 0)
- vector_p = 1;
-
- if (reggroup == float_reggroup)
- return float_p;
-
- if (reggroup == vector_reggroup)
- return vector_p;
-
- if (reggroup == general_reggroup)
- return general_p;
- }
+ if (reg != NULL && reg->group != NULL
+ && (strcmp (reg->group, reggroup_name (reggroup)) == 0))
+ return 1;
if (reg != NULL
&& (reggroup == save_reggroup || reggroup == restore_reggroup))
@@ -1231,6 +1209,27 @@ tdesc_use_registers (struct gdbarch *gdbarch,
void **slot = htab_find_slot (reg_hash, reg, INSERT);
*slot = reg;
+ /* Add reggroup if its new. */
+ if (reg->group != NULL)
+ {
+ struct reggroup *group;
+ bool group_exists = false;
+
+ for (group = reggroup_next (gdbarch, NULL);
+ group != NULL;
+ group = reggroup_next (gdbarch, group))
+ {
+ if (strcmp (reg->group, reggroup_name (group)) == 0)
+ {
+ group_exists = true;
+ break;
+ }
+ }
+
+ if (!group_exists)
+ reggroup_add (gdbarch, reggroup_new (reg->group,
+ USER_REGGROUP));
+ }
}
/* Remove any registers which were assigned numbers by the
@@ -53,5 +53,6 @@
<reg name="bitfields" bitsize="64" type="struct2"/>
<reg name="flags" bitsize="32" type="flags"/>
<reg name="mixed_flags" bitsize="32" type="mixed_flags"/>
+ <reg name="groupreg" bitsize="32" type="uint32" group="foo"/>
</feature>
</target>
@@ -187,6 +187,9 @@ gdb_test "ptype \$flags" \
"type = flag flags {\r\n *bool X @0;\r\n *uint32_t Y @2;\r\n}"
gdb_test "ptype \$mixed_flags" \
"type = flag mixed_flags {\r\n *bool A @0;\r\n *uint32_t B @1-3;\r\n *bool C @4;\r\n *uint32_t D @5;\r\n *uint32_t @6-7;\r\n *enum {yes = 1, no = 0, maybe = 2, so} Z @8-9;\r\n}"
+# Reggroups should have at least general and the extra foo group
+gdb_test "maintenance print reggroups" \
+ " Group\[ \t\]+Type\[ \t\]+\r\n.* general\[ \t\]+user\[ \t\]+\r\n.* foo\[ \t\]+user\[ \t\]+"
load_description "core-only.xml" "" "test-regs.xml"
# The extra register from the previous description should be gone.