[PATCH 0/2] Improvements to 'tui reg' command.

[PATCH 0/2] Improvements to 'tui reg' command.

[Andrew Burgess]

This series is all about 'tui reg'.

Patch #1 adds 'tui reg prev', the oposite of 'tui reg next' for
rotating through the different register groups.

Patch #2 reworks 'tui reg', removing the fixed set of sub-commands,
and instead making 'tui reg' accept any valid register group.  The
user can now switch directly to any register group.  The command
completion has been updated to match and similarly offers up all valid
register groups.

Thanks,
Andrew

--

Andrew Burgess (2):
  gdb: Add 'tui reg prev' command.
  gdb: Rework command completion on 'tui reg'.

 gdb/ChangeLog       |  28 +++++++++++
 gdb/completer.c     |  57 ++++++++++++++--------
 gdb/completer.h     |   3 ++
 gdb/doc/ChangeLog   |   4 ++
 gdb/doc/gdb.texinfo |   6 +++
 gdb/reggroups.c     |  30 ++++++++++++
 gdb/reggroups.h     |   9 ++--
 gdb/tui/tui-regs.c  | 138 ++++++++++++++++++++++++++++++++++++++--------------
 8 files changed, 217 insertions(+), 58 deletions(-)

-- 
2.4.0

[PATCH 2/2] gdb: Rework command completion on 'tui reg'.

[Andrew Burgess]

We previously specified a few known register groups for the 'tui reg'
command.  Other register groups could be accessed, but only by using the
'tui reg next' command and cycling through all the groups.

This commit removes the hard coded sub-commands of 'tui reg' and instead
adds dynamic completion of sub-commands based on the architecturally
defined register groups, giving immediate access to all available
register groups.

There is still the 'next' and 'prev' commands for cycling through the
register groups if that's wanted.

gdb/ChangeLog:

	* completer.c (reg_or_group_completer_1): New function containing
	old reg_or_group_completer.
	(reg_or_group_completer): Call new reg_or_group_completer_1.
	(reggroup_completer): Call new reg_or_group_completer_1.
	* completer.h (reggroup_completer): Add declaration.
	* tui/tui-regs.c: Add 'completer.h' include.
	(tui_reg_next_command): Renamed to...
	(tui_reg_name): ...this.  Adjust parameters.
	(tui_reg_prev_command): Renamed to...
	(tui_reg_prev): ...this.  Adjust parameters.
	(tui_reg_float_command): Delete.
	(tui_reg_general_command): Delete.
	(tui_reg_system_command): Delete.
	(tui_reg_command): Rewrite to perform switching of register group.
	(tuireglist): Remove.
	(tui_reggroup_completer): New function.
	(_initialize_tui_regs): Remove 'tui reg' sub-commands, update
	creation of 'tui reg' command.
---
 gdb/ChangeLog      |  21 +++++++++
 gdb/completer.c    |  57 ++++++++++++++++---------
 gdb/completer.h    |   3 ++
 gdb/tui/tui-regs.c | 123 +++++++++++++++++++++++++++++++++--------------------
 4 files changed, 140 insertions(+), 64 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index b19e3ca..5ee18d0 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,26 @@
 2015-05-21  Andrew Burgess  <andrew.burgess@embecosm.com>
 
+	* completer.c (reg_or_group_completer_1): New function containing
+	old reg_or_group_completer.
+	(reg_or_group_completer): Call new reg_or_group_completer_1.
+	(reggroup_completer): Call new reg_or_group_completer_1.
+	* completer.h (reggroup_completer): Add declaration.
+	* tui/tui-regs.c: Add 'completer.h' include.
+	(tui_reg_next_command): Renamed to...
+	(tui_reg_name): ...this.  Adjust parameters.
+	(tui_reg_prev_command): Renamed to...
+	(tui_reg_prev): ...this.  Adjust parameters.
+	(tui_reg_float_command): Delete.
+	(tui_reg_general_command): Delete.
+	(tui_reg_system_command): Delete.
+	(tui_reg_command): Rewrite to perform switching of register group.
+	(tuireglist): Remove.
+	(tui_reggroup_completer): New function.
+	(_initialize_tui_regs): Remove 'tui reg' sub-commands, update
+	creation of 'tui reg' command.
+
+2015-05-21  Andrew Burgess  <andrew.burgess@embecosm.com>
+
 	* tui/tui-regs.c (tui_reg_prev_command): New function.
 	(_initialize_tui_regs): Add 'prev' command for 'tui reg'.
 	* reggroups.c (reggroup_prev): New function.
