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

[PATCH v4 1/4] 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:

yyyy-mm-dd  Stafford Horne  <shorne@gmail.com>

	* infcmd.c (_initialize_infcmd): Add help for info reg $reggroup
	and info all-registers $reggroup feature.

gdb/doc/ChangeLog:

yyyy-mm-dd  Stafford Horne  <shorne@gmail.com>

	* gdb.texinfo (Registers): Document info reg $reggroup feature.

gdb/testsuite/ChangeLog:

yyyy-mm-dd  Stafford Horne  <shorne@gmail.com>

	* gdb.base/reggroups.c: New file.
	* gdb.base/reggroups.exp: New file.
---

Since v3
 - Fixed grammar issue in gdb.texinfo pointed out by Eli.
 - Added xref in gdb.texinfo pointed out by Eli.
 - Documented multi-reggroup support in infcmd.c suggested by Eli.
 - Enhanced testing to actual test info reg results suggested by Simon.

 gdb/doc/gdb.texinfo                  |   5 ++
 gdb/infcmd.c                         |   8 ++-
 gdb/testsuite/gdb.base/reggroups.c   |   5 ++
 gdb/testsuite/gdb.base/reggroups.exp | 112 +++++++++++++++++++++++++++++++++++
 4 files changed, 128 insertions(+), 2 deletions(-)
 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 60ed80c363..a16e79bc2a 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -11023,6 +11023,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 registers in each of the specified
+@var{reggroup}s.  The @var{reggoup} can be any of those returned by
+@code{maint print reggroups} (@pxref{Maintenance Commands}).
+
 @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/infcmd.c b/gdb/infcmd.c
index 8bde28eab6..1b63f9b730 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -3460,13 +3460,17 @@ interrupt all running threads in non-stop mode, use the -a option."));
 
   c = add_info ("registers", info_registers_command, _("\
 List of integer registers and their contents, for selected stack frame.\n\
-Register name as argument means describe only that register."));
+One or more register names as argument means describe the given registers.\n\
+One or more register group names as argument means describe the registers\n\
+in the named register groups."));
   add_info_alias ("r", "registers", 1);
   set_cmd_completer (c, reg_or_group_completer);
 
   c = add_info ("all-registers", info_all_registers_command, _("\
 List of all registers and their contents, for selected stack frame.\n\
-Register name as argument means describe only that register."));
+One or more register names as argument means describe the given registers.\n\
+One or more register group names as argument means describe the registers\n\
+in the named register groups."));
   set_cmd_completer (c, reg_or_group_completer);
 
   add_info ("program", info_program_command,
diff --git a/gdb/testsuite/gdb.base/reggroups.c b/gdb/testsuite/gdb.base/reggroups.c
new file mode 100644
index 0000000000..8e8f518aae
--- /dev/null
+++ b/gdb/testsuite/gdb.base/reggroups.c
@@ -0,0 +1,5 @@
+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 0000000000..4fc2c9992a
--- /dev/null
+++ b/gdb/testsuite/gdb.base/reggroups.exp
@@ -0,0 +1,112 @@
+# This testcase is part of GDB, the GNU debugger.
+
+# Copyright 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
+}
+
+# Fetch all reggroups from 'maint print reggroups'.
+
+proc fetch_reggroups {test} {
+    global gdb_prompt
+    global expect_out
+
+    set reggroups {}
+    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 } {
+		pass $test
+	    } else {
+		fail "$test - didn't fetch any reggroups"
+	    }
+	}
+    }
+
+    return $reggroups
+}
+
+# Fetch all registers for a reggroup from 'info reg <reggroup>'.
+
+proc fetch_reggroup_regs {reggroup test} {
+    global gdb_prompt
+    global expext_out
+
+    # The command info reg <reggroup> will return something like the following:
+    #
+    # r0             0x0      0^M
+    # r1             0x7fdffc 0x7fdffc^M
+    # r2             0x7fe000 0x7fe000^M
+    # npc            0x23a8   0x23a8 <main+12>^M
+    # sr             0x8401   [ SM CY FO CID=0 ]^M
+    #
+    # We parse out and return the reg names, this is done by detecting
+    # that for each line we have a register name followed by a $hex number.
+    set regs {}
+    gdb_test_multiple "info reg $reggroup" "info reg $reggroup" {
+	-re "info reg $reggroup\r\n" {
+	    exp_continue
+	}
+	-re "^(\[0-9a-zA-Z\-\]+)\[ \t\]+(0x\[0-9a-f\]+)\[ \t\]+(\[^\n\r\]+)\r\n" {
+	    lappend regs $expect_out(1,string)
+	    exp_continue
+	}
+	-re "Invalid register .*\r\n" {
+	    fail "$test - unexpected invalid register response"
+	}
+	-re "$gdb_prompt $" {
+	    pass $test
+	}
+    }
+    return $regs
+}
+
+set reggroups [fetch_reggroups "fetch reggroups"]
+set regcount 0
+foreach reggroup $reggroups {
+    set regs [fetch_reggroup_regs $reggroup "fetch reggroup regs $reggroup"]
+    set regcount [expr $regcount + [llength $regs]]
+}
+if { $regcount != 0 } {
+    pass "system has reggroup regs $regcount"
+} else {
+    fail "system has no reggroup regs at all"
+}
+
+# If this fails it means that probably someone changed the error text returned
+# for an invalid register argument.  If that happens we should fix the pattern
+# here and in the fetch_reggroup_regs procedure above.
+gdb_test "info reg invalid-reggroup" "Invalid register .*" \
+    "info reg invalid-reggroup should report 'Invalid register'"
-- 
2.13.6

