[PATCH 0/3] Support for arbitrary reggroups

[PATCH 0/3] Support for arbitrary reggroups

[Stafford Horne]

Traditionally registers have been limited to names like "vector",
"general", "system" which are hard coded in the gdbarch.  This patch allows
additional reggroups to be defined by the xml target description.

This is necessary for architectures like OpenRISC which have many
registers.

Stafford Horne (3):
  reggroups: Add test and docs for `info reg $reggroup` feature
  reggroups: Convert reggroups from post_init to pre_init
  tdesc: handle arbitrary strings in tdesc_register_in_reggroup_p

 gdb/NEWS                             |  4 ++
 gdb/doc/gdb.texinfo                  |  5 +++
 gdb/reggroups.c                      | 15 ++------
 gdb/target-descriptions.c            | 73 ++++++++++++++++++------------------
 gdb/testsuite/gdb.base/reggroups.c   |  4 ++
 gdb/testsuite/gdb.base/reggroups.exp | 71 +++++++++++++++++++++++++++++++++++
 gdb/testsuite/gdb.xml/extra-regs.xml |  1 +
 gdb/testsuite/gdb.xml/tdesc-regs.exp |  3 ++
 8 files changed, 128 insertions(+), 48 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/reggroups.c
 create mode 100644 gdb/testsuite/gdb.base/reggroups.exp

-- 
2.9.4

[PATCH 1/3] reggroups: Add test and docs for `info reg $reggroup` feature

[Stafford Horne]

Until now this feature has existed but was not documented.  Adding docs
and tests.

gdb/ChangeLog:

2017-06-06  Stafford Horne  <shorne@gmail.com>

	* doc/gdb.texinfo: Document info reg $reggroup feature.

gdb/testsuite/ChangeLog:

2017-06-06  Stafford Horne  <shorne@gmail.com>

	* gdb.base/reggroups.c: New file.
	* gdb.base/reggroups.exp: New file.
---
 gdb/doc/gdb.texinfo                  |  5 +++
 gdb/testsuite/gdb.base/reggroups.c   |  4 ++
 gdb/testsuite/gdb.base/reggroups.exp | 71 ++++++++++++++++++++++++++++++++++++
 3 files changed, 80 insertions(+)
 create mode 100644 gdb/testsuite/gdb.base/reggroups.c
 create mode 100644 gdb/testsuite/gdb.base/reggroups.exp

diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 9fb70f6..a11db0c 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -10878,6 +10878,11 @@ and vector registers (in the selected stack frame).
 Print the names and values of all registers, including floating-point
 and vector registers (in the selected stack frame).
 
+@item info registers @var{reggroup} @dots{}
+Print the name and value of the restiers in each of the specified
+@var{reggroup}.  The @var{reggoup} can be any of those returned by
+@code{maint print reggroups}.
+
 @item info registers @var{regname} @dots{}
 Print the @dfn{relativized} value of each specified register @var{regname}.
 As discussed in detail below, register values are normally relative to
diff --git a/gdb/testsuite/gdb.base/reggroups.c b/gdb/testsuite/gdb.base/reggroups.c
new file mode 100644
index 0000000..f8b643a
--- /dev/null
+++ b/gdb/testsuite/gdb.base/reggroups.c
@@ -0,0 +1,4 @@
+int main()
+{
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/reggroups.exp b/gdb/testsuite/gdb.base/reggroups.exp
new file mode 100644
index 0000000..fd51c50
--- /dev/null
+++ b/gdb/testsuite/gdb.base/reggroups.exp
@@ -0,0 +1,71 @@
+# This testcase is part of GDB, the GNU debugger.
+
+# Copyright 2014-2017 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Test listing reggroups and the registers in each group.
+
+standard_testfile
+
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile debug]} {
+    return -1
+}
+
+if ![runto_main] then {
+    fail "can't run to main"
+    return 0
+}
+
+proc fetch_reggroups {test} {
+    global gdb_prompt
+    global expect_out
+
+    set reggroups {}
+    set bad -1
+    gdb_test_multiple "maint print reggroups" "get reggroups" {
+	-re "maint print reggroups\r\n" {
+	    exp_continue
+	}
+	-re "^ Group\[ \t\]+Type\[ \t\]+\r\n" {
+	    exp_continue
+	}
+	-re "^ (\[0-9a-zA-Z\-\]+)\[ \t\]+(user|internal)\[ \t\]+\r\n" {
+	    lappend reggroups $expect_out(1,string)
+	    exp_continue
+	}
+	-re ".*$gdb_prompt $" {
+	    if { [llength $reggroups] != 0 } {
+		incr bad
+	    } else {
+		set bad 1
+	    }
+	}
+    }
+
+    if {$bad} {
+	fail $test
+	return {}
+    }
+
+    pass $test
+    return $reggroups
+}
+
+set reggroups [fetch_reggroups "fetch reggroups"]
+set reggroup ""
+
+foreach reggroup [lrange $reggroups 0 end] {
+    gdb_test "info reg $reggroup" ".*" "info reg $reggroup"
+}
-- 
2.9.4

