All of lore.kernel.org
 help / color / mirror / Atom feed
* [OpenRISC] [PATCH v3 0/4] Support for arbitrary reggroups
@ 2017-12-19 14:22 Stafford Horne
  2017-12-19 14:22 ` [OpenRISC] [PATCH v3 1/4] reggroups: Add test and docs for `info reg $reggroup` feature Stafford Horne
                   ` (3 more replies)
  0 siblings, 4 replies; 19+ messages in thread
From: Stafford Horne @ 2017-12-19 14:22 UTC (permalink / raw)
  To: openrisc

Hello,

(Its been some time since v2, but now that openrisc is upstream I can focus on
this

There were not many concerns raised with v2.  One possible issue with v3 is that
the reggroups struct uses 'char *' for strings.  We now compare std::string vs
'char *' which is a bit clumsy,  but I didnt think it calls for converting
reggroups strings to 'std::string' yet.)

Onto the change:

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 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: Create reggroup_gdbarch_new for dynamic reggroups
  tdesc: handle arbitrary strings in tdesc_register_in_reggroup_p

 gdb/NEWS                             |  7 ++++
 gdb/doc/gdb.texinfo                  | 14 +++++--
 gdb/infcmd.c                         |  8 +++-
 gdb/reggroups.c                      | 27 +++++++------
 gdb/reggroups.h                      |  4 ++
 gdb/target-descriptions.c            | 74 ++++++++++++++++++------------------
 gdb/testsuite/gdb.base/reggroups.c   |  5 +++
 gdb/testsuite/gdb.base/reggroups.exp | 63 ++++++++++++++++++++++++++++++
 gdb/testsuite/gdb.xml/extra-regs.xml |  1 +
 gdb/testsuite/gdb.xml/tdesc-regs.exp |  3 ++
 10 files changed, 152 insertions(+), 54 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/reggroups.c
 create mode 100644 gdb/testsuite/gdb.base/reggroups.exp

-- 
2.13.6


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [OpenRISC] [PATCH v3 1/4] reggroups: Add test and docs for `info reg $reggroup` feature
  2017-12-19 14:22 [OpenRISC] [PATCH v3 0/4] Support for arbitrary reggroups Stafford Horne
@ 2017-12-19 14:22 ` Stafford Horne
  2017-12-19 16:23   ` Eli Zaretskii
  2017-12-21  2:58   ` Simon Marchi
  2017-12-19 14:22 ` [OpenRISC] [PATCH v3 2/4] reggroups: Convert reggroups from post_init to pre_init Stafford Horne
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 19+ messages in thread
From: Stafford Horne @ 2017-12-19 14:22 UTC (permalink / raw)
  To: openrisc

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

gdb/ChangeLog:

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

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

gdb/doc/ChangeLog:

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

	* gdb.texinfo (Registers): 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/infcmd.c                         |  8 +++--
 gdb/testsuite/gdb.base/reggroups.c   |  5 +++
 gdb/testsuite/gdb.base/reggroups.exp | 63 ++++++++++++++++++++++++++++++++++++
 4 files changed, 79 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..e169260e7e 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).
 
+ at item info registers @var{reggroup} @dots{}
+Print the name and value of the registers in each of the specified
+ at var{reggroup}.  The @var{reggoup} can be any of those returned by
+ at 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/infcmd.c b/gdb/infcmd.c
index 8bde28eab6..6e449d4a0e 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."));
+Register name as argument means describe only that register.\n\
+Register group name as argument means describe the registers in the\n\
+named register group."));
   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."));
+Register name as argument means describe only that register.\n\
+Register group name as argument means describe the registers in the\n\
+named register group."));
   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..c10f02567d
