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

Message ID dca46b0276e36ec42116d95aa850de38978f3b70.1432246159.git.andrew.burgess@embecosm.com
State Dropped
Headers

Commit Message

Andrew Burgess May 21, 2015, 10:17 p.m. UTC
  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(-)
  

Comments

Pedro Alves May 22, 2015, 1:06 a.m. UTC | #1
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
  
Eli Zaretskii May 22, 2015, 7:15 a.m. UTC | #2
> 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.
  

Patch

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);
 }