This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 1/2] gdb: Add 'tui reg prev' command.
- From: Pedro Alves <palves at redhat dot com>
- To: Andrew Burgess <andrew dot burgess at embecosm dot com>, gdb-patches at sourceware dot org
- Date: Fri, 22 May 2015 02:06:12 +0100
- Subject: Re: [PATCH 1/2] gdb: Add 'tui reg prev' command.
- Authentication-results: sourceware.org; auth=none
- References: <cover dot 1432246159 dot git dot andrew dot burgess at embecosm dot com> <dca46b0276e36ec42116d95aa850de38978f3b70 dot 1432246159 dot git dot andrew dot burgess at embecosm dot com>
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