--- /dev/null
+++ b/gdb/testsuite/gdb.base/reggroups.exp
@@ -0,0 +1,63 @@
+# 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
+}
+
+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
+}
+
+set reggroups [fetch_reggroups "fetch reggroups"]
+
+foreach reggroup $reggroups {
+    gdb_test "info reg $reggroup" ".*" "info reg $reggroup"
+}
-- 
2.13.6


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [OpenRISC] [PATCH v3 2/4] reggroups: Convert reggroups from post_init to pre_init
  2017-12-19 14:22 [OpenRISC] [PATCH v3 0/4] Support for arbitrary reggroups Stafford Horne
  2017-12-19 14:22 ` [OpenRISC] [PATCH v3 1/4] reggroups: Add test and docs for `info reg $reggroup` feature Stafford Horne
@ 2017-12-19 14:22 ` Stafford Horne
  2017-12-21  3:03   ` Simon Marchi
  2017-12-19 14:22 ` [OpenRISC] [PATCH v3 3/4] reggroups: Create reggroup_gdbarch_new for dynamic reggroups Stafford Horne
  2017-12-19 14:22 ` [OpenRISC] [PATCH v3 4/4] tdesc: handle arbitrary strings in tdesc_register_in_reggroup_p Stafford Horne
  3 siblings, 1 reply; 19+ messages in thread
From: Stafford Horne @ 2017-12-19 14:22 UTC (permalink / raw)
  To: openrisc

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


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [OpenRISC] [PATCH v3 3/4] reggroups: Create reggroup_gdbarch_new for dynamic reggroups
  2017-12-19 14:22 [OpenRISC] [PATCH v3 0/4] Support for arbitrary reggroups Stafford Horne
  2017-12-19 14:22 ` [OpenRISC] [PATCH v3 1/4] reggroups: Add test and docs for `info reg $reggroup` feature Stafford Horne
  2017-12-19 14:22 ` [OpenRISC] [PATCH v3 2/4] reggroups: Convert reggroups from post_init to pre_init Stafford Horne
@ 2017-12-19 14:22 ` Stafford Horne
  2017-12-21  3:15   ` Simon Marchi
  2017-12-19 14:22 ` [OpenRISC] [PATCH v3 4/4] tdesc: handle arbitrary strings in tdesc_register_in_reggroup_p Stafford Horne
  3 siblings, 1 reply; 19+ messages in thread
From: Stafford Horne @ 2017-12-19 14:22 UTC (permalink / raw)
  To: openrisc

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.

gdb/ChangeLog:

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

	* reggroups.c (reggroup_gdbarch_new): New function.
	* reggroups.h (reggroup_gdbarch_new): New function.
---
 gdb/reggroups.c | 12 ++++++++++++
 gdb/reggroups.h |  4 ++++
 2 files changed, 16 insertions(+)

diff --git a/gdb/reggroups.c b/gdb/reggroups.c
index 5d5e33f2a3..acad91a0ab 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 *
diff --git a/gdb/reggroups.h b/gdb/reggroups.h
index 18fc1bf294..c1653cd39d 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);
-- 
2.13.6


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [OpenRISC] [PATCH v3 4/4] tdesc: handle arbitrary strings in tdesc_register_in_reggroup_p
  2017-12-19 14:22 [OpenRISC] [PATCH v3 0/4] Support for arbitrary reggroups Stafford Horne
                   ` (2 preceding siblings ...)
  2017-12-19 14:22 ` [OpenRISC] [PATCH v3 3/4] reggroups: Create reggroup_gdbarch_new for dynamic reggroups Stafford Horne
@ 2017-12-19 14:22 ` Stafford Horne
  2017-12-19 16:27   ` Eli Zaretskii
  2017-12-21  3:29   ` Simon Marchi
  3 siblings, 2 replies; 19+ messages in thread
From: Stafford Horne @ 2017-12-19 14:22 UTC (permalink / raw)
  To: openrisc

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

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

	* gdb.texinfo (Target Description Format): Explain that arbitrary
	strings are now allowed for register groups.
---
 gdb/NEWS                             |  7 ++++
 gdb/doc/gdb.texinfo                  |  9 +++--
 gdb/target-descriptions.c            | 74 ++++++++++++++++++------------------
 gdb/testsuite/gdb.xml/extra-regs.xml |  1 +
 gdb/testsuite/gdb.xml/tdesc-regs.exp |  3 ++
 5 files changed, 53 insertions(+), 41 deletions(-)

diff --git a/gdb/NEWS b/gdb/NEWS
index 44f481d1f5..7d27262aee 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -3,6 +3,13 @@
 
 *** 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.
+
+* On Unix systems, GDBserver now does globbing expansion and variable
+  substitution in inferior command line arguments.
+
 * 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 e169260e7e..a71faaa89c 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -41761,10 +41761,11 @@ 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
- at 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.  The string should be limited to alphanumeric characters
+and internal hyphens.  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..1435f3f4cf 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 ()
+      && (strcmp (reg->group.c_str (), reggroup_name (reggroup)) == 0))
+	return 1;
 
   if (reg != NULL
       && (reggroup == save_reggroup || reggroup == restore_reggroup))
@@ -1421,6 +1399,28 @@ 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 ())
+	  {
+	    struct reggroup *group;
+	    bool group_exists = false;
+
+	    for (group = reggroup_next (gdbarch, NULL);
+		 group != NULL;
+		 group = reggroup_next (gdbarch, group))
+	      {
+		if (strcmp (reg->group.c_str (), reggroup_name (group)) == 0)
+		  {
+		    group_exists = true;
+		    break;
+		  }
+	      }
+
+	    if (!group_exists)
+	      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


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [OpenRISC] [PATCH v3 1/4] reggroups: Add test and docs for `info reg $reggroup` feature
  2017-12-19 14:22 ` [OpenRISC] [PATCH v3 1/4] reggroups: Add test and docs for `info reg $reggroup` feature Stafford Horne