diff --git a/gdb/completer.c b/gdb/completer.c
index c8c0e4c..73f734e 100644
--- a/gdb/completer.c
+++ b/gdb/completer.c
@@ -971,9 +971,10 @@ signal_completer (struct cmd_list_element *ignore,
 
 /* Complete on a register or reggroup.  */
 
-VEC (char_ptr) *
-reg_or_group_completer (struct cmd_list_element *ignore,
-			const char *text, const char *word)
+static VEC (char_ptr) *
+reg_or_group_completer_1 (struct cmd_list_element *ignore,
+			  const char *text, const char *word,
+			  int add_registers, int add_reggroups)
 {
   VEC (char_ptr) *result = NULL;
   size_t len = strlen (word);
@@ -985,28 +986,46 @@ reg_or_group_completer (struct cmd_list_element *ignore,
   if (!target_has_registers)
     return result;
 
+  gdb_assert (add_registers || add_reggroups);
   gdbarch = get_frame_arch (get_selected_frame (NULL));
 
-  for (i = 0;
-       (name = user_reg_map_regnum_to_name (gdbarch, i)) != NULL;
-       i++)
-    {
-      if (*name != '\0' && strncmp (word, name, len) == 0)
-	VEC_safe_push (char_ptr, result, xstrdup (name));
-    }
-
-  for (group = reggroup_next (gdbarch, NULL);
-       group != NULL;
-       group = reggroup_next (gdbarch, group))
-    {
-      name = reggroup_name (group);
-      if (strncmp (word, name, len) == 0)
-	VEC_safe_push (char_ptr, result, xstrdup (name));
-    }
+  if (add_registers)
+    for (i = 0;
+	 (name = user_reg_map_regnum_to_name (gdbarch, i)) != NULL;
+	 i++)
+      {
+	if (*name != '\0' && strncmp (word, name, len) == 0)
+	  VEC_safe_push (char_ptr, result, xstrdup (name));
+      }
+
+  if (add_reggroups)
+    for (group = reggroup_next (gdbarch, NULL);
+	 group != NULL;
+	 group = reggroup_next (gdbarch, group))
+      {
+	name = reggroup_name (group);
+	if (strncmp (word, name, len) == 0)
+	  VEC_safe_push (char_ptr, result, xstrdup (name));
+      }
 
   return result;
 }
 
+VEC (char_ptr) *
+reg_or_group_completer (struct cmd_list_element *ignore,
+			const char *text, const char *word)
+{
+  /* Complete on both registers and reggroups.  */
+  return reg_or_group_completer_1 (ignore, text, word, 1, 1);
+}
+
+VEC (char_ptr) *
+reggroup_completer (struct cmd_list_element *ignore,
+		    const char *text, const char *word)
+{
+  /* Complete on reggroups only.  */
+  return reg_or_group_completer_1 (ignore, text, word, 0, 1);
+}
 
 /* Get the list of chars that are considered as word breaks
    for the current command.  */
diff --git a/gdb/completer.h b/gdb/completer.h
index 56e1a2b..6c1f257 100644
--- a/gdb/completer.h
+++ b/gdb/completer.h
@@ -96,6 +96,9 @@ extern VEC (char_ptr) *signal_completer (struct cmd_list_element *,
 extern VEC (char_ptr) *reg_or_group_completer (struct cmd_list_element *,
 					       const char *, const char *);
 
+extern VEC (char_ptr) *reggroup_completer (struct cmd_list_element *,
+					   const char *, const char *);
+
 extern char *get_gdb_completer_quote_characters (void);
 
 extern char *gdb_completion_word_break_characters (void);
diff --git a/gdb/tui/tui-regs.c b/gdb/tui/tui-regs.c
index 8dbcda1..685a896 100644
--- a/gdb/tui/tui-regs.c
+++ b/gdb/tui/tui-regs.c
@@ -39,6 +39,7 @@
 #include "tui/tui-io.h"
 #include "reggroups.h"
 #include "valprint.h"
+#include "completer.h"
 
 #include "gdb_curses.h"
 
@@ -557,10 +558,8 @@ tui_display_register (struct tui_data_element *data,
 }
 
 static void
-tui_reg_next_command (char *arg, int from_tty)
+tui_reg_next (struct gdbarch *gdbarch)
 {
-  struct gdbarch *gdbarch = get_current_arch ();
-
   if (TUI_DATA_WIN != NULL)
     {
       struct reggroup *group
@@ -576,10 +575,8 @@ tui_reg_next_command (char *arg, int from_tty)
 }
 
 static void
-tui_reg_prev_command (char *arg, int from_tty)
+tui_reg_prev (struct gdbarch *gdbarch)
 {
-  struct gdbarch *gdbarch = get_current_arch ();
-
   if (TUI_DATA_WIN != NULL)
     {
       struct reggroup *group
@@ -606,31 +603,84 @@ tui_reg_prev_command (char *arg, int from_tty)
 }
 
 static void
-tui_reg_float_command (char *arg, int from_tty)
+tui_reg_command (char *args, int from_tty)
 {
-  tui_show_registers (float_reggroup);
-}
+  struct gdbarch *gdbarch = get_current_arch ();
 
-static void
-tui_reg_general_command (char *arg, int from_tty)
-{
-  tui_show_registers (general_reggroup);
-}
+  if (args)
+    {
+      if (strcmp (args, "next") == 0)
+	tui_reg_next (gdbarch);
+      else if (strcmp (args, "prev") == 0)
+	tui_reg_prev (gdbarch);
+      else
+	{
+	  struct reggroup *group = NULL, *match = NULL;
+	  size_t len = strlen (args);
 
-static void
-tui_reg_system_command (char *arg, int from_tty)
-{
-  tui_show_registers (system_reggroup);
+	  for (group = reggroup_next (gdbarch, group);
+	       group != NULL;
+	       group = reggroup_next (gdbarch, group))
+	    {
+	      if (strncmp (reggroup_name (group), args, len) == 0)
+		{
+		  if (match != NULL)
+		    {
+		      match = NULL;
+		      break;
+		    }
+		  match = group;
+		}
+	    }
+
+	  if (match != NULL)
+	    tui_show_registers (match);
+	  else
+	    error (_("unknown register group '%s'"), args);
+	}
+    }
+  else
+    {
+      struct reggroup *group = NULL;
+
+      printf_unfiltered (_("\"tui reg\" must be followed by the name of "
+			   "either a register group,\nor one of 'next' "
+			   "or 'prev'.  Known register groups are:\n"));
+      for (group = reggroup_next (gdbarch, group); group != NULL; )
+	{
+	  printf_unfiltered ("%s", reggroup_name (group));
+	  group = reggroup_next (gdbarch, group);
+	  if (group != NULL)
+	    printf_unfiltered (", ");
+	}
+      printf_unfiltered ("\n");
+    }
 }
 
-static struct cmd_list_element *tuireglist;
+/* Complete names of register groups, and add the special "prev" and "next"
+   names.  */
 
-static void
-tui_reg_command (char *args, int from_tty)
+static VEC (char_ptr) *
+tui_reggroup_completer (struct cmd_list_element *ignore,
+			const char *text, const char *word)
 {
-  printf_unfiltered (_("\"tui reg\" must be followed by the name of a "
-                     "tui reg command.\n"));
-  help_list (tuireglist, "tui reg ", all_commands, gdb_stdout);
+  VEC (char_ptr) *result = NULL;
+  static const char *extra[] = { "next", "prev", NULL };
+  size_t len = strlen (word);
+  const char **tmp;
+
+  if (!target_has_registers)
+    return result;
+
+  result = reggroup_completer (ignore, text, word);
+
+  for (tmp = extra; *tmp; ++tmp)
+    {
+      if (strncmp (word, *tmp, len) == 0)
+	VEC_safe_push (char_ptr, result, xstrdup (*tmp));
+    }
+
+  return result;
 }
 
 /* Provide a prototype to silence -Wmissing-prototypes.  */
@@ -639,30 +689,13 @@ extern initialize_file_ftype _initialize_tui_regs;
 void
 _initialize_tui_regs (void)
 {
-  struct cmd_list_element **tuicmd;
+  struct cmd_list_element **tuicmd, *cmd;
 
   tuicmd = tui_get_cmd_list ();
 
-  add_prefix_cmd ("reg", class_tui, tui_reg_command,
-                  _("TUI commands to control the register window."),
-                  &tuireglist, "tui reg ", 0,
-                  tuicmd);
-
-  add_cmd ("float", class_tui, tui_reg_float_command,
-           _("Display only floating point registers."),
-           &tuireglist);
-  add_cmd ("general", class_tui, tui_reg_general_command,
-           _("Display only general registers."),
-           &tuireglist);
-  add_cmd ("system", class_tui, tui_reg_system_command,
-           _("Display only system registers."),
-           &tuireglist);
-  add_cmd ("next", class_tui, tui_reg_next_command,
-           _("Display next register group."),
-           &tuireglist);
-  add_cmd ("prev", class_tui, tui_reg_prev_command,
-           _("Display previous register group."),
-           &tuireglist);
+  cmd = add_cmd ("reg", class_tui, tui_reg_command, _("\
+TUI command to control the register window."), tuicmd);
+  set_cmd_completer (cmd, tui_reggroup_completer);
 }
 
 
-- 
2.4.0

[PATCH 1/2] gdb: Add 'tui reg prev' command.

[Andrew Burgess]

There is already a 'tui reg next' command, this adds a symmetric 'tui
reg prev' command.

gdb/ChangeLog:

	* tui/tui-regs.c (tui_reg_prev_command): New function.
	(_initialize_tui_regs): Add 'prev' command for 'tui reg'.
	* reggroups.c (reggroup_prev): New function.
	* reggroups.h (reggroup_prev): Add declaration.  Update comment.

gdb/doc/ChangeLog:

	* gdb.texinfo (TUI Commands): Add 'tui reg prev' details.
---
 gdb/ChangeLog       |  7 +++++++
 gdb/doc/ChangeLog   |  4 ++++
 gdb/doc/gdb.texinfo |  6 ++++++
 gdb/reggroups.c     | 30 ++++++++++++++++++++++++++++++
 gdb/reggroups.h     |  9 ++++++---
 gdb/tui/tui-regs.c  | 33 +++++++++++++++++++++++++++++++++
 6 files changed, 86 insertions(+), 3 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 42ef67d..b19e3ca 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,12 @@
 2015-05-21  Andrew Burgess  <andrew.burgess@embecosm.com>
 
+	* tui/tui-regs.c (tui_reg_prev_command): New function.
+	(_initialize_tui_regs): Add 'prev' command for 'tui reg'.
+	* reggroups.c (reggroup_prev): New function.
+	* reggroups.h (reggroup_prev): Add declaration.  Update comment.
+
+2015-05-21  Andrew Burgess  <andrew.burgess@embecosm.com>
+
 	* tui/tui-regs.c (tui_reg_next_command): Use NULL not 0.
 
 2015-05-21  Andrew Burgess  <andrew.burgess@embecosm.com>
diff --git a/gdb/doc/ChangeLog b/gdb/doc/ChangeLog
index f8b0487..ef38740 100644
--- a/gdb/doc/ChangeLog
+++ b/gdb/doc/ChangeLog
@@ -1,3 +1,7 @@
+2015-05-21  Andrew Burgess  <andrew.burgess@embecosm.com>
+
+	* gdb.texinfo (TUI Commands): Add 'tui reg prev' details.
+
 2015-05-16  Doug Evans  <xdje42@gmail.com>
 
 	* guile.texi (Memory Ports in Guile): Document support for unbuffered
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 1665372..44dff6e 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -25003,6 +25003,12 @@ their order is target specific.  The predefined register groups are the
 following: @code{general}, @code{float}, @code{system}, @code{vector},
 @code{all}, @code{save}, @code{restore}.
 
+@item tui reg prev
+Show the previous register group.  The list of register groups as well
+as their order is target specific.  The predefined register groups are
+the following: @code{general}, @code{float}, @code{system},
+@code{vector}, @code{all}, @code{save}, @code{restore}.
+
 @item tui reg system
 Show the system registers in the register window.
 
diff --git a/gdb/reggroups.c b/gdb/reggroups.c
index cbafc01..27be704 100644
--- a/gdb/reggroups.c
+++ b/gdb/reggroups.c
@@ -150,6 +150,36 @@ reggroup_next (struct gdbarch *gdbarch, struct reggroup *last)
   return NULL;
 }
 
+struct reggroup *
+reggroup_prev (struct gdbarch *gdbarch, struct reggroup *last)
+{
+  struct reggroups *groups;
+  struct reggroup_el *el;
+  struct reggroup *prev;
+
+  /* Don't allow this function to be called during architecture
+     creation.  If there are no groups, use the default groups list.  */
+  groups = gdbarch_data (gdbarch, reggroups_data);
+  gdb_assert (groups != NULL);
+  if (groups->first == NULL)
+    {
+      groups = &default_groups;
+      gdb_assert (groups->first != NULL);
+    }
+
+  /* Return the first/next reggroup.  */
+  if (last == NULL)
+    return groups->first->group;
+  prev = NULL;
+  for (el = groups->first; el != NULL; el = el->next)
+    {
+      if (el->group == last)
+	return prev;
+      prev = el->group;
+    }
+  return NULL;
+}
+
 /* Is REGNUM a member of REGGROUP?  */
 int
 default_register_reggroup_p (struct gdbarch *gdbarch, int regnum,
diff --git a/gdb/reggroups.h b/gdb/reggroups.h
index 2ad74bc..bb0d6e7 100644
--- a/gdb/reggroups.h
+++ b/gdb/reggroups.h
@@ -49,11 +49,14 @@ extern void reggroup_add (struct gdbarch *gdbarch, struct reggroup *group);
 extern const char *reggroup_name (struct reggroup *reggroup);
 extern enum reggroup_type reggroup_type (struct reggroup *reggroup);
 
-/* Interator for the architecture's register groups.  Pass in NULL,
-   returns the first group.  Pass in a group, returns the next group,
-   or NULL when the last group is reached.  */
+/* Iterators for the architecture's register groups.  Pass in NULL, returns
+   the first group.  Pass in a group, returns the next or previous group,
+   or NULL when either the end or the beginning of the group list is
+   reached.  */
 extern struct reggroup *reggroup_next (struct gdbarch *gdbarch,
 				       struct reggroup *last);
+extern struct reggroup *reggroup_prev (struct gdbarch *gdbarch,
+				       struct reggroup *last);
 
 /* Is REGNUM a member of REGGROUP?  */
 extern int default_register_reggroup_p (struct gdbarch *gdbarch, int regnum,
diff --git a/gdb/tui/tui-regs.c b/gdb/tui/tui-regs.c
index 8d4c0f8..8dbcda1 100644
--- a/gdb/tui/tui-regs.c
+++ b/gdb/tui/tui-regs.c
@@ -576,6 +576,36 @@ tui_reg_next_command (char *arg, int from_tty)
 }
 
 static void
+tui_reg_prev_command (char *arg, int from_tty)
+{
+  struct gdbarch *gdbarch = get_current_arch ();
+
+  if (TUI_DATA_WIN != NULL)
+    {
+      struct reggroup *group
+        = TUI_DATA_WIN->detail.data_display_info.current_group;
+
+      group = reggroup_prev (gdbarch, group);
+      if (group == NULL)
+	{
+	  struct reggroup *prev, *next;
+
+	  next = reggroup_next (gdbarch, NULL);
+	  prev = NULL;
+	  while (next)
+	    {
+	      prev = next;
+	      next = reggroup_next (gdbarch, next);
+	    }
+	  group = prev;
+	}
+
+      if (group)
+        tui_show_registers (group);
+    }
+}
+
+static void
 tui_reg_float_command (char *arg, int from_tty)
 {
   tui_show_registers (float_reggroup);
@@ -630,6 +660,9 @@ _initialize_tui_regs (void)
   add_cmd ("next", class_tui, tui_reg_next_command,
            _("Display next register group."),
            &tuireglist);
+  add_cmd ("prev", class_tui, tui_reg_prev_command,
+           _("Display previous register group."),
+           &tuireglist);
 }
 
 
-- 
2.4.0

Re: [PATCH 1/2] gdb: Add 'tui reg prev' command.

[Pedro Alves]

On 05/21/2015 11:17 PM, Andrew Burgess wrote:
> There is already a 'tui reg next' command, this adds a symmetric 'tui
> reg prev' command.

Thanks!

> 
> gdb/ChangeLog:
> 
> 	* tui/tui-regs.c (tui_reg_prev_command): New function.
> 	(_initialize_tui_regs): Add 'prev' command for 'tui reg'.
> 	* reggroups.c (reggroup_prev): New function.
> 	* reggroups.h (reggroup_prev): Add declaration.  Update comment.
> 
> gdb/doc/ChangeLog:
> 
> 	* gdb.texinfo (TUI Commands): Add 'tui reg prev' details.
> ---
>  gdb/ChangeLog       |  7 +++++++
>  gdb/doc/ChangeLog   |  4 ++++
>  gdb/doc/gdb.texinfo |  6 ++++++
>  gdb/reggroups.c     | 30 ++++++++++++++++++++++++++++++
>  gdb/reggroups.h     |  9 ++++++---
>  gdb/tui/tui-regs.c  | 33 +++++++++++++++++++++++++++++++++
>  6 files changed, 86 insertions(+), 3 deletions(-)
> 
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 42ef67d..b19e3ca 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,5 +1,12 @@
>  2015-05-21  Andrew Burgess  <andrew.burgess@embecosm.com>
>  
> +	* tui/tui-regs.c (tui_reg_prev_command): New function.
> +	(_initialize_tui_regs): Add 'prev' command for 'tui reg'.
> +	* reggroups.c (reggroup_prev): New function.
> +	* reggroups.h (reggroup_prev): Add declaration.  Update comment.
> +
> +2015-05-21  Andrew Burgess  <andrew.burgess@embecosm.com>
> +
>  	* tui/tui-regs.c (tui_reg_next_command): Use NULL not 0.
>  
>  2015-05-21  Andrew Burgess  <andrew.burgess@embecosm.com>
> diff --git a/gdb/doc/ChangeLog b/gdb/doc/ChangeLog
> index f8b0487..ef38740 100644
> --- a/gdb/doc/ChangeLog
> +++ b/gdb/doc/ChangeLog
> @@ -1,3 +1,7 @@
> +2015-05-21  Andrew Burgess  <andrew.burgess@embecosm.com>
> +
> +	* gdb.texinfo (TUI Commands): Add 'tui reg prev' details.
> +
>  2015-05-16  Doug Evans  <xdje42@gmail.com>
>  
>  	* guile.texi (Memory Ports in Guile): Document support for unbuffered
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index 1665372..44dff6e 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -25003,6 +25003,12 @@ their order is target specific.  The predefined register groups are the
>  following: @code{general}, @code{float}, @code{system}, @code{vector},
>  @code{all}, @code{save}, @code{restore}.
>  
> +@item tui reg prev
> +Show the previous register group.  The list of register groups as well
> +as their order is target specific.  The predefined register groups are
> +the following: @code{general}, @code{float}, @code{system},
> +@code{vector}, @code{all}, @code{save}, @code{restore}.
> +
>  @item tui reg system
>  Show the system registers in the register window.
>  
> diff --git a/gdb/reggroups.c b/gdb/reggroups.c
> index cbafc01..27be704 100644
> --- a/gdb/reggroups.c
> +++ b/gdb/reggroups.c
> @@ -150,6 +150,36 @@ reggroup_next (struct gdbarch *gdbarch, struct reggroup *last)
>    return NULL;
>  }
>  

/* See reggroups.h.  */

> +struct reggroup *
> +reggroup_prev (struct gdbarch *gdbarch, struct reggroup *last)
> +{

...


>  
>  static void
> +tui_reg_prev_command (char *arg, int from_tty)

Misses intro comment.  E.g.: "Implementation of "tui reg prev" command".

> +{
> +  struct gdbarch *gdbarch = get_current_arch ();
> +
> +  if (TUI_DATA_WIN != NULL)
> +    {
> +      struct reggroup *group
> +        = TUI_DATA_WIN->detail.data_display_info.current_group;
> +
> +      group = reggroup_prev (gdbarch, group);
> +      if (group == NULL)
> +	{
> +	  struct reggroup *prev, *next;
> +
> +	  next = reggroup_next (gdbarch, NULL);
> +	  prev = NULL;
> +	  while (next)
> +	    {
> +	      prev = next;
> +	      next = reggroup_next (gdbarch, next);
> +	    }
> +	  group = prev;
> +	}

Hmm, this is surprising, and duplicating what reggroup_prev
should itself be doing.  I think this here should just be
(just like the "tui reg next" mirror):

      group = reggroup_prev (gdbarch, group);
      if (group == NULL)
        group = reggroup_prev (gdbarch, NULL);
      if (group != NULL)
        tui_show_registers (group);

That is, like the "next" iterator is circular, so should
the "prev" one be.

If that doesn't work, seems to me that the reggroup_prev
iterator is broken for not being symmetrical with
reggroup_next.

> +struct reggroup *
> +reggroup_prev (struct gdbarch *gdbarch, struct reggroup *last)
> +{
> +  struct reggroups *groups;
> +  struct reggroup_el *el;
> +  struct reggroup *prev;
> +
> +  /* Don't allow this function to be called during architecture
> +     creation.  If there are no groups, use the default groups list.  */
> +  groups = gdbarch_data (gdbarch, reggroups_data);
> +  gdb_assert (groups != NULL);
> +  if (groups->first == NULL)
> +    {
> +      groups = &default_groups;
> +      gdb_assert (groups->first != NULL);
> +    }
> +
> +  /* Return the first/next reggroup.  */
> +  if (last == NULL)
> +    return groups->first->group;

Isn't this the problem here?

"Return the first/next reggroup" makes sense for
reggroups_next, but here I'd expect (renaming
the "last" parameter to avoid ambiguity):

reggroup_prev (struct gdbarch *gdbarch, struct reggroup *iter)
{
...
  /* Return the last/prev reggroup.  */
  if (iter == groups->first)
    return NULL;
  prev = NULL;
  for (el = groups->first; el != NULL; el = el->next)
    {
      if (el->group == iter)
	return prev;
      prev = el->group;
    }
  return NULL;

That is, if iter==NULL, return the last group, otherwise
return the previous.  I may have easily missed something though.

Thanks,
Pedro Alves

Re: [PATCH 1/2] gdb: Add 'tui reg prev' command.

[Eli Zaretskii]

> From: Andrew Burgess <andrew.burgess@embecosm.com>
> Cc: Andrew Burgess <andrew.burgess@embecosm.com>
> Date: Fri, 22 May 2015 00:17:11 +0200
> 
> There is already a 'tui reg next' command, this adds a symmetric 'tui
> reg prev' command.
> 
> gdb/ChangeLog:
> 
> 	* tui/tui-regs.c (tui_reg_prev_command): New function.
> 	(_initialize_tui_regs): Add 'prev' command for 'tui reg'.
> 	* reggroups.c (reggroup_prev): New function.
> 	* reggroups.h (reggroup_prev): Add declaration.  Update comment.
> 
> gdb/doc/ChangeLog:
> 
> 	* gdb.texinfo (TUI Commands): Add 'tui reg prev' details.

Thanks, the documentation parts are OK.

Re: [PATCH 1/2] gdb: Add 'tui reg prev' command.

[Andrew Burgess]

* Pedro Alves <palves@redhat.com> [2015-05-22 02:06:12 +0100]:

> On 05/21/2015 11:17 PM, Andrew Burgess wrote:
> > There is already a 'tui reg next' command, this adds a symmetric 'tui
> > reg prev' command.
> 
> Thanks!

New version.  I've addressed your comments from the first review.  The
reggroup_prev iterator has been fixed along the lines you suggested.

Thanks,
Andrew

--

gdb: Add 'tui reg prev' command.

There is already a 'tui reg next' command, this adds a symmetric 'tui
reg prev' command.

gdb/ChangeLog:

	* tui/tui-regs.c (tui_reg_prev_command): New function.
	(_initialize_tui_regs): Add 'prev' command for 'tui reg'.
	* reggroups.c (reggroup_prev): New function.
	* reggroups.h (reggroup_prev): Add declaration.  Update comment.

gdb/doc/ChangeLog:

	* gdb.texinfo (TUI Commands): Add 'tui reg prev' details.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 42ef67d..51cacf0 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,10 @@
+2015-05-22  Andrew Burgess  <andrew.burgess@embecosm.com>
+
+	* tui/tui-regs.c (tui_reg_prev_command): New function.
+	(_initialize_tui_regs): Add 'prev' command for 'tui reg'.
+	* reggroups.c (reggroup_prev): New function.
+	* reggroups.h (reggroup_prev): Add declaration.  Update comment.
+
 2015-05-21  Andrew Burgess  <andrew.burgess@embecosm.com>
 
 	* tui/tui-regs.c (tui_reg_next_command): Use NULL not 0.
diff --git a/gdb/doc/ChangeLog b/gdb/doc/ChangeLog
index f8b0487..7842ef5 100644
--- a/gdb/doc/ChangeLog
+++ b/gdb/doc/ChangeLog
@@ -1,3 +1,7 @@
+2015-05-22  Andrew Burgess  <andrew.burgess@embecosm.com>
+
+	* gdb.texinfo (TUI Commands): Add 'tui reg prev' details.
+
 2015-05-16  Doug Evans  <xdje42@gmail.com>
 
 	* guile.texi (Memory Ports in Guile): Document support for unbuffered
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 1665372..44dff6e 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -25003,6 +25003,12 @@ their order is target specific.  The predefined register groups are the
 following: @code{general}, @code{float}, @code{system}, @code{vector},
 @code{all}, @code{save}, @code{restore}.
 
+@item tui reg prev
+Show the previous register group.  The list of register groups as well
+as their order is target specific.  The predefined register groups are
+the following: @code{general}, @code{float}, @code{system},
+@code{vector}, @code{all}, @code{save}, @code{restore}.
+
 @item tui reg system
 Show the system registers in the register window.
 
diff --git a/gdb/reggroups.c b/gdb/reggroups.c
index cbafc01..745c5ea 100644
--- a/gdb/reggroups.c
+++ b/gdb/reggroups.c
@@ -150,6 +150,35 @@ reggroup_next (struct gdbarch *gdbarch, struct reggroup *last)
   return NULL;
 }
 
+/* See reggroups.h.  */
+
+struct reggroup *
+reggroup_prev (struct gdbarch *gdbarch, struct reggroup *curr)
+{
+  struct reggroups *groups;
+  struct reggroup_el *el;
+  struct reggroup *prev;
+
+  /* Don't allow this function to be called during architecture
+     creation.  If there are no groups, use the default groups list.  */
+  groups = gdbarch_data (gdbarch, reggroups_data);
+  gdb_assert (groups != NULL);
+  if (groups->first == NULL)
+    groups = &default_groups;
+
+  prev = NULL;
+  for (el = groups->first; el != NULL; el = el->next)
+    {
+      gdb_assert (el->group != NULL);
+      if (el->group == curr)
+	return prev;
+      prev = el->group;
+    }
+  if (curr == NULL)
+    return prev;
+  return NULL;
+}
+
 /* Is REGNUM a member of REGGROUP?  */
 int
 default_register_reggroup_p (struct gdbarch *gdbarch, int regnum,
diff --git a/gdb/reggroups.h b/gdb/reggroups.h
index 2ad74bc..425a25c 100644
--- a/gdb/reggroups.h
+++ b/gdb/reggroups.h
@@ -49,11 +49,14 @@ extern void reggroup_add (struct gdbarch *gdbarch, struct reggroup *group);
 extern const char *reggroup_name (struct reggroup *reggroup);
 extern enum reggroup_type reggroup_type (struct reggroup *reggroup);
 
-/* Interator for the architecture's register groups.  Pass in NULL,
-   returns the first group.  Pass in a group, returns the next group,
-   or NULL when the last group is reached.  */
+/* Iterators for the architecture's register groups.  Pass in NULL, returns
+   the first (for next), or last (for prev) group.  Pass in a group,
+   returns the next or previous group, or NULL when either the end or the
+   beginning of the group list is reached.  */
 extern struct reggroup *reggroup_next (struct gdbarch *gdbarch,
 				       struct reggroup *last);
+extern struct reggroup *reggroup_prev (struct gdbarch *gdbarch,
+				       struct reggroup *curr);
 
 /* Is REGNUM a member of REGGROUP?  */
 extern int default_register_reggroup_p (struct gdbarch *gdbarch, int regnum,
diff --git a/gdb/tui/tui-regs.c b/gdb/tui/tui-regs.c
index 8d4c0f8..62ecaf0 100644
--- a/gdb/tui/tui-regs.c
+++ b/gdb/tui/tui-regs.c
@@ -575,6 +575,29 @@ tui_reg_next_command (char *arg, int from_tty)
     }
 }
 
+/* Implementation of the "tui reg prev" command.  Cycle the register group
+   displayed in the tui REG window, moving backwards through the list of
+   available register groups.  */
+
+static void
+tui_reg_prev_command (char *arg, int from_tty)
+{
+  struct gdbarch *gdbarch = get_current_arch ();
+
+  if (TUI_DATA_WIN != NULL)
+    {
+      struct reggroup *group
+        = TUI_DATA_WIN->detail.data_display_info.current_group;
+
+      group = reggroup_prev (gdbarch, group);
+      if (group == NULL)
+	group = reggroup_prev (gdbarch, NULL);
+
+      if (group)
+        tui_show_registers (group);
+    }
+}
+
 static void
 tui_reg_float_command (char *arg, int from_tty)
 {
@@ -630,6 +653,9 @@ _initialize_tui_regs (void)
   add_cmd ("next", class_tui, tui_reg_next_command,
            _("Display next register group."),
            &tuireglist);
+  add_cmd ("prev", class_tui, tui_reg_prev_command,
+           _("Display previous register group."),
+           &tuireglist);
 }
 
 

Re: [PATCH 1/2] gdb: Add 'tui reg prev' command.

[Pedro Alves]

On 05/22/2015 05:37 PM, Andrew Burgess wrote:

> New version.  I've addressed your comments from the first review.  The
> reggroup_prev iterator has been fixed along the lines you suggested.
> 

Great, thanks.

> +/* Implementation of the "tui reg prev" command.  Cycle the register group
> +   displayed in the tui REG window, moving backwards through the list of
> +   available register groups.  */
> +
> +static void
> +tui_reg_prev_command (char *arg, int from_tty)
> +{
> +  struct gdbarch *gdbarch = get_current_arch ();
> +
> +  if (TUI_DATA_WIN != NULL)
> +    {
> +      struct reggroup *group
> +        = TUI_DATA_WIN->detail.data_display_info.current_group;
> +
> +      group = reggroup_prev (gdbarch, group);
> +      if (group == NULL)
> +	group = reggroup_prev (gdbarch, NULL);
> +
> +      if (group)

group != NULL.

> +        tui_show_registers (group);

Indentation looks odd here.

Otherwise OK.

Thanks,
Pedro Alves

Re: [PATCH 2/2] gdb: Rework command completion on 'tui reg'.

[Pedro Alves]

On 05/21/2015 11:17 PM, Andrew Burgess wrote:
> We previously specified a few known register groups for the 'tui reg'
> command.  Other register groups could be accessed, but only by using the
> 'tui reg next' command and cycling through all the groups.
> 
> This commit removes the hard coded sub-commands of 'tui reg' and instead
> adds dynamic completion of sub-commands based on the architecturally
> defined register groups, giving immediate access to all available
> register groups.
> 
> There is still the 'next' and 'prev' commands for cycling through the
> register groups if that's wanted.

I think this deserves a news entry, being a user-visible change.

> gdb/ChangeLog:
> 
> 	* completer.c (reg_or_group_completer_1): New function containing
> 	old reg_or_group_completer.
> 	(reg_or_group_completer): Call new reg_or_group_completer_1.
> 	(reggroup_completer): Call new reg_or_group_completer_1.
> 	* completer.h (reggroup_completer): Add declaration.
> 	* tui/tui-regs.c: Add 'completer.h' include.
> 	(tui_reg_next_command): Renamed to...
> 	(tui_reg_name): ...this.  Adjust parameters.
> 	(tui_reg_prev_command): Renamed to...
> 	(tui_reg_prev): ...this.  Adjust parameters.
> 	(tui_reg_float_command): Delete.
> 	(tui_reg_general_command): Delete.
> 	(tui_reg_system_command): Delete.
> 	(tui_reg_command): Rewrite to perform switching of register group.
> 	(tuireglist): Remove.
> 	(tui_reggroup_completer): New function.
> 	(_initialize_tui_regs): Remove 'tui reg' sub-commands, update
> 	creation of 'tui reg' command.
> ---
>  gdb/ChangeLog      |  21 +++++++++
>  gdb/completer.c    |  57 ++++++++++++++++---------
>  gdb/completer.h    |   3 ++
>  gdb/tui/tui-regs.c | 123 +++++++++++++++++++++++++++++++++--------------------
>  4 files changed, 140 insertions(+), 64 deletions(-)
> 
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index b19e3ca..5ee18d0 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,5 +1,26 @@
>  2015-05-21  Andrew Burgess  <andrew.burgess@embecosm.com>
>  
> +	* completer.c (reg_or_group_completer_1): New function containing
> +	old reg_or_group_completer.
> +	(reg_or_group_completer): Call new reg_or_group_completer_1.
> +	(reggroup_completer): Call new reg_or_group_completer_1.
> +	* completer.h (reggroup_completer): Add declaration.
> +	* tui/tui-regs.c: Add 'completer.h' include.
> +	(tui_reg_next_command): Renamed to...
> +	(tui_reg_name): ...this.  Adjust parameters.
> +	(tui_reg_prev_command): Renamed to...
> +	(tui_reg_prev): ...this.  Adjust parameters.
> +	(tui_reg_float_command): Delete.
> +	(tui_reg_general_command): Delete.
> +	(tui_reg_system_command): Delete.
> +	(tui_reg_command): Rewrite to perform switching of register group.
> +	(tuireglist): Remove.
> +	(tui_reggroup_completer): New function.
> +	(_initialize_tui_regs): Remove 'tui reg' sub-commands, update
> +	creation of 'tui reg' command.
> +
> +2015-05-21  Andrew Burgess  <andrew.burgess@embecosm.com>
> +
>  	* tui/tui-regs.c (tui_reg_prev_command): New function.
>  	(_initialize_tui_regs): Add 'prev' command for 'tui reg'.
>  	* reggroups.c (reggroup_prev): New function.
> diff --git a/gdb/completer.c b/gdb/completer.c
> index c8c0e4c..73f734e 100644
> --- a/gdb/completer.c
> +++ b/gdb/completer.c
> @@ -971,9 +971,10 @@ signal_completer (struct cmd_list_element *ignore,
>  
>  /* Complete on a register or reggroup.  */
>  
> -VEC (char_ptr) *
> -reg_or_group_completer (struct cmd_list_element *ignore,
> -			const char *text, const char *word)
> +static VEC (char_ptr) *
> +reg_or_group_completer_1 (struct cmd_list_element *ignore,
> +			  const char *text, const char *word,
> +			  int add_registers, int add_reggroups)

Making this a bit mask may make callers more obvious.

> +  if (add_registers)
> +    for (i = 0;
> +	 (name = user_reg_map_regnum_to_name (gdbarch, i)) != NULL;
> +	 i++)
> +      {
> +	if (*name != '\0' && strncmp (word, name, len) == 0)
> +	  VEC_safe_push (char_ptr, result, xstrdup (name));
> +      }

Would you mind wrapping the whole for in a {} block?  I think
you could move the 'i' variable to this scope.

> +
> +  if (add_reggroups)
> +    for (group = reggroup_next (gdbarch, NULL);
> +	 group != NULL;
> +	 group = reggroup_next (gdbarch, group))
> +      {
> +	name = reggroup_name (group);
> +	if (strncmp (word, name, len) == 0)
> +	  VEC_safe_push (char_ptr, result, xstrdup (name));
> +      }

Likewise, and move 'group' local.

>  
>    return result;
>  }
>  
> +VEC (char_ptr) *
> +reg_or_group_completer (struct cmd_list_element *ignore,
> +			const char *text, const char *word)
> +{
> +  /* Complete on both registers and reggroups.  */
> +  return reg_or_group_completer_1 (ignore, text, word, 1, 1);
> +}
> +
> +VEC (char_ptr) *
> +reggroup_completer (struct cmd_list_element *ignore,
> +		    const char *text, const char *word)
> +{
> +  /* Complete on reggroups only.  */
> +  return reg_or_group_completer_1 (ignore, text, word, 0, 1);
> +}

Intro comments.  You could reuse the comments inside the
functions outside.

>  
>  /* Get the list of chars that are considered as word breaks
>     for the current command.  */
> diff --git a/gdb/completer.h b/gdb/completer.h
> index 56e1a2b..6c1f257 100644
> --- a/gdb/completer.h
> +++ b/gdb/completer.h
> @@ -96,6 +96,9 @@ extern VEC (char_ptr) *signal_completer (struct cmd_list_element *,
>  extern VEC (char_ptr) *reg_or_group_completer (struct cmd_list_element *,
>  					       const char *, const char *);
>  
> +extern VEC (char_ptr) *reggroup_completer (struct cmd_list_element *,
> +					   const char *, const char *);
> +
>  extern char *get_gdb_completer_quote_characters (void);
>  
>  extern char *gdb_completion_word_break_characters (void);
> diff --git a/gdb/tui/tui-regs.c b/gdb/tui/tui-regs.c
> index 8dbcda1..685a896 100644
> --- a/gdb/tui/tui-regs.c
> +++ b/gdb/tui/tui-regs.c
> @@ -39,6 +39,7 @@
>  #include "tui/tui-io.h"
>  #include "reggroups.h"
>  #include "valprint.h"
> +#include "completer.h"
>  
>  #include "gdb_curses.h"
>  
> @@ -557,10 +558,8 @@ tui_display_register (struct tui_data_element *data,
>  }
>  
>  static void
> -tui_reg_next_command (char *arg, int from_tty)
> +tui_reg_next (struct gdbarch *gdbarch)
>  {
> -  struct gdbarch *gdbarch = get_current_arch ();
> -
>    if (TUI_DATA_WIN != NULL)
>      {
>        struct reggroup *group
> @@ -576,10 +575,8 @@ tui_reg_next_command (char *arg, int from_tty)
>  }
>  
>  static void
> -tui_reg_prev_command (char *arg, int from_tty)
> +tui_reg_prev (struct gdbarch *gdbarch)
>  {
> -  struct gdbarch *gdbarch = get_current_arch ();
> -
>    if (TUI_DATA_WIN != NULL)
>      {
>        struct reggroup *group
> @@ -606,31 +603,84 @@ tui_reg_prev_command (char *arg, int from_tty)
>  }
>  
>  static void
> -tui_reg_float_command (char *arg, int from_tty)
> +tui_reg_command (char *args, int from_tty)
>  {
> -  tui_show_registers (float_reggroup);
> -}
> +  struct gdbarch *gdbarch = get_current_arch ();
>  
> -static void
> -tui_reg_general_command (char *arg, int from_tty)
> -{
> -  tui_show_registers (general_reggroup);
> -}
> +  if (args)

args != NULL


> +    {
> +      if (strcmp (args, "next") == 0)
> +	tui_reg_next (gdbarch);
> +      else if (strcmp (args, "prev") == 0)
> +	tui_reg_prev (gdbarch);

AFAICS, this doesn't handle unambiguious "next", "prev"
abbreviations, though "help tui reg" says it should work.

> +      else
> +	{
> +	  struct reggroup *group = NULL, *match = NULL;
> +	  size_t len = strlen (args);
>  
> -static void
> -tui_reg_system_command (char *arg, int from_tty)
> -{
> -  tui_show_registers (system_reggroup);
> +	  for (group = reggroup_next (gdbarch, group);
> +	       group != NULL;
> +	       group = reggroup_next (gdbarch, group))
> +	    {
> +	      if (strncmp (reggroup_name (group), args, len) == 0)
> +		{
> +		  if (match != NULL)
> +		    {
> +		      match = NULL;
> +		      break;

IIUC, this is to handle ambiguous cases, right?

> +		    }
> +		  match = group;

> +		}
> +	    }
> +
> +	  if (match != NULL)
> +	    tui_show_registers (match);
> +	  else
> +	    error (_("unknown register group '%s'"), args);

Though I think the ambiguous case should have a different error.

> +	}
> +    }
> +  else
> +    {
> +      struct reggroup *group = NULL;

No need to initialize as NULL ...

> +
> +      printf_unfiltered (_("\"tui reg\" must be followed by the name of "
> +			   "either a register group,\nor one of 'next' "
> +			   "or 'prev'.  Known register groups are:\n"));
> +      for (group = reggroup_next (gdbarch, group); group != NULL; )

... with:

      for (group = reggroup_next (gdbarch, NULL); group != NULL; )

which is a bit clearer.

> +	{
> +	  printf_unfiltered ("%s", reggroup_name (group));
> +	  group = reggroup_next (gdbarch, group);
> +	  if (group != NULL)
> +	    printf_unfiltered (", ");
> +	}
> +      printf_unfiltered ("\n");

(Though IMO, something like this is simpler to follow / more obvious:

      int first = 1;

      for (group = reggroup_next (gdbarch, NULL);
           group != NULL;
           group = reggroup_next (gdbarch, group))
	{
	  if (!first)
	    printf_unfiltered (", ");
          else
            first = 0;
	  printf_unfiltered ("%s", reggroup_name (group));
	}
)

> +    }
>  }
>  
> -static struct cmd_list_element *tuireglist;
> +/* Complete names of register groups, and add the special "prev" and "next"
> +   names.  */
>  
> -static void
> -tui_reg_command (char *args, int from_tty)
> +static VEC (char_ptr) *
> +tui_reggroup_completer (struct cmd_list_element *ignore,
> +			const char *text, const char *word)
>  {
> -  printf_unfiltered (_("\"tui reg\" must be followed by the name of a "
> -                     "tui reg command.\n"));
> -  help_list (tuireglist, "tui reg ", all_commands, gdb_stdout);
> +  VEC (char_ptr) *result = NULL;
> +  static const char *extra[] = { "next", "prev", NULL };
> +  size_t len = strlen (word);
> +  const char **tmp;
> +
> +  if (!target_has_registers)
> +    return result;

Do we need this check?  If the target is not running yet, we still
have a default architecture.  AFAICS, tui_reg_command itself works
with get_current_arch, which works even without a running program.
I think the completer should match.  Ah, reggroup_completer gets
gdbarch from the selected frame.  Definitely the completer and the
"tui reg" function itself should use the same gdbarch.  Sounds like
we should pass a gdbarch to reggroup_completer, and leave it to
callers to decide which to use.

> +
> +  result = reggroup_completer (ignore, text, word);
> +
> +  for (tmp = extra; *tmp; ++tmp)

*tmp != NULL

> +    {
> +      if (strncmp (word, *tmp, len) == 0)
> +	VEC_safe_push (char_ptr, result, xstrdup (*tmp));
> +    }
> +
> +  return result;
>  }
>  
>  /* Provide a prototype to silence -Wmissing-prototypes.  */
> @@ -639,30 +689,13 @@ extern initialize_file_ftype _initialize_tui_regs;
>  void
>  _initialize_tui_regs (void)
>  {
> -  struct cmd_list_element **tuicmd;
> +  struct cmd_list_element **tuicmd, *cmd;
>  
>    tuicmd = tui_get_cmd_list ();
>  
> -  add_prefix_cmd ("reg", class_tui, tui_reg_command,
> -                  _("TUI commands to control the register window."),
> -                  &tuireglist, "tui reg ", 0,
> -                  tuicmd);
> -
> -  add_cmd ("float", class_tui, tui_reg_float_command,
> -           _("Display only floating point registers."),
> -           &tuireglist);
> -  add_cmd ("general", class_tui, tui_reg_general_command,
> -           _("Display only general registers."),
> -           &tuireglist);
> -  add_cmd ("system", class_tui, tui_reg_system_command,
> -           _("Display only system registers."),
> -           &tuireglist);
> -  add_cmd ("next", class_tui, tui_reg_next_command,
> -           _("Display next register group."),
> -           &tuireglist);
> -  add_cmd ("prev", class_tui, tui_reg_prev_command,
> -           _("Display previous register group."),
> -           &tuireglist);

(I notice we lose these expanded reggroup descriptions ("floating point"
vs "float").  We could save them by adding a "long_name" string
field to "struct reggroup", which doesn't look complicated.
Though I guess this isn't a big deal.  For another day.

(We could then reuse the reggroup description in "maint print reggroups".
Hmm, why is that a maintenance command, if we can use the reggroup
names in "info register"?  (and "help info registers" doesn't talk
about register groups, even though they are accepted.  Gah, lots of
small paper cuts.  :-/)
)

Thanks,
Pedro Alves

Re: [PATCH 2/2] gdb: Rework command completion on 'tui reg'.

[Andrew Burgess]

Thanks for the review.  I've reworked the patch to address all the
issues raised.  Here's the new version.

--

We previously specified a few known register groups for the 'tui reg'
command.  Other register groups could be accessed, but only by using the
'tui reg next' command and cycling through all the groups.

This commit removes the hard coded sub-commands of 'tui reg' and instead
adds dynamic completion of sub-commands based on the architecturally
defined register groups, giving immediate access to all available
register groups.

There is still the 'next' and 'prev' commands for cycling through the
register groups if that's wanted.

The new code maintains the ability to only enter partial names for
register groups, which is something we got for free when using the
standard sub-command mechanism.

The register (and register group) completer has been changed to use
get_current_arch rather than using the architecture of the currently
selected frame.  When the target is running, this is equivalent,
however, when the target is not running, using get_current_arch will
provide results from the default architecture.

gdb/ChangeLog:

	* completer.c Add arch-utils.h include.
	(enum reg_completer_targets): New enum.
	(reg_or_group_completer_1): New function containing old
	reg_or_group_completer, add and use new parameter to control what
	is completed on.  Use get_current_arch rather than architecture of
	currently selected frame.
	(reg_or_group_completer): Call new reg_or_group_completer_1.
	(reggroup_completer): Call new reg_or_group_completer_1.
	* completer.h (reggroup_completer): Add declaration.
	* tui/tui-regs.c: Add 'completer.h' include.
	(tui_reg_next_command): Renamed to...
	(tui_reg_next): ...this.  Adjust parameters and return rather than
	display new group.
	(tui_reg_prev_command): Renamed to...
	(tui_reg_prev): ...this.  Adjust parameters and return rather than
	display new group.
	(tui_reg_float_command): Delete.
	(tui_reg_general_command): Delete.
	(tui_reg_system_command): Delete.
	(tui_reg_command): Rewrite to perform switching of register group.
	Add header comment.
	(tuireglist): Remove.
	(tui_reggroup_completer): New function.
	(_initialize_tui_regs): Remove 'tui reg' sub-commands, update
	creation of 'tui reg' command.
	* NEWS: Add comment about 'tui reg' changes.

gdb/doc/ChangeLog:

	* gdb.texinfo (TUI Commands): Bring all 'tui reg' commands into a
	single table entry.
---
 gdb/ChangeLog       |  29 +++++++++
 gdb/NEWS            |   3 +
 gdb/completer.c     |  79 +++++++++++++++++++------
 gdb/completer.h     |   3 +
 gdb/doc/ChangeLog   |   5 ++
 gdb/doc/gdb.texinfo |  47 ++++++++-------
 gdb/tui/tui-regs.c  | 165 +++++++++++++++++++++++++++++++++-------------------
 7 files changed, 233 insertions(+), 98 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index eca000f..bc1bd49 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,32 @@
+2015-06-01  Andrew Burgess  <andrew.burgess@embecosm.com>
+
+	* completer.c Add arch-utils.h include.
+	(enum reg_completer_targets): New enum.
+	(reg_or_group_completer_1): New function containing old
+	reg_or_group_completer, add and use new parameter to control what
+	is completed on.  Use get_current_arch rather than architecture of
+	currently selected frame.
+	(reg_or_group_completer): Call new reg_or_group_completer_1.
+	(reggroup_completer): Call new reg_or_group_completer_1.
+	* completer.h (reggroup_completer): Add declaration.
+	* tui/tui-regs.c: Add 'completer.h' include.
+	(tui_reg_next_command): Renamed to...
+	(tui_reg_next): ...this.  Adjust parameters and return rather than
+	display new group.
+	(tui_reg_prev_command): Renamed to...
+	(tui_reg_prev): ...this.  Adjust parameters and return rather than
+	display new group.
+	(tui_reg_float_command): Delete.
+	(tui_reg_general_command): Delete.
+	(tui_reg_system_command): Delete.
+	(tui_reg_command): Rewrite to perform switching of register group.
+	Add header comment.
+	(tuireglist): Remove.
+	(tui_reggroup_completer): New function.
+	(_initialize_tui_regs): Remove 'tui reg' sub-commands, update
+	creation of 'tui reg' command.
+	* NEWS: Add comment about 'tui reg' changes.
+
 2015-06-02  Yao Qi  <yao.qi@linaro.org>
 
 	* i386-linux-nat.c: Include linux-nat.h.
diff --git a/gdb/NEWS b/gdb/NEWS
index bbfb55d..2933e35 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -52,6 +52,9 @@
 
 * GDB now supports the vector ABI on S/390 GNU/Linux targets.
 
+* The "tui reg" command now provides completion for all of the
+  available register groups, including target specific groups.
+
 * Guile Scripting
 
   ** Memory ports can now be unbuffered.
diff --git a/gdb/completer.c b/gdb/completer.c
index c8c0e4c..a0be935 100644
--- a/gdb/completer.c
+++ b/gdb/completer.c
@@ -26,6 +26,7 @@
 #include "target.h"
 #include "reggroups.h"
 #include "user-regs.h"
+#include "arch-utils.h"
 
 #include "cli/cli-decode.h"
 
@@ -969,44 +970,86 @@ signal_completer (struct cmd_list_element *ignore,
   return return_val;
 }
 
-/* Complete on a register or reggroup.  */
+/* Bit-flags for selecting what the register and/or register-group
+   completer should complete on.  */
 
-VEC (char_ptr) *
-reg_or_group_completer (struct cmd_list_element *ignore,
-			const char *text, const char *word)
+enum reg_completer_targets
+  {
+    complete_register_names = 0x1,
+    complete_reggroup_names = 0x2
+  };
+
+/* Complete on a register names (when WITH_REGISTERS is true) and/or
+   reggroup names (when WITH_REGGROUPS) is true.  One, or both of
+   WITH_REGISTERS and WITH_REGGROUPS must be true.  */
+
+static VEC (char_ptr) *
+reg_or_group_completer_1 (struct cmd_list_element *ignore,
+			  const char *text, const char *word,
+			  enum reg_completer_targets targets)
 {
   VEC (char_ptr) *result = NULL;
   size_t len = strlen (word);
   struct gdbarch *gdbarch;
-  struct reggroup *group;
   const char *name;
-  int i;
 
   if (!target_has_registers)
     return result;
 
-  gdbarch = get_frame_arch (get_selected_frame (NULL));
+  gdb_assert ((targets & (complete_register_names
+			  | complete_reggroup_names)) != 0);
+  gdbarch = get_current_arch ();
 
-  for (i = 0;
-       (name = user_reg_map_regnum_to_name (gdbarch, i)) != NULL;
-       i++)
+  if ((targets & complete_register_names) != 0)
     {
-      if (*name != '\0' && strncmp (word, name, len) == 0)
-	VEC_safe_push (char_ptr, result, xstrdup (name));
+      int i;
+
+      for (i = 0;
+	   (name = user_reg_map_regnum_to_name (gdbarch, i)) != NULL;
+	   i++)
+	{
+	  if (*name != '\0' && strncmp (word, name, len) == 0)
+	    VEC_safe_push (char_ptr, result, xstrdup (name));
+	}
     }
 
-  for (group = reggroup_next (gdbarch, NULL);
-       group != NULL;
-       group = reggroup_next (gdbarch, group))
+  if ((targets & complete_reggroup_names) != 0)
     {
-      name = reggroup_name (group);
-      if (strncmp (word, name, len) == 0)
-	VEC_safe_push (char_ptr, result, xstrdup (name));
+      struct reggroup *group;
+
+      for (group = reggroup_next (gdbarch, NULL);
+	   group != NULL;
+	   group = reggroup_next (gdbarch, group))
+	{
+	  name = reggroup_name (group);
+	  if (strncmp (word, name, len) == 0)
+	    VEC_safe_push (char_ptr, result, xstrdup (name));
+	}
     }
 
   return result;
 }
 
+/* Perform completion on register and reggroup names.  */
+
+VEC (char_ptr) *
+reg_or_group_completer (struct cmd_list_element *ignore,
+			const char *text, const char *word)
+{
+  return reg_or_group_completer_1 (ignore, text, word,
+				   (complete_register_names
+				    | complete_reggroup_names));
+}
+
+/* Perform completion on reggroup names.  */
+
+VEC (char_ptr) *
+reggroup_completer (struct cmd_list_element *ignore,
+		    const char *text, const char *word)
+{
+  return reg_or_group_completer_1 (ignore, text, word,
+				   complete_reggroup_names);
+}
 
 /* Get the list of chars that are considered as word breaks
    for the current command.  */
diff --git a/gdb/completer.h b/gdb/completer.h
index 56e1a2b..6c1f257 100644
--- a/gdb/completer.h
+++ b/gdb/completer.h
@@ -96,6 +96,9 @@ extern VEC (char_ptr) *signal_completer (struct cmd_list_element *,
 extern VEC (char_ptr) *reg_or_group_completer (struct cmd_list_element *,
 					       const char *, const char *);
 
+extern VEC (char_ptr) *reggroup_completer (struct cmd_list_element *,
+					   const char *, const char *);
+
 extern char *get_gdb_completer_quote_characters (void);
 
 extern char *gdb_completion_word_break_characters (void);
diff --git a/gdb/doc/ChangeLog b/gdb/doc/ChangeLog
index 171d30e..24555c2 100644
--- a/gdb/doc/ChangeLog
+++ b/gdb/doc/ChangeLog
@@ -1,3 +1,8 @@
+2015-06-01  Andrew Burgess  <andrew.burgess@embecosm.com>
+
+	* gdb.texinfo (TUI Commands): Bring all 'tui reg' commands into a
+	single table entry.
+
 2015-05-27  Doug Evans  <dje@google.com>
 
 	* gdb.texinfo (Debugging Output): Mention set/show debug dwarf-line.
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 9ea846a..f927eff 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -25009,27 +25009,34 @@ Make the command window active for scrolling.
 @kindex refresh
 Refresh the screen.  This is similar to typing @kbd{C-L}.
 
-@item tui reg float
+@item tui reg @var{group}
 @kindex tui reg
-Show the floating point registers in the register window.
-
-@item tui reg general
-Show the general registers in the register window.
-
-@item tui reg next
-Show the next register group.  The list of register groups as well as
-their order is target specific.  The predefined register groups are the
-following: @code{general}, @code{float}, @code{system}, @code{vector},
-@code{all}, @code{save}, @code{restore}.
-
-@item tui reg prev
-Show the previous register group.  The list of register groups as well
-as their order is target specific.  The predefined register groups are
-the following: @code{general}, @code{float}, @code{system},
-@code{vector}, @code{all}, @code{save}, @code{restore}.
-
-@item tui reg system
-Show the system registers in the register window.
+Changes the register group displayed in the tui register window to
+@var{group}.  If the register window is not currently displayed this
+command will cause the register window to be displayed.  The list of
+register groups, as well as their order is target specific. The
+following groups are avaiable on most targets:
+@table @code
+@item next
+Repeatedly selecting this group will cause the display to cycle
+through all of the available register groups.
+
+@item prev
+Repeatedly selecting this group will cause the display to cycle
+through all of the available register groups in the reverse order to
+@var{next}.
+
+@item general
+Display the general registers.
+@item float
+Display the floating point registers.
+@item system
+Display the system registers.
+@item vector
+Display the vector registers.
+@item all
+Display all registers.
+@end table
 
 @item update
 @kindex update
diff --git a/gdb/tui/tui-regs.c b/gdb/tui/tui-regs.c
index 6ac3c2b..a61fadb 100644
--- a/gdb/tui/tui-regs.c
+++ b/gdb/tui/tui-regs.c
@@ -39,6 +39,7 @@
 #include "tui/tui-io.h"
 #include "reggroups.h"
 #include "valprint.h"
+#include "completer.h"
 
 #include "gdb_curses.h"
 
@@ -556,74 +557,135 @@ tui_display_register (struct tui_data_element *data,
     }
 }
 
-static void
-tui_reg_next_command (char *arg, int from_tty)
+/* Helper for "tui reg next", wraps a call to REGGROUP_NEXT, but adds wrap
+   around behaviour.  Returns the next register group, or NULL if the
+   register window is not currently being displayed.  */
+
+static struct reggroup *
+tui_reg_next (struct gdbarch *gdbarch)
 {
-  struct gdbarch *gdbarch = get_current_arch ();
+  struct reggroup *group = NULL;
 
   if (TUI_DATA_WIN != NULL)
     {
-      struct reggroup *group
-        = TUI_DATA_WIN->detail.data_display_info.current_group;
-
+      group = TUI_DATA_WIN->detail.data_display_info.current_group;
       group = reggroup_next (gdbarch, group);
       if (group == NULL)
         group = reggroup_next (gdbarch, NULL);
-
-      if (group != NULL)
-        tui_show_registers (group);
     }
+  return group;
 }
 
-/* Implementation of the "tui reg prev" command.  Cycle the register group
-   displayed in the tui REG window, moving backwards through the list of
-   available register groups.  */
+/* Helper for "tui reg prev", wraps a call to REGGROUP_PREV, but adds wrap
+   around behaviour.  Returns the previous register group, or NULL if the
+   register window is not currently being displayed.  */
 
-static void
-tui_reg_prev_command (char *arg, int from_tty)
+static struct reggroup *
+tui_reg_prev (struct gdbarch *gdbarch)
 {
-  struct gdbarch *gdbarch = get_current_arch ();
+  struct reggroup *group = NULL;
 
   if (TUI_DATA_WIN != NULL)
     {
-      struct reggroup *group
-        = TUI_DATA_WIN->detail.data_display_info.current_group;
-
+      group = TUI_DATA_WIN->detail.data_display_info.current_group;
       group = reggroup_prev (gdbarch, group);
       if (group == NULL)
 	group = reggroup_prev (gdbarch, NULL);
-
-      if (group != NULL)
-	tui_show_registers (group);
     }
+  return group;
 }
 
-static void
-tui_reg_float_command (char *arg, int from_tty)
-{
-  tui_show_registers (float_reggroup);
-}
+/* Implement the 'tui reg' command.  Changes the register group displayed
+   in the tui register window.  Displays the tui register window if it is
+   not already on display.  */
 
 static void
-tui_reg_general_command (char *arg, int from_tty)
+tui_reg_command (char *args, int from_tty)
 {
-  tui_show_registers (general_reggroup);
-}
+  struct gdbarch *gdbarch = get_current_arch ();
 
-static void
-tui_reg_system_command (char *arg, int from_tty)
-{
-  tui_show_registers (system_reggroup);
+  if (args != NULL)
+    {
+      struct reggroup *group, *match = NULL;
+      size_t len = strlen (args);
+
+      /* Make sure the curses mode is enabled.  */
+      tui_enable ();
+
+      /* Make sure the register window is visible.  If not, select an
+	 appropriate layout.  We need to do this before trying to run the
+	 'next' or 'prev' commands.  */
+      if (TUI_DATA_WIN == NULL || !TUI_DATA_WIN->generic.is_visible)
+	tui_set_layout_by_name (DATA_NAME);
+
+      if (strncmp (args, "next", len) == 0)
+	match = tui_reg_next (gdbarch);
+      else if (strncmp (args, "prev", len) == 0)
+	match = tui_reg_prev (gdbarch);
+
+      /* This loop matches on the initial part of a register group
+	 name.  If this initial part in ARGS matches only one register
+	 group then the switch is made.  */
+      for (group = reggroup_next (gdbarch, NULL);
+	   group != NULL;
+	   group = reggroup_next (gdbarch, group))
+	{
+	  if (strncmp (reggroup_name (group), args, len) == 0)
+	    {
+	      if (match != NULL)
+		error (_("ambiguous register group name '%s'"), args);
+	      match = group;
+	    }
+	}
+
+      if (match == NULL)
+	error (_("unknown register group '%s'"), args);
+
+      tui_show_registers (match);
+    }
+  else
+    {
+      struct reggroup *group;
+      int first;
+
+      printf_unfiltered (_("\"tui reg\" must be followed by the name of "
+			   "either a register group,\nor one of 'next' "
+			   "or 'prev'.  Known register groups are:\n"));
+
+      for (first = 1, group = reggroup_next (gdbarch, NULL);
+	   group != NULL;
+	   first = 0, group = reggroup_next (gdbarch, group))
+	{
+	  if (!first)
+	    printf_unfiltered (", ");
+	  printf_unfiltered ("%s", reggroup_name (group));
+	}
+
+      printf_unfiltered ("\n");
+    }
 }
 
-static struct cmd_list_element *tuireglist;
+/* Complete names of register groups, and add the special "prev" and "next"
+   names.  */
 
-static void
-tui_reg_command (char *args, int from_tty)
+static VEC (char_ptr) *
+tui_reggroup_completer (struct cmd_list_element *ignore,
+			const char *text, const char *word)
 {
-  printf_unfiltered (_("\"tui reg\" must be followed by the name of a "
-                     "tui reg command.\n"));
-  help_list (tuireglist, "tui reg ", all_commands, gdb_stdout);
+  VEC (char_ptr) *result = NULL;
+  static const char *extra[] = { "next", "prev", NULL };
+  size_t len = strlen (word);
+  const char **tmp;
+
+  result = reggroup_completer (ignore, text, word);
+
+  for (tmp = extra; *tmp != NULL; ++tmp)
+    {
+      if (strncmp (word, *tmp, len) == 0)
+	VEC_safe_push (char_ptr, result, xstrdup (*tmp));
+    }
+
+  return result;
 }
 
 /* Provide a prototype to silence -Wmissing-prototypes.  */
@@ -632,30 +694,13 @@ extern initialize_file_ftype _initialize_tui_regs;
 void
 _initialize_tui_regs (void)
 {
-  struct cmd_list_element **tuicmd;
+  struct cmd_list_element **tuicmd, *cmd;
 
   tuicmd = tui_get_cmd_list ();
 
-  add_prefix_cmd ("reg", class_tui, tui_reg_command,
-                  _("TUI commands to control the register window."),
-                  &tuireglist, "tui reg ", 0,
-                  tuicmd);
-
-  add_cmd ("float", class_tui, tui_reg_float_command,
-           _("Display only floating point registers."),
-           &tuireglist);
-  add_cmd ("general", class_tui, tui_reg_general_command,
-           _("Display only general registers."),
-           &tuireglist);
-  add_cmd ("system", class_tui, tui_reg_system_command,
-           _("Display only system registers."),
-           &tuireglist);
-  add_cmd ("next", class_tui, tui_reg_next_command,
-           _("Display next register group."),
-           &tuireglist);
-  add_cmd ("prev", class_tui, tui_reg_prev_command,
-           _("Display previous register group."),
-           &tuireglist);
+  cmd = add_cmd ("reg", class_tui, tui_reg_command, _("\
+TUI command to control the register window."), tuicmd);
+  set_cmd_completer (cmd, tui_reggroup_completer);
 }
 
 
-- 
2.4.0

Re: [PATCH 2/2] gdb: Rework command completion on 'tui reg'.

[Eli Zaretskii]

> Date: Tue, 2 Jun 2015 11:11:11 +0100
> From: Andrew Burgess <andrew.burgess@embecosm.com>
> Cc: Pedro Alves <palves@redhat.com>
> 
> Thanks for the review.  I've reworked the patch to address all the
> issues raised.  Here's the new version.

Thanks.

> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -52,6 +52,9 @@
>  
>  * GDB now supports the vector ABI on S/390 GNU/Linux targets.
>  
> +* The "tui reg" command now provides completion for all of the
> +  available register groups, including target specific groups.

This part is OK.

> +Changes the register group displayed in the tui register window to
> +@var{group}.  If the register window is not currently displayed this
> +command will cause the register window to be displayed.  The list of
> +register groups, as well as their order is target specific. The
> +following groups are avaiable on most targets:            ^^

Two spaces between sentences, please.

> +@table @code
> +@item next
> +Repeatedly selecting this group will cause the display to cycle
> +through all of the available register groups.
> +
> +@item prev
> +Repeatedly selecting this group will cause the display to cycle
> +through all of the available register groups in the reverse order to
> +@var{next}.
> +
> +@item general
> +Display the general registers.
> +@item float
> +Display the floating point registers.
> +@item system
> +Display the system registers.
> +@item vector
> +Display the vector registers.
> +@item all
> +Display all registers.
> +@end table

Please keep one empty line before each @item, for consistency.

The documentation part is OK with those fixed.

Re: [PATCH 2/2] gdb: Rework command completion on 'tui reg'.

[Pedro Alves]

On 06/02/2015 11:11 AM, Andrew Burgess wrote:
> Thanks for the review.  I've reworked the patch to address all the
> issues raised.  Here's the new version.
> 

Thanks.  Sorry for the delay, I was away last week.

This version looks fine apart from a couple details.  Go ahead
and push with these fixed.

> gdb/ChangeLog:
> 
> 	* completer.c Add arch-utils.h include.

Missing colon.


> -/* Complete on a register or reggroup.  */
> +/* Bit-flags for selecting what the register and/or register-group
> +   completer should complete on.  */
>  
> -VEC (char_ptr) *
> -reg_or_group_completer (struct cmd_list_element *ignore,
> -			const char *text, const char *word)
> +enum reg_completer_targets
> +  {
> +    complete_register_names = 0x1,
> +    complete_reggroup_names = 0x2
> +  };
> +
> +/* Complete on a register names (when WITH_REGISTERS is true) and/or
> +   reggroup names (when WITH_REGGROUPS) is true.  One, or both of
> +   WITH_REGISTERS and WITH_REGGROUPS must be true.  */

The comment is stale wrt to the bit flags.

> +
> +static VEC (char_ptr) *
> +reg_or_group_completer_1 (struct cmd_list_element *ignore,
> +			  const char *text, const char *word,
> +			  enum reg_completer_targets targets)
>  {
>    VEC (char_ptr) *result = NULL;
>    size_t len = strlen (word);
>    struct gdbarch *gdbarch;
> -  struct reggroup *group;
>    const char *name;
> -  int i;
>  
>    if (!target_has_registers)
>      return result;

Unless I'm missing something, I think you need to remove
this check, otherwise completion doesn't really work when there's
no running program yet.

>  
> -  gdbarch = get_frame_arch (get_selected_frame (NULL));
> +  gdb_assert ((targets & (complete_register_names
> +			  | complete_reggroup_names)) != 0);
> +  gdbarch = get_current_arch ();
>  

> +following groups are avaiable on most targets:

typo "available".

Thanks,
Pedro Alves