[PATCH v4 3/4] reggroups: Add reggroup_gdbarch_new, reggroup_find for dynamic reggroups

[Stafford Horne]

Traditionally reggroups have been created via reggroup_new() during
initialization code and never freed.  Now, if we want to initialize
reggroups dynamically (i.e. in target description) we should be able to
free them.  Create this function reggroup_gdbarch_new() which will
allocate the reggroup memory onto the passed gdbarch obstack.

Also creating reggroup_find() as a utility to find a gdbarch registered
reggroup object by name.

gdb/ChangeLog:

yyyy-mm-dd  Stafford Horne  <shorne@gmail.com>

	* reggroups.c (reggroup_gdbarch_new): New function.
	(reggroup_find): New function.
	* reggroups.h (reggroup_gdbarch_new): New function.
	(reggroup_find): New function.
---
Changes since v3
 * Added the reggroup_find function.

 gdb/reggroups.c | 29 +++++++++++++++++++++++++++++
 gdb/reggroups.h |  5 +++++
 2 files changed, 34 insertions(+)

diff --git a/gdb/reggroups.c b/gdb/reggroups.c
index 5d5e33f2a3..4e6b452f88 100644
--- a/gdb/reggroups.c
+++ b/gdb/reggroups.c
@@ -46,6 +46,18 @@ reggroup_new (const char *name, enum reggroup_type type)
   return group;
 }
 
+struct reggroup *
+reggroup_gdbarch_new (struct gdbarch *gdbarch, const char *name,
+		      enum reggroup_type type)
+{
+  struct reggroup *group = GDBARCH_OBSTACK_ZALLOC (gdbarch,
+						   struct reggroup);
+
+  group->name = gdbarch_obstack_strdup (gdbarch, name);
+  group->type = type;
+  return group;
+}
+
 /* Register group attributes.  */
 
 const char *
@@ -201,6 +213,23 @@ default_register_reggroup_p (struct gdbarch *gdbarch, int regnum,
   return 0;   
 }
 
+/* Find a reggroup by name.  */
+
+reggroup *
+reggroup_find (struct gdbarch *gdbarch, const char *name)
+{
+  struct reggroup *group;
+
+  for (group = reggroup_next (gdbarch, NULL);
+       group != NULL;
+       group = reggroup_next (gdbarch, group))
+    {
+      if (strcmp (name, reggroup_name (group)) == 0)
+	return group;
+    }
+  return NULL;
+}
+
 /* Dump out a table of register groups for the current architecture.  */
 
 static void
diff --git a/gdb/reggroups.h b/gdb/reggroups.h
index 18fc1bf294..5cfa51abff 100644
--- a/gdb/reggroups.h
+++ b/gdb/reggroups.h
@@ -41,6 +41,10 @@ extern struct reggroup *const restore_reggroup;
 /* Create a new local register group.  */
 extern struct reggroup *reggroup_new (const char *name,
 				      enum reggroup_type type);
+/* Create a new register group allocated onto the gdbarch obstack.  */
+extern struct reggroup *reggroup_gdbarch_new (struct gdbarch *gdbarch,
+					      const char *name,
+					      enum reggroup_type type);
 
 /* Add a register group (with attribute values) to the pre-defined list.  */
 extern void reggroup_add (struct gdbarch *gdbarch, struct reggroup *group);
@@ -57,6 +61,7 @@ extern struct reggroup *reggroup_next (struct gdbarch *gdbarch,
 				       struct reggroup *last);
 extern struct reggroup *reggroup_prev (struct gdbarch *gdbarch,
 				       struct reggroup *curr);