@ 2017-12-19 16:23   ` Eli Zaretskii
  2017-12-20 10:40     ` Stafford Horne
  2017-12-21  2:40     ` Simon Marchi
  2017-12-21  2:58   ` Simon Marchi
  1 sibling, 2 replies; 19+ messages in thread
From: Eli Zaretskii @ 2017-12-19 16:23 UTC (permalink / raw)
  To: openrisc

> From: Stafford Horne <shorne@gmail.com>
> Cc: Openrisc <openrisc@lists.librecores.org>,	Stafford Horne <shorne@gmail.com>
> Date: Tue, 19 Dec 2017 23:22:54 +0900
> 
> Until now this feature has existed but was not documented.  Adding docs
> and tests.

Thanks!

> + at item info registers @var{reggroup} @dots{}
> +Print the name and value of the registers in each of the specified
> + at var{reggroup}.  The @var{reggoup} can be any of those returned by

Please use "@var{reggroup}s", with the trailing "s", otherwise this is
not correct English.

> + at code{maint print reggroups}.

Please add here a cross-reference to the node where "maint print
reggroups" is described.

>    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."));
> +Register name as argument means describe only that register.\n\
> +Register group name as argument means describe the registers in the\n\
> +named register group."));

Since this command accepts more than one reggroup, I think the doc
string should mention that.

The documentation parts are okay with these nits fixed.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [OpenRISC] [PATCH v3 4/4] tdesc: handle arbitrary strings in tdesc_register_in_reggroup_p
  2017-12-19 14:22 ` [OpenRISC] [PATCH v3 4/4] tdesc: handle arbitrary strings in tdesc_register_in_reggroup_p Stafford Horne
@ 2017-12-19 16:27   ` Eli Zaretskii
  2017-12-19 22:13     ` Stafford Horne
  2017-12-21  3:29   ` Simon Marchi
  1 sibling, 1 reply; 19+ messages in thread
From: Eli Zaretskii @ 2017-12-19 16:27 UTC (permalink / raw)
  To: openrisc

> From: Stafford Horne <shorne@gmail.com>
> Cc: Openrisc <openrisc@lists.librecores.org>,	Stafford Horne <shorne@gmail.com>
> diff --git a/gdb/NEWS b/gdb/NEWS
> index 44f481d1f5..7d27262aee 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -3,6 +3,13 @@
>  
>  *** 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.
> +
> +* On Unix systems, GDBserver now does globbing expansion and variable
> +  substitution in inferior command line arguments.

The second paragraph doesn't belong to this changeset, right?

>  @item group
> -The register group to which this register belongs.  It must
> -be either @code{general}, @code{float}, or @code{vector}.  If no
> - at 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.  The string should be limited to alphanumeric characters
> +and internal hyphens.  If no @var{group} is specified, @value{GDBN} will

What do you mean by "internal hyphens"?

Thanks.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [OpenRISC] [PATCH v3 4/4] tdesc: handle arbitrary strings in tdesc_register_in_reggroup_p
  2017-12-19 16:27   ` Eli Zaretskii
@ 2017-12-19 22:13     ` Stafford Horne
  2017-12-20 13:34       ` Stafford Horne
  2017-12-20 15:53       ` Eli Zaretskii
  0 siblings, 2 replies; 19+ messages in thread
From: Stafford Horne @ 2017-12-19 22:13 UTC (permalink / raw)
  To: openrisc

On Tue, Dec 19, 2017 at 06:27:33PM +0200, Eli Zaretskii wrote:
> > From: Stafford Horne <shorne@gmail.com>
> > Cc: Openrisc <openrisc@lists.librecores.org>,	Stafford Horne <shorne@gmail.com>
> > diff --git a/gdb/NEWS b/gdb/NEWS
> > index 44f481d1f5..7d27262aee 100644
> > --- a/gdb/NEWS
> > +++ b/gdb/NEWS
> > @@ -3,6 +3,13 @@
> >  
> >  *** 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.
> > +
> > +* On Unix systems, GDBserver now does globbing expansion and variable
> > +  substitution in inferior command line arguments.
> 
> The second paragraph doesn't belong to this changeset, right?

Right, I accidently brought this in during conflict resolution.  Will fix.

> >  @item group
> > -The register group to which this register belongs.  It must
> > -be either @code{general}, @code{float}, or @code{vector}.  If no
> > - at 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.  The string should be limited to alphanumeric characters
> > +and internal hyphens.  If no @var{group} is specified, @value{GDBN} will
> 
> What do you mean by "internal hyphens"?

