This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]