+extern reggroup *reggroup_find (struct gdbarch *gdbarch, const char *name);
 
 /* Is REGNUM a member of REGGROUP?  */
 extern int default_register_reggroup_p (struct gdbarch *gdbarch, int regnum,
-- 
2.13.6

[PATCH v4 0/4] 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.

This series also adds documentation on tests for the feature of listing
register groups via the "info reg $reggroup" command.

-Stafford

--

Changes since v3
 * Fixes for grammar, tests and refactorings suggsted by Eli and Simon.
 See diffs for detail.

Changes since v2
 * Fixed NEWS entry XML/descriptions typo seggested by Petro
 * Rebased on latest upstream/master.

Changes since v1
 * On 'info reg $reggroup' test and docs patch
  - Suggested by Eli - Fix changelog
  - Suggested by Simon
    > Added help text in 'help info registers'
    > Fixed 'register' typos
    > Fixed style of test program
    > Fixed copyright '2017'
    > Fixed code styles in expect
 * On 'arbitrary strings' patch
  - Suggested by Simon
    > Allow for freeing reggroups
  - Suggested by Eli
    > Add documentation for this feature

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

 gdb/NEWS                             |   4 ++
 gdb/doc/gdb.texinfo                  |  16 +++--
 gdb/infcmd.c                         |   8 ++-
 gdb/reggroups.c                      |  44 ++++++++++----
 gdb/reggroups.h                      |   5 ++
 gdb/target-descriptions.c            |  58 +++++++-----------
 gdb/testsuite/gdb.base/reggroups.c   |   5 ++
 gdb/testsuite/gdb.base/reggroups.exp | 112 +++++++++++++++++++++++++++++++++++
 gdb/testsuite/gdb.xml/extra-regs.xml |   1 +
 gdb/testsuite/gdb.xml/tdesc-regs.exp |   3 +
 10 files changed, 202 insertions(+), 54 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/reggroups.c
 create mode 100644 gdb/testsuite/gdb.base/reggroups.exp

-- 
2.13.6

[PATCH v4 2/4] 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:

yyyy-mm-dd  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().
---

Changes since v3
 * No changes.

 gdb/reggroups.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/gdb/reggroups.c b/gdb/reggroups.c
index 2ecd0b494f..5d5e33f2a3 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));
 }
@@ -298,7 +291,7 @@ struct reggroup *const restore_reggroup = &restore_group;
 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.13.6

[PATCH v4 4/4] 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:

yyyy-mm-dd  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:

yyyy-mm-dd  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/doc/ChangeLog:

yyyy-mm-dd  Stafford Horne  <shorne@gmail.com>

	* gdb.texinfo (Target Description Format): Explain that arbitrary
	strings are now allowed for register groups.
---

Changes since v3
 * Removed bad paragraph from NEWS (Eli)
 * Reworded "internal hyphens" in gdb.texinfo (Eli)
 * Use == comparitor for std::string compares (Simon)
 * Use new reggroup_find() utility function (Simon)

 gdb/NEWS                             |  4 +++
 gdb/doc/gdb.texinfo                  | 11 ++++---
 gdb/target-descriptions.c            | 58 +++++++++++++-----------------------
 gdb/testsuite/gdb.xml/extra-regs.xml |  1 +
 gdb/testsuite/gdb.xml/tdesc-regs.exp |  3 ++
 5 files changed, 36 insertions(+), 41 deletions(-)

diff --git a/gdb/NEWS b/gdb/NEWS
index 44f481d1f5..c861853798 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 descriptions.  This allows for finer grain grouping of
+  registers on systems with a large amount of registers.
+
 * The 'ptype' command now accepts a '/o' flag, which prints the
   offsets and sizes of fields in a struct, like the pahole(1) tool.
 
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index a16e79bc2a..f8ecf216b4 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -41761,10 +41761,13 @@ architecture's normal floating point format) of the correct size for
 @var{bitsize}.  The default is @code{int}.
 
 @item group
-The register group to which this register belongs.  It must
-be either @code{general}, @code{float}, or @code{vector}.  If no
-@var{group} is specified, @value{GDBN} will not display the register
-in @code{info registers}.
+The register group to which this register belongs.  It can be one of the
+standard register groups @code{general}, @code{float}, @code{vector} or an
+arbitrary string.  Group names should be limited to alphanumeric characters.
+If a group name is made up of multiple words the words may be separated by
+hyphens; e.g.@: @code{special-group} or @code{ultra-special-group}.  If no
+@var{group} is specified, @value{GDBN} will not display the register in
+@code{info registers}.
 
 @end table
 
diff --git a/gdb/target-descriptions.c b/gdb/target-descriptions.c
index 88ac55f404..349cebb51b 100644
--- a/gdb/target-descriptions.c
+++ b/gdb/target-descriptions.c
@@ -111,12 +111,11 @@ struct tdesc_reg : tdesc_element
   int save_restore;
 
   /* The name of the register group containing this register, or empty
-     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 empty).  */
+     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.  */
   std::string group;
 
   /* The size of the register, in bits.  */
@@ -1279,17 +1278,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 registered 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.  */
 
@@ -1299,26 +1294,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.empty ())
-    {
-      int general_p = 0, float_p = 0, vector_p = 0;
-
-      if (reg->group == "general")
-	general_p = 1;
-      else if (reg->group == "float")
-	float_p = 1;
-      else if (reg->group == "vector")
-	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.empty ()
+      && (reg->group == reggroup_name (reggroup)))
+	return 1;
 
   if (reg != NULL
       && (reggroup == save_reggroup || reggroup == restore_reggroup))