[PATCH 2/3] reggroups: Convert reggroups from post_init to pre_init

[Stafford Horne]

Currently the reggroups gdbarch_data cannot be manipulated until after
the gdbarch is completely initialized.  This is usually done when the
object init depends on architecture specific fields.  In the case of
reggroups it only depends on the obstack being available.

Coverting this to pre_init allows using reggroups during gdbarch
initialization.  This is needed to allow registering arbitrary reggroups
during gdbarch initializations.

gdb/ChangeLog:

2017-06-06  Stafford Horne  <shorne@gmail.com>

	* reggroups.c (reggroups_init): Change to depend only on
	obstack rather than gdbarch.
	(reggroup_add): Remove logic for forcing premature init.
	(_initialize_reggroup): Set `reggroups_data` with
	gdbarch_data_register_pre_init() rather than
	gdbarch_data_register_post_init().
---
 gdb/reggroups.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/gdb/reggroups.c b/gdb/reggroups.c
index ae7d4ce..bf40964 100644
--- a/gdb/reggroups.c
+++ b/gdb/reggroups.c
@@ -26,6 +26,7 @@
 #include "regcache.h"
 #include "command.h"
 #include "gdbcmd.h"		/* For maintenanceprintlist.  */
+#include "gdb_obstack.h"
 
 /* Individual register groups.  */
 
@@ -76,10 +77,9 @@ struct reggroups
 static struct gdbarch_data *reggroups_data;
 
 static void *
-reggroups_init (struct gdbarch *gdbarch)
+reggroups_init (struct obstack *obstack)
 {
-  struct reggroups *groups = GDBARCH_OBSTACK_ZALLOC (gdbarch,
-						     struct reggroups);
+  struct reggroups *groups = OBSTACK_ZALLOC (obstack, struct reggroups);
 
   groups->last = &groups->first;
   return groups;
@@ -105,13 +105,6 @@ reggroup_add (struct gdbarch *gdbarch, struct reggroup *group)
   struct reggroups *groups
     = (struct reggroups *) gdbarch_data (gdbarch, reggroups_data);
 
-  if (groups == NULL)
-    {
-      /* ULGH, called during architecture initialization.  Patch
-         things up.  */
-      groups = (struct reggroups *) reggroups_init (gdbarch);
-      deprecated_set_gdbarch_data (gdbarch, reggroups_data, groups);
-    }
   add_group (groups, group,
 	     GDBARCH_OBSTACK_ZALLOC (gdbarch, struct reggroup_el));
 }