This means, hyphens withing the register group name, not starting or ending with
hyphens.  (i.e. special-spr,  but not rg1- or -rg2)

-Stafford

> Thanks.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [OpenRISC] [PATCH v3 1/4] reggroups: Add test and docs for `info reg $reggroup` feature
  2017-12-19 16:23   ` Eli Zaretskii
@ 2017-12-20 10:40     ` Stafford Horne
  2017-12-21  2:40     ` Simon Marchi
  1 sibling, 0 replies; 19+ messages in thread
From: Stafford Horne @ 2017-12-20 10:40 UTC (permalink / raw)
  To: openrisc

On Tue, Dec 19, 2017 at 06:23:29PM +0200, Eli Zaretskii wrote:
> > From: Stafford Horne <shorne@gmail.com>
> > Cc: Openrisc <openrisc@lists.librecores.org>,	Stafford Horne <shorne@gmail.com>
> > Date: Tue, 19 Dec 2017 23:22:54 +0900
> > 
> > Until now this feature has existed but was not documented.  Adding docs
> > and tests.
> 
> Thanks!
> 
> > + at item info registers @var{reggroup} @dots{}
> > +Print the name and value of the registers in each of the specified
> > + at var{reggroup}.  The @var{reggoup} can be any of those returned by
> 
> Please use "@var{reggroup}s", with the trailing "s", otherwise this is
> not correct English.

OK.

> > + at code{maint print reggroups}.

OK.

> Please add here a cross-reference to the node where "maint print
> reggroups" is described.
> 
> >    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."));
> > +Register name as argument means describe only that register.\n\
> > +Register group name as argument means describe the registers in the\n\
> > +named register group."));
> 
> Since this command accepts more than one reggroup, I think the doc
> string should mention that.

OK.

> The documentation parts are okay with these nits fixed.

Thank you.

-Stafford

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [OpenRISC] [PATCH v3 4/4] tdesc: handle arbitrary strings in tdesc_register_in_reggroup_p
  2017-12-19 22:13     ` Stafford Horne
@ 2017-12-20 13:34       ` Stafford Horne
  2017-12-20 15:53       ` Eli Zaretskii
  1 sibling, 0 replies; 19+ messages in thread
From: Stafford Horne @ 2017-12-20 13:34 UTC (permalink / raw)
  To: openrisc

Hi Eli

On Wed, Dec 20, 2017 at 07:13:00AM +0900, Stafford Horne wrote:
> On Tue, Dec 19, 2017 at 06:27:33PM +0200, Eli Zaretskii wrote:
> > > From: Stafford Horne <shorne@gmail.com>
> > > Cc: Openrisc <openrisc@lists.librecores.org>,	Stafford Horne <shorne@gmail.com>
> > > diff --git a/gdb/NEWS b/gdb/NEWS
> > > index 44f481d1f5..7d27262aee 100644
> > > --- a/gdb/NEWS
> > > +++ b/gdb/NEWS
> > > @@ -3,6 +3,13 @@
> > >  
> > >  *** 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.
> > > +
> > > +* On Unix systems, GDBserver now does globbing expansion and variable
> > > +  substitution in inferior command line arguments.
> > 
> > The second paragraph doesn't belong to this changeset, right?
> 
> Right, I accidently brought this in during conflict resolution.  Will fix.
> 
> > >  @item group
> > > -The register group to which this register belongs.  It must
> > > -be either @code{general}, @code{float}, or @code{vector}.  If no
> > > - at 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.  The string should be limited to alphanumeric characters
> > > +and internal hyphens.  If no @var{group} is specified, @value{GDBN} will
> > 
> > What do you mean by "internal hyphens"?
> 
> This means, hyphens withing the register group name, not starting or ending with
> hyphens.  (i.e. special-spr,  but not rg1- or -rg2)

I looked a bit into this,  I seem to have pulled this text from the
target-descriptions.c file.  The line saying ".. and internal hyphens" was added
as a comment in 2007 in commit 123dc839145 by Daniel Jacobowitz.  The commit was
the introduction of the target descriptions code.

I don't see any actual enforcement of the name format.  I can leave that whole
sentence out.

But I think its useful to mention as it is the intended format.  I could change
it to say something like "alphanumeric characters and hyphens as word
separators."  or "The group name should be limited to hyphen-separated
alphanumeric strings."

-Stafford

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [OpenRISC] [PATCH v3 4/4] tdesc: handle arbitrary strings in tdesc_register_in_reggroup_p
  2017-12-19 22:13     ` Stafford Horne
  2017-12-20 13:34       ` Stafford Horne