@@ -1421,6 +1399,12 @@ tdesc_use_registers (struct gdbarch *gdbarch,
 	void **slot = htab_find_slot (reg_hash, reg.get (), INSERT);
 
 	*slot = reg.get ();
+	/* Add reggroup if its new.  */
+	if (!reg->group.empty ())
+	  if (reggroup_find (gdbarch, reg->group.c_str ()) == NULL)
+	    reggroup_add (gdbarch, reggroup_gdbarch_new (gdbarch,
+							 reg->group.c_str (),
+							 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 997d6598e5..302e64ce7d 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 d62ed9841b..e8b7fd506b 100644
--- a/gdb/testsuite/gdb.xml/tdesc-regs.exp
+++ b/gdb/testsuite/gdb.xml/tdesc-regs.exp
@@ -190,6 +190,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.13.6

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

[Eli Zaretskii]

> From: Stafford Horne <shorne@gmail.com>
> Cc: Simon Marchi <simon.marchi@polymtl.ca>,
> 	Eli Zaretskii <eliz@gnu.org>,
> 	Stafford Horne <shorne@gmail.com>
> Date: Tue, 26 Dec 2017 22:48:29 +0900
> 
> Until now this feature has existed but was not documented.  Adding docs
> and tests.
> 
> gdb/ChangeLog:
> 
> yyyy-mm-dd  Stafford Horne  <shorne@gmail.com>
> 
> 	* infcmd.c (_initialize_infcmd): Add help for info reg $reggroup
> 	and info all-registers $reggroup feature.
> 
> gdb/doc/ChangeLog:
> 
> yyyy-mm-dd  Stafford Horne  <shorne@gmail.com>
> 
> 	* gdb.texinfo (Registers): Document info reg $reggroup feature.
> 
> gdb/testsuite/ChangeLog:
> 
> yyyy-mm-dd  Stafford Horne  <shorne@gmail.com>
> 
> 	* gdb.base/reggroups.c: New file.
> 	* gdb.base/reggroups.exp: New file.

OK for the documentation parts, thanks.

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

[Eli Zaretskii]

> From: Stafford Horne <shorne@gmail.com>
> Cc: Simon Marchi <simon.marchi@polymtl.ca>,
> 	Eli Zaretskii <eliz@gnu.org>,
> 	Stafford Horne <shorne@gmail.com>
> Date: Tue, 26 Dec 2017 22:48:32 +0900
> 
> 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:
> 
> yyyy-mm-dd  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:
> 
> yyyy-mm-dd  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/doc/ChangeLog:
> 
> yyyy-mm-dd  Stafford Horne  <shorne@gmail.com>
> 
> 	* gdb.texinfo (Target Description Format): Explain that arbitrary
> 	strings are now allowed for register groups.

Thanks, the documentation parts are OK.

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

[Simon Marchi]

Hi Stafford,

I just pointed out some nits, it's fine with me to push with those fixed.

On 2017-12-26 08:48 AM, Stafford Horne wrote:
> Until now this feature has existed but was not documented.  Adding docs
> and tests.
> 
> gdb/ChangeLog:
> 
> yyyy-mm-dd  Stafford Horne  <shorne@gmail.com>
> 
> 	* infcmd.c (_initialize_infcmd): Add help for info reg $reggroup
> 	and info all-registers $reggroup feature.
> 
> gdb/doc/ChangeLog:
> 
> yyyy-mm-dd  Stafford Horne  <shorne@gmail.com>
> 
> 	* gdb.texinfo (Registers): Document info reg $reggroup feature.
> 
> gdb/testsuite/ChangeLog:
> 
> yyyy-mm-dd  Stafford Horne  <shorne@gmail.com>
> 
> 	* gdb.base/reggroups.c: New file.
> 	* gdb.base/reggroups.exp: New file.
> ---
> 
> Since v3
>  - Fixed grammar issue in gdb.texinfo pointed out by Eli.
>  - Added xref in gdb.texinfo pointed out by Eli.
>  - Documented multi-reggroup support in infcmd.c suggested by Eli.
>  - Enhanced testing to actual test info reg results suggested by Simon.
> 
>  gdb/doc/gdb.texinfo                  |   5 ++
>  gdb/infcmd.c                         |   8 ++-
>  gdb/testsuite/gdb.base/reggroups.c   |   5 ++
>  gdb/testsuite/gdb.base/reggroups.exp | 112 +++++++++++++++++++++++++++++++++++
>  4 files changed, 128 insertions(+), 2 deletions(-)
>  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 60ed80c363..a16e79bc2a 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -11023,6 +11023,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 registers in each of the specified
> +@var{reggroup}s.  The @var{reggoup} can be any of those returned by
> +@code{maint print reggroups} (@pxref{Maintenance Commands}).
> +
>  @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/infcmd.c b/gdb/infcmd.c
> index 8bde28eab6..1b63f9b730 100644
> --- a/gdb/infcmd.c
> +++ b/gdb/infcmd.c
> @@ -3460,13 +3460,17 @@ interrupt all running threads in non-stop mode, use the -a option."));
>  
>    c = add_info ("registers", info_registers_command, _("\
>  List of integer registers and their contents, for selected stack frame.\n\
> -Register name as argument means describe only that register."));
> +One or more register names as argument means describe the given registers.\n\
> +One or more register group names as argument means describe the registers\n\
> +in the named register groups."));
>    add_info_alias ("r", "registers", 1);
>    set_cmd_completer (c, reg_or_group_completer);
>  
>    c = add_info ("all-registers", info_all_registers_command, _("\
>  List of all registers and their contents, for selected stack frame.\n\
> -Register name as argument means describe only that register."));
> +One or more register names as argument means describe the given registers.\n\
> +One or more register group names as argument means describe the registers\n\
> +in the named register groups."));
>    set_cmd_completer (c, reg_or_group_completer);
>  
>    add_info ("program", info_program_command,
> diff --git a/gdb/testsuite/gdb.base/reggroups.c b/gdb/testsuite/gdb.base/reggroups.c
> new file mode 100644
> index 0000000000..8e8f518aae
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/reggroups.c
> @@ -0,0 +1,5 @@

I think we need a license head for this file, even if it's trivial.  At least
that's what we do for other tests with trivial source files.

> +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 0000000000..4fc2c9992a
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/reggroups.exp
> @@ -0,0 +1,112 @@
> +# This testcase is part of GDB, the GNU debugger.
> +
> +# Copyright 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
> +}
> +
> +# Fetch all reggroups from 'maint print reggroups'.
> +
> +proc fetch_reggroups {test} {
> +    global gdb_prompt
> +    global expect_out
> +
> +    set reggroups {}
> +    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" {

You don't need to escape the - in the regex.  I assume you put it to avoid
it meaning a range in the character set?  When it's first or last in the set,
it doesn't need to be escaped.  But anyway, here, the backslash is removed
by TCL when it interprets the string (even though the dash has no special
meaning for tcl, unlike the square brackets for example), so it's not there
anymore when the regex gets evaluated.  You would need two backslashes for
that.  But since it's not needed in this case, I suggest to remove it.

> +	    lappend reggroups $expect_out(1,string)
> +	    exp_continue
> +	}
> +	-re "$gdb_prompt $" {
> +	    if { [llength $reggroups] != 0 } {
> +		pass $test
> +	    } else {
> +		fail "$test - didn't fetch any reggroups"
> +	    }
> +	}

In cases like this, it's good to use the same test name for the pass and the fail.
If this test starts failing, tools that analyze regressions (for example in the buildbot)
will show it as a FAIL -> PASS, rather than a new FAIL, so it's a bit clearer.

And in this case I would suggest using gdb_assert:

  gdb_assert "[llength $reggroups] != 0" $test

Finally, you should use the same test name here than what you pass to gdb_test_multiple,
so that same name will be used regardless of the outcome.  For example, if an internal
error happens when sending the command, the gdb_test_multiple code will catch it and
cause a FAIL with that test name.  And for the reason stated above, it's good if the
test name is consistent.  So to summarize, please change:

    gdb_test_multiple "maint print reggroups" "get reggroups"

to

    gdb_test_multiple "maint print reggroups" $test

> +    }
> +
> +    return $reggroups
> +}
> +
> +# Fetch all registers for a reggroup from 'info reg <reggroup>'.
> +
> +proc fetch_reggroup_regs {reggroup test} {
> +    global gdb_prompt
> +    global expext_out

Typo here?  It probably means that it's not required?  If it's not required here,
it's probably not required in the other proc either.

> +
> +    # The command info reg <reggroup> will return something like the following:
> +    #
> +    # r0             0x0      0^M
> +    # r1             0x7fdffc 0x7fdffc^M
> +    # r2             0x7fe000 0x7fe000^M
> +    # npc            0x23a8   0x23a8 <main+12>^M
> +    # sr             0x8401   [ SM CY FO CID=0 ]^M
> +    #
> +    # We parse out and return the reg names, this is done by detecting
> +    # that for each line we have a register name followed by a $hex number.
> +    set regs {}
> +    gdb_test_multiple "info reg $reggroup" "info reg $reggroup" {

Use $test as the test name.

> +	-re "info reg $reggroup\r\n" {
> +	    exp_continue
> +	}
> +	-re "^(\[0-9a-zA-Z\-\]+)\[ \t\]+(0x\[0-9a-f\]+)\[ \t\]+(\[^\n\r\]+)\r\n" {

Same thing about escaping the dash.

There are registers whose value is not a simple number, like vector registers:

xmm0           {v4_float = {0x0, 0x0, 0x0, 0x0}, v2_double = {0x0, 0x0}, v16_int8 = {0x0 <repeats 16 times>}, ...

Those won't be matched and returned by this regex (not sure where that output goes,
since it's never matched).  I think it doesn't matter for this test, but I just
want to be sure you've considered it.

> +	    lappend regs $expect_out(1,string)
> +	    exp_continue
> +	}
> +	-re "Invalid register .*\r\n" {
> +	    fail "$test - unexpected invalid register response"

Again, we should use the same test name.  But if you want to put more
info about the failure, as you do here, put it in parentheses.  The buildbot
strips trailing text in parentheses from test names when analyzing regressions,
so "$test" and "$test (unexpected invalid register response)" will be
considered as the same test name.

https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Do_not_use_.22tail_parentheses.22_on_test_messages

> +	}
> +	-re "$gdb_prompt $" {
> +	    pass $test
> +	}
> +    }
> +    return $regs
> +}
> +
> +set reggroups [fetch_reggroups "fetch reggroups"]
> +set regcount 0
> +foreach reggroup $reggroups {
> +    set regs [fetch_reggroup_regs $reggroup "fetch reggroup regs $reggroup"]
> +    set regcount [expr $regcount + [llength $regs]]
> +}
> +if { $regcount != 0 } {
> +    pass "system has reggroup regs $regcount"
> +} else {
> +    fail "system has no reggroup regs at all"
> +}

I suggest using gdb_assert.

> +
> +# If this fails it means that probably someone changed the error text returned
> +# for an invalid register argument.  If that happens we should fix the pattern
> +# here and in the fetch_reggroup_regs procedure above.
> +gdb_test "info reg invalid-reggroup" "Invalid register .*" \
> +    "info reg invalid-reggroup should report 'Invalid register'"

I would suggest putting the regex in a variable

  set invalid_register_re "Invalid register .*"

Simon

Re: [PATCH v4 3/4] reggroups: Add reggroup_gdbarch_new, reggroup_find for dynamic reggroups

[Simon Marchi]

On 2017-12-26 08:48 AM, Stafford Horne wrote:
> Traditionally reggroups have been created via reggroup_new() during
> initialization code and never freed.  Now, if we want to initialize
> reggroups dynamically (i.e. in target description) we should be able to
> free them.  Create this function reggroup_gdbarch_new() which will
> allocate the reggroup memory onto the passed gdbarch obstack.
> 
> Also creating reggroup_find() as a utility to find a gdbarch registered
> reggroup object by name.
> 
> gdb/ChangeLog:
> 
> yyyy-mm-dd  Stafford Horne  <shorne@gmail.com>
> 
> 	* reggroups.c (reggroup_gdbarch_new): New function.
> 	(reggroup_find): New function.
> 	* reggroups.h (reggroup_gdbarch_new): New function.
> 	(reggroup_find): New function.
> ---
> Changes since v3
>  * Added the reggroup_find function.
> 
>  gdb/reggroups.c | 29 +++++++++++++++++++++++++++++
>  gdb/reggroups.h |  5 +++++
>  2 files changed, 34 insertions(+)
> 
> diff --git a/gdb/reggroups.c b/gdb/reggroups.c
> index 5d5e33f2a3..4e6b452f88 100644
> --- a/gdb/reggroups.c
> +++ b/gdb/reggroups.c
> @@ -46,6 +46,18 @@ reggroup_new (const char *name, enum reggroup_type type)
>    return group;
>  }
>  
> +struct reggroup *
> +reggroup_gdbarch_new (struct gdbarch *gdbarch, const char *name,
> +		      enum reggroup_type type)
> +{
> +  struct reggroup *group = GDBARCH_OBSTACK_ZALLOC (gdbarch,
> +						   struct reggroup);
> +
> +  group->name = gdbarch_obstack_strdup (gdbarch, name);
> +  group->type = type;
> +  return group;
> +}
> +
>  /* Register group attributes.  */
>  
>  const char *
> @@ -201,6 +213,23 @@ default_register_reggroup_p (struct gdbarch *gdbarch, int regnum,
>    return 0;   
>  }
>  
> +/* Find a reggroup by name.  */
> +
> +reggroup *
> +reggroup_find (struct gdbarch *gdbarch, const char *name)
> +{
> +  struct reggroup *group;
> +
> +  for (group = reggroup_next (gdbarch, NULL);
> +       group != NULL;
> +       group = reggroup_next (gdbarch, group))
> +    {
> +      if (strcmp (name, reggroup_name (group)) == 0)
> +	return group;
> +    }
> +  return NULL;
> +}
> +
>  /* Dump out a table of register groups for the current architecture.  */
>  
>  static void
> diff --git a/gdb/reggroups.h b/gdb/reggroups.h
> index 18fc1bf294..5cfa51abff 100644
> --- a/gdb/reggroups.h
> +++ b/gdb/reggroups.h
> @@ -41,6 +41,10 @@ extern struct reggroup *const restore_reggroup;
>  /* Create a new local register group.  */
>  extern struct reggroup *reggroup_new (const char *name,
>  				      enum reggroup_type type);
> +/* Create a new register group allocated onto the gdbarch obstack.  */
> +extern struct reggroup *reggroup_gdbarch_new (struct gdbarch *gdbarch,
> +					      const char *name,
> +					      enum reggroup_type type);
>  
>  /* Add a register group (with attribute values) to the pre-defined list.  */
>  extern void reggroup_add (struct gdbarch *gdbarch, struct reggroup *group);
> @@ -57,6 +61,7 @@ extern struct reggroup *reggroup_next (struct gdbarch *gdbarch,
>  				       struct reggroup *last);
>  extern struct reggroup *reggroup_prev (struct gdbarch *gdbarch,
>  				       struct reggroup *curr);
> +extern reggroup *reggroup_find (struct gdbarch *gdbarch, const char *name);

Please add a doc comment for this declaration.  In the .c, write

  /* See reggroups.h.  */

for both functions.

It's ok to push with that fixed.

Simon

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

[Simon Marchi]

On 2017-12-26 08:48 AM, Stafford Horne wrote:
> 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:
> 
> yyyy-mm-dd  Franck Jullien  <franck.jullien@gmail.com>
> 	    Stafford Horne  <shorne@gmail.com>
> 
> 	* target-descriptions.c (tdesc_register_in_reggroup_p): Support
> 	arbitrary strings.

Please mention tdesc_use_registers in the ChangeLog.  I also usually mention
changes to comments, such as the change to tdesc_reg::group's comment.

LGTM with that fixed.

Thanks!

Simon

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

[Stafford Horne]

On Tue, Dec 26, 2017 at 07:20:09PM -0500, Simon Marchi wrote:
> Hi Stafford,
> 
> I just pointed out some nits, it's fine with me to push with those fixed.
> 
> On 2017-12-26 08:48 AM, Stafford Horne wrote:
> > Until now this feature has existed but was not documented.  Adding docs
> > and tests.
> > 
> > gdb/ChangeLog:
> > 
> > yyyy-mm-dd  Stafford Horne  <shorne@gmail.com>
> > 
> > 	* infcmd.c (_initialize_infcmd): Add help for info reg $reggroup
> > 	and info all-registers $reggroup feature.
> > 
> > gdb/doc/ChangeLog:
> > 
> > yyyy-mm-dd  Stafford Horne  <shorne@gmail.com>
> > 
> > 	* gdb.texinfo (Registers): Document info reg $reggroup feature.
> > 
> > gdb/testsuite/ChangeLog:
> > 
> > yyyy-mm-dd  Stafford Horne  <shorne@gmail.com>
> > 
> > 	* gdb.base/reggroups.c: New file.
> > 	* gdb.base/reggroups.exp: New file.
> > ---
> > 
> > Since v3
> >  - Fixed grammar issue in gdb.texinfo pointed out by Eli.
> >  - Added xref in gdb.texinfo pointed out by Eli.
> >  - Documented multi-reggroup support in infcmd.c suggested by Eli.
> >  - Enhanced testing to actual test info reg results suggested by Simon.
> > 
> >  gdb/doc/gdb.texinfo                  |   5 ++
> >  gdb/infcmd.c                         |   8 ++-
> >  gdb/testsuite/gdb.base/reggroups.c   |   5 ++
> >  gdb/testsuite/gdb.base/reggroups.exp | 112 +++++++++++++++++++++++++++++++++++
> >  4 files changed, 128 insertions(+), 2 deletions(-)
> >  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 60ed80c363..a16e79bc2a 100644
> > --- a/gdb/doc/gdb.texinfo
> > +++ b/gdb/doc/gdb.texinfo
> > @@ -11023,6 +11023,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 registers in each of the specified
> > +@var{reggroup}s.  The @var{reggoup} can be any of those returned by
> > +@code{maint print reggroups} (@pxref{Maintenance Commands}).
> > +
> >  @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/infcmd.c b/gdb/infcmd.c
> > index 8bde28eab6..1b63f9b730 100644
> > --- a/gdb/infcmd.c
> > +++ b/gdb/infcmd.c
> > @@ -3460,13 +3460,17 @@ interrupt all running threads in non-stop mode, use the -a option."));
> >  
> >    c = add_info ("registers", info_registers_command, _("\
> >  List of integer registers and their contents, for selected stack frame.\n\
> > -Register name as argument means describe only that register."));
> > +One or more register names as argument means describe the given registers.\n\
> > +One or more register group names as argument means describe the registers\n\
> > +in the named register groups."));
> >    add_info_alias ("r", "registers", 1);
> >    set_cmd_completer (c, reg_or_group_completer);
> >  
> >    c = add_info ("all-registers", info_all_registers_command, _("\
> >  List of all registers and their contents, for selected stack frame.\n\
> > -Register name as argument means describe only that register."));
> > +One or more register names as argument means describe the given registers.\n\
> > +One or more register group names as argument means describe the registers\n\
> > +in the named register groups."));
> >    set_cmd_completer (c, reg_or_group_completer);
> >  
> >    add_info ("program", info_program_command,
> > diff --git a/gdb/testsuite/gdb.base/reggroups.c b/gdb/testsuite/gdb.base/reggroups.c
> > new file mode 100644
> > index 0000000000..8e8f518aae
> > --- /dev/null
> > +++ b/gdb/testsuite/gdb.base/reggroups.c
> > @@ -0,0 +1,5 @@
> 
> I think we need a license head for this file, even if it's trivial.  At least
> that's what we do for other tests with trivial source files.

OK.

> > +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 0000000000..4fc2c9992a
> > --- /dev/null
> > +++ b/gdb/testsuite/gdb.base/reggroups.exp
> > @@ -0,0 +1,112 @@
> > +# This testcase is part of GDB, the GNU debugger.
> > +
> > +# Copyright 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
> > +}
> > +
> > +# Fetch all reggroups from 'maint print reggroups'.
> > +
> > +proc fetch_reggroups {test} {
> > +    global gdb_prompt
> > +    global expect_out
> > +
> > +    set reggroups {}
> > +    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" {
> 
> You don't need to escape the - in the regex.  I assume you put it to avoid
> it meaning a range in the character set?  When it's first or last in the set,
> it doesn't need to be escaped.  But anyway, here, the backslash is removed
> by TCL when it interprets the string (even though the dash has no special
> meaning for tcl, unlike the square brackets for example), so it's not there
> anymore when the regex gets evaluated.  You would need two backslashes for
> that.  But since it's not needed in this case, I suggest to remove it.

Thanks, I admit I am pretty goood with regular expressions, but I really don't
remember all of the escaping details.

Will fix here and the other spot.

> > +	    lappend reggroups $expect_out(1,string)
> > +	    exp_continue
> > +	}
> > +	-re "$gdb_prompt $" {
> > +	    if { [llength $reggroups] != 0 } {
> > +		pass $test
> > +	    } else {
> > +		fail "$test - didn't fetch any reggroups"
> > +	    }
> > +	}
> 
> In cases like this, it's good to use the same test name for the pass and the fail.
> If this test starts failing, tools that analyze regressions (for example in the buildbot)
> will show it as a FAIL -> PASS, rather than a new FAIL, so it's a bit clearer.

OK. Didn't know that.

> And in this case I would suggest using gdb_assert:
> 
>   gdb_assert "[llength $reggroups] != 0" $test
> 
> Finally, you should use the same test name here than what you pass to gdb_test_multiple,
> so that same name will be used regardless of the outcome.  For example, if an internal
> error happens when sending the command, the gdb_test_multiple code will catch it and
> cause a FAIL with that test name.  And for the reason stated above, it's good if the
> test name is consistent.  So to summarize, please change:
> 
>     gdb_test_multiple "maint print reggroups" "get reggroups"
> 
> to
> 
>     gdb_test_multiple "maint print reggroups" $test
> 
> > +    }
> > +
> > +    return $reggroups
> > +}
> > +
> > +# Fetch all registers for a reggroup from 'info reg <reggroup>'.
> > +
> > +proc fetch_reggroup_regs {reggroup test} {
> > +    global gdb_prompt
> > +    global expext_out
> 
> Typo here?  It probably means that it's not required?  If it's not required here,
> it's probably not required in the other proc either.

Right, gdb_prompt is needed, but not expect_out.

> > +
> > +    # The command info reg <reggroup> will return something like the following:
> > +    #
> > +    # r0             0x0      0^M
> > +    # r1             0x7fdffc 0x7fdffc^M
> > +    # r2             0x7fe000 0x7fe000^M
> > +    # npc            0x23a8   0x23a8 <main+12>^M
> > +    # sr             0x8401   [ SM CY FO CID=0 ]^M
> > +    #
> > +    # We parse out and return the reg names, this is done by detecting
> > +    # that for each line we have a register name followed by a $hex number.
> > +    set regs {}
> > +    gdb_test_multiple "info reg $reggroup" "info reg $reggroup" {
> 
> Use $test as the test name.
> 
> > +	-re "info reg $reggroup\r\n" {
> > +	    exp_continue
> > +	}
> > +	-re "^(\[0-9a-zA-Z\-\]+)\[ \t\]+(0x\[0-9a-f\]+)\[ \t\]+(\[^\n\r\]+)\r\n" {
> 
> Same thing about escaping the dash.
> 
> There are registers whose value is not a simple number, like vector registers:
> 
> xmm0           {v4_float = {0x0, 0x0, 0x0, 0x0}, v2_double = {0x0, 0x0}, v16_int8 = {0x0 <repeats 16 times>}, ...
> 
> Those won't be matched and returned by this regex (not sure where that output goes,
> since it's never matched).  I think it doesn't matter for this test, but I just
> want to be sure you've considered it.

My thought is that the test is trying to ensure we capture some interger
registers and no Invalid register result accross the system.  I think limiting
it to this is ok.   Let add a comment.

> > +	    lappend regs $expect_out(1,string)
> > +	    exp_continue
> > +	}
> > +	-re "Invalid register .*\r\n" {
> > +	    fail "$test - unexpected invalid register response"
> 
> Again, we should use the same test name.  But if you want to put more
> info about the failure, as you do here, put it in parentheses.  The buildbot
> strips trailing text in parentheses from test names when analyzing regressions,
> so "$test" and "$test (unexpected invalid register response)" will be
> considered as the same test name.
> 
> https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Do_not_use_.22tail_parentheses.22_on_test_messages

OK, good to know.

> > +	}
> > +	-re "$gdb_prompt $" {
> > +	    pass $test
> > +	}
> > +    }
> > +    return $regs
> > +}
> > +
> > +set reggroups [fetch_reggroups "fetch reggroups"]
> > +set regcount 0
> > +foreach reggroup $reggroups {
> > +    set regs [fetch_reggroup_regs $reggroup "fetch reggroup regs $reggroup"]
> > +    set regcount [expr $regcount + [llength $regs]]
> > +}
> > +if { $regcount != 0 } {
> > +    pass "system has reggroup regs $regcount"
> > +} else {
> > +    fail "system has no reggroup regs at all"
> > +}
> 
> I suggest using gdb_assert.

OK.

> > +
> > +# If this fails it means that probably someone changed the error text returned
> > +# for an invalid register argument.  If that happens we should fix the pattern
> > +# here and in the fetch_reggroup_regs procedure above.
> > +gdb_test "info reg invalid-reggroup" "Invalid register .*" \
> > +    "info reg invalid-reggroup should report 'Invalid register'"
> 
> I would suggest putting the regex in a variable
> 
>   set invalid_register_re "Invalid register .*"

OK.

> Simon