@@ -300,7 +293,7 @@ extern initialize_file_ftype _initialize_reggroup; /* -Wmissing-prototypes */
 void
 _initialize_reggroup (void)
 {
-  reggroups_data = gdbarch_data_register_post_init (reggroups_init);
+  reggroups_data = gdbarch_data_register_pre_init (reggroups_init);
 
   /* The pre-defined list of groups.  */
   add_group (&default_groups, general_reggroup, XNEW (struct reggroup_el));
-- 
2.9.4

[PATCH 3/3] tdesc: handle arbitrary strings in tdesc_register_in_reggroup_p

[Stafford Horne]

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(-)

diff --git a/gdb/NEWS b/gdb/NEWS
index 112aa2f..0b18552 100644
--- 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.
+
 *** Changes in GDB 8.0
 
 * GDB now supports access to the PKU register on GNU/Linux. The register is
diff --git a/gdb/target-descriptions.c b/gdb/target-descriptions.c
index 9a7e2dd..015f6ba 100644
--- a/gdb/target-descriptions.c
+++ b/gdb/target-descriptions.c
@@ -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
diff --git a/gdb/testsuite/gdb.xml/extra-regs.xml b/gdb/testsuite/gdb.xml/extra-regs.xml
index 997d659..302e64c 100644
--- a/gdb/testsuite/gdb.xml/extra-regs.xml
+++ b/gdb/testsuite/gdb.xml/extra-regs.xml
@@ -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>
diff --git a/gdb/testsuite/gdb.xml/tdesc-regs.exp b/gdb/testsuite/gdb.xml/tdesc-regs.exp
index 70fc0e0..21f6fcc 100644
--- a/gdb/testsuite/gdb.xml/tdesc-regs.exp
+++ b/gdb/testsuite/gdb.xml/tdesc-regs.exp
@@ -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.
-- 
2.9.4

Re: [PATCH 1/3] reggroups: Add test and docs for `info reg $reggroup` feature

[Eli Zaretskii]

> From: Stafford Horne <shorne@gmail.com>
> Cc: Stafford Horne <shorne@gmail.com>
> Date: Thu,  8 Jun 2017 07:15:45 +0900
> 
> Until now this feature has existed but was not documented.  Adding docs
> and tests.
> 
> gdb/ChangeLog:
> 
> 2017-06-06  Stafford Horne  <shorne@gmail.com>
> 
> 	* doc/gdb.texinfo: Document info reg $reggroup feature.

OK for the documentation part, but please state in the ChangeLog entry
the name of the node(s) in which you make the change(s).

Re: [PATCH 3/3] tdesc: handle arbitrary strings in tdesc_register_in_reggroup_p

[Eli Zaretskii]

> 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.

Re: [PATCH 1/3] reggroups: Add test and docs for `info reg $reggroup` feature

[Stafford Horne]

On Thu, Jun 08, 2017 at 05:38:27AM +0300, Eli Zaretskii wrote:
> > From: Stafford Horne <shorne@gmail.com>
> > Cc: Stafford Horne <shorne@gmail.com>
> > Date: Thu,  8 Jun 2017 07:15:45 +0900
> > 
> > Until now this feature has existed but was not documented.  Adding docs
> > and tests.
> > 
> > gdb/ChangeLog:
> > 
> > 2017-06-06  Stafford Horne  <shorne@gmail.com>
> > 
> > 	* doc/gdb.texinfo: Document info reg $reggroup feature.
> 
> OK for the documentation part, but please state in the ChangeLog entry
> the name of the node(s) in which you make the change(s).

Thanks for the review,  I didnt know about this for doc changes.  I will
update.

Re: [PATCH 3/3] tdesc: handle arbitrary strings in tdesc_register_in_reggroup_p

[Stafford Horne]

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

Re: [PATCH 3/3] tdesc: handle arbitrary strings in tdesc_register_in_reggroup_p

[Eli Zaretskii]

> 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.

Re: [PATCH 1/3] reggroups: Add test and docs for `info reg $reggroup` feature

[Simon Marchi]

On 2017-06-08 00:15, Stafford Horne wrote:
> Until now this feature has existed but was not documented.  Adding docs
> and tests.

Hi Stafford,

Thanks for doing this!  It would be cool if you could improve the 
documentation printed by "help info registers", while at it.

> gdb/ChangeLog:
> 
> 2017-06-06  Stafford Horne  <shorne@gmail.com>
> 
> 	* doc/gdb.texinfo: Document info reg $reggroup feature.
> 
> gdb/testsuite/ChangeLog:
> 
> 2017-06-06  Stafford Horne  <shorne@gmail.com>
> 
> 	* gdb.base/reggroups.c: New file.
> 	* gdb.base/reggroups.exp: New file.
> ---
>  gdb/doc/gdb.texinfo                  |  5 +++
>  gdb/testsuite/gdb.base/reggroups.c   |  4 ++
>  gdb/testsuite/gdb.base/reggroups.exp | 71 
> ++++++++++++++++++++++++++++++++++++
>  3 files changed, 80 insertions(+)
>  create mode 100644 gdb/testsuite/gdb.base/reggroups.c
>  create mode 100644 gdb/testsuite/gdb.base/reggroups.exp
> 
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index 9fb70f6..a11db0c 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -10878,6 +10878,11 @@ and vector registers (in the selected stack 
> frame).
>  Print the names and values of all registers, including floating-point
>  and vector registers (in the selected stack frame).
> 
> +@item info registers @var{reggroup} @dots{}
> +Print the name and value of the restiers in each of the specified

Do you mean "registers"?

> +@var{reggroup}.  The @var{reggoup} can be any of those returned by
> +@code{maint print reggroups}.
> +
>  @item info registers @var{regname} @dots{}
>  Print the @dfn{relativized} value of each specified register 
> @var{regname}.
>  As discussed in detail below, register values are normally relative to
> diff --git a/gdb/testsuite/gdb.base/reggroups.c
> b/gdb/testsuite/gdb.base/reggroups.c
> new file mode 100644
> index 0000000..f8b643a
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/reggroups.c
> @@ -0,0 +1,4 @@
> +int main()

We try to use the GDB formatting style for the test code as well, so 
here it would be:

int
main ()
{
   return 0;
}

> +{
> +  return 0;
> +}
> diff --git a/gdb/testsuite/gdb.base/reggroups.exp
> b/gdb/testsuite/gdb.base/reggroups.exp
> new file mode 100644
> index 0000000..fd51c50
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/reggroups.exp
> @@ -0,0 +1,71 @@
> +# This testcase is part of GDB, the GNU debugger.
> +
> +# Copyright 2014-2017 Free Software Foundation, Inc.

Should it be 2017 only?

> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see 
> <http://www.gnu.org/licenses/>.
> +
> +# Test listing reggroups and the registers in each group.
> +
> +standard_testfile
> +
> +if {[prepare_for_testing "failed to prepare" $testfile $srcfile 
> debug]} {
> +    return -1
> +}
> +
> +if ![runto_main] then {
> +    fail "can't run to main"
> +    return 0
> +}
> +
> +proc fetch_reggroups {test} {
> +    global gdb_prompt
> +    global expect_out
> +
> +    set reggroups {}
> +    set bad -1
> +    gdb_test_multiple "maint print reggroups" "get reggroups" {
> +	-re "maint print reggroups\r\n" {
> +	    exp_continue
> +	}
> +	-re "^ Group\[ \t\]+Type\[ \t\]+\r\n" {
> +	    exp_continue
> +	}
> +	-re "^ (\[0-9a-zA-Z\-\]+)\[ \t\]+(user|internal)\[ \t\]+\r\n" {
> +	    lappend reggroups $expect_out(1,string)
> +	    exp_continue
> +	}
> +	-re ".*$gdb_prompt $" {
> +	    if { [llength $reggroups] != 0 } {
> +		incr bad
> +	    } else {
> +		set bad 1
> +	    }

I don't quite understand this.  Why not simply "set bad 1"?

Another option would be to call error to throw an exception, which will 
end the test abruptly:

   error "maint print reggroups: unexpected output"

You could also assert that there is at least one reggroup with something 
like:

   gdb_assert { [llength $reggroups] > 0 } "at least one reggroup"

> +	}
> +    }
> +
> +    if {$bad} {
> +	fail $test
> +	return {}
> +    }
> 
> +
> +    pass $test
> +    return $reggroups
> +}
> +
> +set reggroups [fetch_reggroups "fetch reggroups"]
> +set reggroup ""

You don't need to declare reggroup.

> +
> +foreach reggroup [lrange $reggroups 0 end] {

And this can be simply:

   foreach reggroup $reggroups

> +    gdb_test "info reg $reggroup" ".*" "info reg $reggroup"
> +}

Simon

Re: [PATCH 3/3] tdesc: handle arbitrary strings in tdesc_register_in_reggroup_p

[Simon Marchi]

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

Re: [PATCH 1/3] reggroups: Add test and docs for `info reg $reggroup` feature

[Stafford Horne]

On Thu, Jun 08, 2017 at 10:31:56PM +0200, Simon Marchi wrote:
> On 2017-06-08 00:15, Stafford Horne wrote:
> > Until now this feature has existed but was not documented.  Adding docs
> > and tests.
> 
> Hi Stafford,
> 
> Thanks for doing this!  It would be cool if you could improve the
> documentation printed by "help info registers", while at it.
> 
> > gdb/ChangeLog:
> > 
> > 2017-06-06  Stafford Horne  <shorne@gmail.com>
> > 
> > 	* doc/gdb.texinfo: Document info reg $reggroup feature.
> > 
> > gdb/testsuite/ChangeLog:
> > 
> > 2017-06-06  Stafford Horne  <shorne@gmail.com>
> > 
> > 	* gdb.base/reggroups.c: New file.
> > 	* gdb.base/reggroups.exp: New file.
> > ---
> >  gdb/doc/gdb.texinfo                  |  5 +++
> >  gdb/testsuite/gdb.base/reggroups.c   |  4 ++
> >  gdb/testsuite/gdb.base/reggroups.exp | 71
> > ++++++++++++++++++++++++++++++++++++
> >  3 files changed, 80 insertions(+)
> >  create mode 100644 gdb/testsuite/gdb.base/reggroups.c
> >  create mode 100644 gdb/testsuite/gdb.base/reggroups.exp
> > 
> > diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> > index 9fb70f6..a11db0c 100644
> > --- a/gdb/doc/gdb.texinfo
> > +++ b/gdb/doc/gdb.texinfo
> > @@ -10878,6 +10878,11 @@ and vector registers (in the selected stack
> > frame).
> >  Print the names and values of all registers, including floating-point
> >  and vector registers (in the selected stack frame).
> > 
> > +@item info registers @var{reggroup} @dots{}
> > +Print the name and value of the restiers in each of the specified
> 
> Do you mean "registers"?

Yes.  will fix

> > +@var{reggroup}.  The @var{reggoup} can be any of those returned by
> > +@code{maint print reggroups}.
> > +
> >  @item info registers @var{regname} @dots{}
> >  Print the @dfn{relativized} value of each specified register
> > @var{regname}.
> >  As discussed in detail below, register values are normally relative to
> > diff --git a/gdb/testsuite/gdb.base/reggroups.c
> > b/gdb/testsuite/gdb.base/reggroups.c
> > new file mode 100644
> > index 0000000..f8b643a
> > --- /dev/null
> > +++ b/gdb/testsuite/gdb.base/reggroups.c
> > @@ -0,0 +1,4 @@
> > +int main()
> 
> We try to use the GDB formatting style for the test code as well, so here it
> would be:
> 
> int
> main ()
> {
>   return 0;
> }

OK

> > +{
> > +  return 0;
> > +}
> > diff --git a/gdb/testsuite/gdb.base/reggroups.exp
> > b/gdb/testsuite/gdb.base/reggroups.exp
> > new file mode 100644
> > index 0000000..fd51c50
> > --- /dev/null
> > +++ b/gdb/testsuite/gdb.base/reggroups.exp
> > @@ -0,0 +1,71 @@
> > +# This testcase is part of GDB, the GNU debugger.
> > +
> > +# Copyright 2014-2017 Free Software Foundation, Inc.
> 
> Should it be 2017 only?

Yes. will fix

> > +
> > +# This program is free software; you can redistribute it and/or modify
> > +# it under the terms of the GNU General Public License as published by
> > +# the Free Software Foundation; either version 3 of the License, or
> > +# (at your option) any later version.
> > +#
> > +# This program is distributed in the hope that it will be useful,
> > +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > +# GNU General Public License for more details.
> > +#
> > +# You should have received a copy of the GNU General Public License
> > +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> > +
> > +# Test listing reggroups and the registers in each group.
> > +
> > +standard_testfile
> > +
> > +if {[prepare_for_testing "failed to prepare" $testfile $srcfile debug]}
> > {
> > +    return -1
> > +}
> > +
> > +if ![runto_main] then {
> > +    fail "can't run to main"
> > +    return 0
> > +}
> > +
> > +proc fetch_reggroups {test} {
> > +    global gdb_prompt
> > +    global expect_out
> > +
> > +    set reggroups {}
> > +    set bad -1
> > +    gdb_test_multiple "maint print reggroups" "get reggroups" {
> > +	-re "maint print reggroups\r\n" {
> > +	    exp_continue
> > +	}
> > +	-re "^ Group\[ \t\]+Type\[ \t\]+\r\n" {
> > +	    exp_continue
> > +	}
> > +	-re "^ (\[0-9a-zA-Z\-\]+)\[ \t\]+(user|internal)\[ \t\]+\r\n" {
> > +	    lappend reggroups $expect_out(1,string)
> > +	    exp_continue
> > +	}
> > +	-re ".*$gdb_prompt $" {
> > +	    if { [llength $reggroups] != 0 } {
> > +		incr bad
> > +	    } else {
> > +		set bad 1
> > +	    }
> 
> I don't quite understand this.  Why not simply "set bad 1"?
> 
> Another option would be to call error to throw an exception, which will end
> the test abruptly:
> 
>   error "maint print reggroups: unexpected output"

Alright, I was copying this pattern from other test, I did think it was a
bit strange but it makese a bit of sense if you want to capture the failure
condition but continue with other tests.

Ill change it as you suggest.

> You could also assert that there is at least one reggroup with something
> like:
> 
>   gdb_assert { [llength $reggroups] > 0 } "at least one reggroup"

OK, ill look at that.

> > +	}
> > +    }
> > +
> > +    if {$bad} {
> > +	fail $test
> > +	return {}
> > +    }
> > 
> > +
> > +    pass $test
> > +    return $reggroups
> > +}
> > +
> > +set reggroups [fetch_reggroups "fetch reggroups"]
> > +set reggroup ""
> 
> You don't need to declare reggroup.

OK

> > +
> > +foreach reggroup [lrange $reggroups 0 end] {
> 
> And this can be simply:
> 
>   foreach reggroup $reggroups

OK

> > +    gdb_test "info reg $reggroup" ".*" "info reg $reggroup"
> > +}

Thanks for reviewing, I dont write much expect code, this is helpful.

-Stafford

Re: [PATCH 1/3] reggroups: Add test and docs for `info reg $reggroup` feature

[Simon Marchi]

On 2017-06-09 01:27, Stafford Horne wrote:
> Thanks for reviewing, I dont write much expect code, this is helpful.

I understand, and share the pain :)

Simon

Re: [PATCH 3/3] tdesc: handle arbitrary strings in tdesc_register_in_reggroup_p

[Stafford Horne]

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

Re: [PATCH 3/3] tdesc: handle arbitrary strings in tdesc_register_in_reggroup_p

[Simon Marchi]

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

Re: [PATCH 3/3] tdesc: handle arbitrary strings in tdesc_register_in_reggroup_p

[Stafford Horne]

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

Re: [PATCH 1/3] reggroups: Add test and docs for `info reg $reggroup` feature

[Pedro Alves]

On 06/07/2017 11:15 PM, Stafford Horne wrote:
> +@item info registers @var{reggroup} @dots{}
> +Print the name and value of the restiers in each of the specified
> +@var{reggroup}.  The @var{reggoup} can be any of those returned by
> +@code{maint print reggroups}.

Not a blocker, but seems odd to be suggesting maintenance commands
to users.  Should we add a "info reggroups" or some such command?

Thanks,
Pedro Alves

Re: [PATCH 3/3] tdesc: handle arbitrary strings in tdesc_register_in_reggroup_p

[Pedro Alves]

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