@ 2017-12-20 15:53       ` Eli Zaretskii
  1 sibling, 0 replies; 19+ messages in thread
From: Eli Zaretskii @ 2017-12-20 15:53 UTC (permalink / raw)
  To: openrisc

> Date: Wed, 20 Dec 2017 07:13:00 +0900
> From: Stafford Horne <shorne@gmail.com>
> Cc: gdb-patches at sourceware.org, openrisc at lists.librecores.org
> 
> > >  @item group
> > > -The register group to which this register belongs.  It must
> > > -be either @code{general}, @code{float}, or @code{vector}.  If no
> > > - at 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.  The string should be limited to alphanumeric characters
> > > +and internal hyphens.  If no @var{group} is specified, @value{GDBN} will
> > 
> > What do you mean by "internal hyphens"?
> 
> This means, hyphens withing the register group name, not starting or ending with
> hyphens.  (i.e. special-spr,  but not rg1- or -rg2)

Then I think this text needs to be rephrased, so that it first says
what are the characters allowed in a reggroup name, and then says that
ranges of groups, with 2 group names separated by hyphens, are also
accepted.

Thanks.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [OpenRISC] [PATCH v3 1/4] reggroups: Add test and docs for `info reg $reggroup` feature
  2017-12-19 16:23   ` Eli Zaretskii
  2017-12-20 10:40     ` Stafford Horne
@ 2017-12-21  2:40     ` Simon Marchi
  2017-12-21  3:40       ` Eli Zaretskii
  1 sibling, 1 reply; 19+ messages in thread
From: Simon Marchi @ 2017-12-21  2:40 UTC (permalink / raw)
  To: openrisc

On 2017-12-19 11:23, Eli Zaretskii wrote:
>> From: Stafford Horne <shorne@gmail.com>
>> Cc: Openrisc <openrisc@lists.librecores.org>,	Stafford Horne 
>> <shorne@gmail.com>
>> Date: Tue, 19 Dec 2017 23:22:54 +0900
>> 
>> Until now this feature has existed but was not documented.  Adding 
>> docs
>> and tests.
> 
> Thanks!
> 
>> + at item info registers @var{reggroup} @dots{}
>> +Print the name and value of the registers in each of the specified
>> + at var{reggroup}.  The @var{reggoup} can be any of those returned by
> 
> Please use "@var{reggroup}s", with the trailing "s", otherwise this is
> not correct English.
> 
>> + at code{maint print reggroups}.
> 
> Please add here a cross-reference to the node where "maint print
> reggroups" is described.

Is it ok for a non-maint command to refer to a maint command?  AFAIK, we 
don't expect an average user to have to use maintenance commands when 
using GDB.  So maybe "maint print reggroups" should be promoted to a 
non-maint command (e.g. info register-groups)?

Simon

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [OpenRISC] [PATCH v3 1/4] reggroups: Add test and docs for `info reg $reggroup` feature
  2017-12-19 14:22 ` [OpenRISC] [PATCH v3 1/4] reggroups: Add test and docs for `info reg $reggroup` feature Stafford Horne
  2017-12-19 16:23   ` Eli Zaretskii
@ 2017-12-21  2:58   ` Simon Marchi
  2017-12-24 14:47     ` Stafford Horne
  1 sibling, 1 reply; 19+ messages in thread
From: Simon Marchi @ 2017-12-21  2:58 UTC (permalink / raw)
  To: openrisc

On 2017-12-19 09:22, Stafford Horne wrote:
> Until now this feature has existed but was not documented.  Adding docs
> and tests.
> 
> gdb/ChangeLog:
> 
> 2017-06-06  Stafford Horne  <shorne@gmail.com>
> 
> 	* infcmd.c (_initialize_infcmd): Add help for info reg $reggroup
> 	feature.
> 
> gdb/doc/ChangeLog:
> 
> 2017-06-06  Stafford Horne  <shorne@gmail.com>
> 
> 	* gdb.texinfo (Registers): 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/infcmd.c                         |  8 +++--
>  gdb/testsuite/gdb.base/reggroups.c   |  5 +++
>  gdb/testsuite/gdb.base/reggroups.exp | 63 
> ++++++++++++++++++++++++++++++++++++
>  4 files changed, 79 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..e169260e7e 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).
> 
> + at item info registers @var{reggroup} @dots{}
> +Print the name and value of the registers in each of the specified
> + at var{reggroup}.  The @var{reggoup} can be any of those returned by
> + at 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/infcmd.c b/gdb/infcmd.c
> index 8bde28eab6..6e449d4a0e 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."));
> +Register name as argument means describe only that register.\n\
> +Register group name as argument means describe the registers in the\n\
> +named register group."));
>    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."));
> +Register name as argument means describe only that register.\n\
> +Register group name as argument means describe the registers in the\n\
> +named register group."));
>    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..c10f02567d
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/reggroups.exp
> @@ -0,0 +1,63 @@
> +# 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
> +}
> +
> +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 $" {

The .* should not be necessary here.

> +	    if { [llength $reggroups] != 0 } {
> +		pass $test
> +	    } else {
> +		fail "$test - didn't fetch any reggroups"
> +	    }
> +	}
> +    }
> +
> +    return $reggroups
> +}
> +
> +set reggroups [fetch_reggroups "fetch reggroups"]
> +
> +foreach reggroup $reggroups {
> +    gdb_test "info reg $reggroup" ".*" "info reg $reggroup"
> +}

This doesn't really test anything.  If you change the line to

   gdb_test "info reg hello$reggroup" ".*" "info reg $reggroup"

to fake that the command doesn't work, the test still passes.  So if 
something breaks the feature of "info registers" handling reg groups, 
the test won't catch it.  But I understand the problem, the output is 
not really predictable.  I thought about matching at least one $hex 
number, but it's not even guaranteed (some groups like mmx output 
nothing).

What we could do (I'm open to better suggestions) is at least validate 
that it doesn't output "Invalid register", which is the message given 
when passing a register that doesn't exist.  But then, if that message 
changes one day for some reason, the test will become moot again 
(because GDB will output something else than "Invalid register" if the 
functionality breaks, but the test won't catch it).  So in addition, we 
could also validate that "info registers a_non_existent_register" does 
output "Invalid register".  This way, if some GDB developers of the 
future change the message, the test will fail, they will go look at your 
test file, read the comment that you will have left, and update the test 
accordingly.

Simon

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [OpenRISC] [PATCH v3 2/4] reggroups: Convert reggroups from post_init to pre_init
  2017-12-19 14:22 ` [OpenRISC] [PATCH v3 2/4] reggroups: Convert reggroups from post_init to pre_init Stafford Horne
@ 2017-12-21  3:03   ` Simon Marchi
  0 siblings, 0 replies; 19+ messages in thread
From: Simon Marchi @ 2017-12-21  3:03 UTC (permalink / raw)
  To: openrisc

On 2017-12-19 09:22, Stafford Horne wrote:
> 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 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));

LGTM.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [OpenRISC] [PATCH v3 3/4] reggroups: Create reggroup_gdbarch_new for dynamic reggroups
  2017-12-19 14:22 ` [OpenRISC] [PATCH v3 3/4] reggroups: Create reggroup_gdbarch_new for dynamic reggroups Stafford Horne
@ 2017-12-21  3:15   ` Simon Marchi
  0 siblings, 0 replies; 19+ messages in thread
From: Simon Marchi @ 2017-12-21  3:15 UTC (permalink / raw)
  To: openrisc

On 2017-12-19 09:22, 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.
> 
> gdb/ChangeLog:
> 
> 2017-06-10  Stafford Horne  <shorne@gmail.com>
> 
> 	* reggroups.c (reggroup_gdbarch_new): New function.
> 	* reggroups.h (reggroup_gdbarch_new): New function.
> ---
>  gdb/reggroups.c | 12 ++++++++++++
>  gdb/reggroups.h |  4 ++++
>  2 files changed, 16 insertions(+)
> 
> diff --git a/gdb/reggroups.c b/gdb/reggroups.c
> index 5d5e33f2a3..acad91a0ab 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 *
> diff --git a/gdb/reggroups.h b/gdb/reggroups.h
> index 18fc1bf294..c1653cd39d 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);

LGTM.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [OpenRISC] [PATCH v3 4/4] tdesc: handle arbitrary strings in tdesc_register_in_reggroup_p
  2017-12-19 14:22 ` [OpenRISC] [PATCH v3 4/4] tdesc: handle arbitrary strings in tdesc_register_in_reggroup_p Stafford Horne
  2017-12-19 16:27   ` Eli Zaretskii
@ 2017-12-21  3:29   ` Simon Marchi
  2017-12-21 12:55     ` Stafford Horne
  1 sibling, 1 reply; 19+ messages in thread
From: Simon Marchi @ 2017-12-21  3:29 UTC (permalink / raw)
  To: openrisc

On 2017-12-19 09:22, 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:
> 
> 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/doc/ChangeLog:
> 
> 2017-06-06  Stafford Horne  <shorne@gmail.com>
> 
> 	* gdb.texinfo (Target Description Format): Explain that arbitrary
> 	strings are now allowed for register groups.
> ---
>  gdb/NEWS                             |  7 ++++
>  gdb/doc/gdb.texinfo                  |  9 +++--
>  gdb/target-descriptions.c            | 74 
> ++++++++++++++++++------------------
>  gdb/testsuite/gdb.xml/extra-regs.xml |  1 +
>  gdb/testsuite/gdb.xml/tdesc-regs.exp |  3 ++
>  5 files changed, 53 insertions(+), 41 deletions(-)
> 
> diff --git a/gdb/NEWS b/gdb/NEWS
> index 44f481d1f5..7d27262aee 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -3,6 +3,13 @@
> 
>  *** 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.
> +
> +* On Unix systems, GDBserver now does globbing expansion and variable
> +  substitution in inferior command line arguments.
> +
>  * 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 e169260e7e..a71faaa89c 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -41761,10 +41761,11 @@ 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
> - at 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.  The string should be limited to alphanumeric 
> characters
> +and internal hyphens.  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..1435f3f4cf 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 ()
> +      && (strcmp (reg->group.c_str (), reggroup_name (reggroup)) == 
> 0))

I suggest

   reg->group == reggroup_name (reggroup)

> +	return 1;
> 
>    if (reg != NULL
>        && (reggroup == save_reggroup || reggroup == restore_reggroup))
> @@ -1421,6 +1399,28 @@ 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 ())
> +	  {
> +	    struct reggroup *group;
> +	    bool group_exists = false;
> +
> +	    for (group = reggroup_next (gdbarch, NULL);
> +		 group != NULL;
> +		 group = reggroup_next (gdbarch, group))
> +	      {
> +		if (strcmp (reg->group.c_str (), reggroup_name (group)) == 0)

Here too.

> +		  {
> +		    group_exists = true;
> +		    break;
> +		  }
> +	      }
> +
> +	    if (!group_exists)
> +	      reggroup_add (gdbarch, reggroup_gdbarch_new (gdbarch,
> +							   reg->group.c_str (),
> +							   USER_REGGROUP));
> +	  }

Could you factor this out in a separate function?  It would probably be 
useful to have a general-purpose function

   reggroup *reggroup_find (struct gdbarch *gdbarch, const char *name);

Which you could then use here

   if (reggroup_find (...) == NULL)
     {
       // Create group
     }

Thanks,

Simon

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [OpenRISC] [PATCH v3 1/4] reggroups: Add test and docs for `info reg $reggroup` feature
  2017-12-21  2:40     ` Simon Marchi
@ 2017-12-21  3:40       ` Eli Zaretskii
  0 siblings, 0 replies; 19+ messages in thread
From: Eli Zaretskii @ 2017-12-21  3:40 UTC (permalink / raw)
  To: openrisc

> Date: Wed, 20 Dec 2017 21:40:10 -0500
> From: Simon Marchi <simon.marchi@polymtl.ca>
> Cc: Stafford Horne <shorne@gmail.com>, gdb-patches at sourceware.org,
>         openrisc at lists.librecores.org
> 
> >> + at code{maint print reggroups}.
> > 
> > Please add here a cross-reference to the node where "maint print
> > reggroups" is described.
> 
> Is it ok for a non-maint command to refer to a maint command?

Yes.

> AFAIK, we don't expect an average user to have to use maintenance
> commands when using GDB.  So maybe "maint print reggroups" should be
> promoted to a non-maint command (e.g. info register-groups)?

Registers are not for the average user anyway, so I don't see a
problem here.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [OpenRISC] [PATCH v3 4/4] tdesc: handle arbitrary strings in tdesc_register_in_reggroup_p
  2017-12-21  3:29   ` Simon Marchi
@ 2017-12-21 12:55     ` Stafford Horne
  0 siblings, 0 replies; 19+ messages in thread
From: Stafford Horne @ 2017-12-21 12:55 UTC (permalink / raw)
  To: openrisc

On Wed, Dec 20, 2017 at 10:29:31PM -0500, Simon Marchi wrote:
> On 2017-12-19 09:22, Stafford Horne wrote:
> > @@ -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 ()
> > +      && (strcmp (reg->group.c_str (), reggroup_name (reggroup)) == 0))
> 
> I suggest
> 
>   reg->group == reggroup_name (reggroup)

OK. Strange, I thought I tried that first but was having issues with std::string
vs (char *), but I just tried again and it works fine.

> > +	return 1;
> > 
> >    if (reg != NULL
> >        && (reggroup == save_reggroup || reggroup == restore_reggroup))
> > @@ -1421,6 +1399,28 @@ 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 ())
> > +	  {
> > +	    struct reggroup *group;
> > +	    bool group_exists = false;
> > +
> > +	    for (group = reggroup_next (gdbarch, NULL);
> > +		 group != NULL;
> > +		 group = reggroup_next (gdbarch, group))
> > +	      {
> > +		if (strcmp (reg->group.c_str (), reggroup_name (group)) == 0)
> 
> Here too.

OK.

> > +		  {
> > +		    group_exists = true;
> > +		    break;
> > +		  }
> > +	      }
> > +
> > +	    if (!group_exists)
> > +	      reggroup_add (gdbarch, reggroup_gdbarch_new (gdbarch,
> > +							   reg->group.c_str (),
> > +							   USER_REGGROUP));
> > +	  }
> 
> Could you factor this out in a separate function?  It would probably be
> useful to have a general-purpose function
> 
>   reggroup *reggroup_find (struct gdbarch *gdbarch, const char *name);
> 
> Which you could then use here
> 
>   if (reggroup_find (...) == NULL)
>     {
>       // Create group
>     }

Sure good point, if I find any places I can use it I will.

Thank You,

-Stafford

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [OpenRISC] [PATCH v3 1/4] reggroups: Add test and docs for `info reg $reggroup` feature
  2017-12-21  2:58   ` Simon Marchi
@ 2017-12-24 14:47     ` Stafford Horne
  0 siblings, 0 replies; 19+ messages in thread
From: Stafford Horne @ 2017-12-24 14:47 UTC (permalink / raw)
  To: openrisc

On Wed, Dec 20, 2017 at 09:58:45PM -0500, Simon Marchi wrote:
> On 2017-12-19 09:22, Stafford Horne wrote:
> > Until now this feature has existed but was not documented.  Adding docs
> > and tests.
> > 

...

> > +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 $" {
> 
> The .* should not be necessary here.

OK.

> > +	    if { [llength $reggroups] != 0 } {
> > +		pass $test
> > +	    } else {
> > +		fail "$test - didn't fetch any reggroups"
> > +	    }
> > +	}
> > +    }
> > +
> > +    return $reggroups
> > +}
> > +
> > +set reggroups [fetch_reggroups "fetch reggroups"]
> > +
> > +foreach reggroup $reggroups {
> > +    gdb_test "info reg $reggroup" ".*" "info reg $reggroup"
> > +}
> 
> This doesn't really test anything.  If you change the line to
> 
>   gdb_test "info reg hello$reggroup" ".*" "info reg $reggroup"
> 
> to fake that the command doesn't work, the test still passes.  So if
> something breaks the feature of "info registers" handling reg groups, the
> test won't catch it.  But I understand the problem, the output is not really
> predictable.  I thought about matching at least one $hex number, but it's
> not even guaranteed (some groups like mmx output nothing).
> 
> What we could do (I'm open to better suggestions) is at least validate that
> it doesn't output "Invalid register", which is the message given when
> passing a register that doesn't exist.  But then, if that message changes
> one day for some reason, the test will become moot again (because GDB will
> output something else than "Invalid register" if the functionality breaks,
> but the test won't catch it).  So in addition, we could also validate that
> "info registers a_non_existent_register" does output "Invalid register".
> This way, if some GDB developers of the future change the message, the test
> will fail, they will go look at your test file, read the comment that you
> will have left, and update the test accordingly.

Right, I will do this.   I will also try to match at least one $hex number
accross all reggroups.  Some will be blank, but if all are blank its probably an
issue.

-Stafford


^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2017-12-24 14:47 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-12-19 14:22 [OpenRISC] [PATCH v3 0/4] Support for arbitrary reggroups Stafford Horne
2017-12-19 14:22 ` [OpenRISC] [PATCH v3 1/4] reggroups: Add test and docs for `info reg $reggroup` feature Stafford Horne
2017-12-19 16:23   ` Eli Zaretskii
2017-12-20 10:40     ` Stafford Horne
2017-12-21  2:40     ` Simon Marchi
2017-12-21  3:40       ` Eli Zaretskii
2017-12-21  2:58   ` Simon Marchi
2017-12-24 14:47     ` Stafford Horne
2017-12-19 14:22 ` [OpenRISC] [PATCH v3 2/4] reggroups: Convert reggroups from post_init to pre_init Stafford Horne
2017-12-21  3:03   ` Simon Marchi
2017-12-19 14:22 ` [OpenRISC] [PATCH v3 3/4] reggroups: Create reggroup_gdbarch_new for dynamic reggroups Stafford Horne
2017-12-21  3:15   ` Simon Marchi
2017-12-19 14:22 ` [OpenRISC] [PATCH v3 4/4] tdesc: handle arbitrary strings in tdesc_register_in_reggroup_p Stafford Horne
2017-12-19 16:27   ` Eli Zaretskii
2017-12-19 22:13     ` Stafford Horne
2017-12-20 13:34       ` Stafford Horne
2017-12-20 15:53       ` Eli Zaretskii
2017-12-21  3:29   ` Simon Marchi
2017-12-21 12:55     ` Stafford Horne

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.