Re: [PATCH] Provide useful completer for "info registers"

[Sergio Durigan Junior <sergiodj@redhat.com>]

On Tuesday, November 25 2014, Andreas Arnez wrote:

> Provide a new completion function for the argument of "info
> registers", "info all-registers", and the "lr" command in dbx mode.
> Without this patch the default symbol completer is used, which is more
> confusing than helpful.

Thank you for the patch, Andreas!

> gdb/ChangeLog:
>
> 	* completer.c: Include "target.h" and "reggroups.h".
> 	(reg_or_group_completer): New.
> 	* completer.h (reg_or_group_completer): Declare.
> 	* infcmd.c (_initialize_infcmd): Set reg_or_group_completer for
> 	the "info registers" and "info all-registers" commands and the
> 	dbx-mode "lr" command.
> ---
>  gdb/completer.c |   51 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  gdb/completer.h |    3 +++
>  gdb/infcmd.c    |   12 +++++++++---
>  3 files changed, 63 insertions(+), 3 deletions(-)
>
> diff --git a/gdb/completer.c b/gdb/completer.c
> index a0f3fa3..42188c0 100644
> --- a/gdb/completer.c
> +++ b/gdb/completer.c
> @@ -23,6 +23,8 @@
>  #include "filenames.h"		/* For DOSish file names.  */
>  #include "language.h"
>  #include "gdb_signals.h"
> +#include "target.h"
> +#include "reggroups.h"
>  
>  #include "cli/cli-decode.h"
>  
> @@ -836,6 +838,55 @@ signal_completer (struct cmd_list_element *ignore,
>    return return_val;
>  }
>  
> +/* Complete on a register or reggroup.  */
> +
> +VEC (char_ptr) *
> +reg_or_group_completer (struct cmd_list_element *ignore,
> +			const char *text, const char *word)
> +{
> +  VEC (char_ptr) *result = NULL;
> +  size_t len = strlen (text);

Hm, this should be "strlen (word)".

The "text" will hold the entire line that is being completed, and "word"
will hold just the last word, according to the breaking characters being
used for this specific completer.  For example, consider:

  (gdb) info registers rsp es

In this case, "text" will be "rsp es", and "word" will be "es".  Most of
the time, you will only be interested in using "word" for the
completion.

Therefore, the "len" variable should hold "strlen (word)".  Also, later
in the code you are comparing each register name against "text", but you
should be comparing against "word", for the reason explained above.

Yeah, it can be confusing :-/.

> +  struct frame_info *frame;
> +  struct gdbarch *gdbarch;
> +
> +  if (!target_has_registers)
> +    return result;
> +
> +  frame = get_selected_frame (NULL);
> +  gdbarch = get_frame_arch (frame);
> +
> +  {
> +    int i;
> +    int n_regs = (gdbarch_num_regs (gdbarch)
> +		  + gdbarch_num_pseudo_regs (gdbarch));
> +
> +    for (i = 0; i < n_regs; i++)
> +      {
> +	const char *reg_name = gdbarch_register_name (gdbarch, i);
> +
> +	if (reg_name != NULL && strncmp (text, reg_name, len) == 0)

As said above, here you should do:

  strncmp (word, reg_name, len) == 0

If you compare against "text", only the first completion will work,
i.e., you won't be able to complete more than one register name in the
command line.

> +	  VEC_safe_push (char_ptr, result, xstrdup (reg_name));
> +      }
> +  }
> +
> +  {
> +    struct reggroup *group;
> +
> +    for (group = reggroup_next (gdbarch, NULL);
> +	 group != NULL;
> +	 group = reggroup_next (gdbarch, group))
> +      {
> +	const char *group_name = reggroup_name (group);
> +
> +	if (strncmp (text, group_name, len) == 0)

Likewise.

> +	  VEC_safe_push (char_ptr, result, xstrdup (group_name));
> +      }
> +  }
> +
> +  return result;
> +}

While I understand and like this approach, we have a function that does
the "strncmp" dance for you.  All you need to do is provide a list of
possible candidates (char **), and the word being completed.  I gave it
a try and hacked your patch to do that.  The resulting patch is
attached, feel free to use it if you like the approach.

> +
> +
>  /* 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 bc7ed96..5e91030 100644
> --- a/gdb/completer.h
> +++ b/gdb/completer.h
> @@ -45,6 +45,9 @@ extern VEC (char_ptr) *command_completer (struct cmd_list_element *,
>  extern VEC (char_ptr) *signal_completer (struct cmd_list_element *,
>  					 const char *, const char *);
>  
> +extern VEC (char_ptr) *reg_or_group_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/infcmd.c b/gdb/infcmd.c
> index 4415b31..de0d24d 100644
> --- a/gdb/infcmd.c
> +++ b/gdb/infcmd.c
> @@ -3235,18 +3235,24 @@ If non-stop mode is enabled, interrupt only the current thread,\n\
>  otherwise all the threads in the program are stopped.  To \n\
>  interrupt all running threads in non-stop mode, use the -a option."));
>  
> -  add_info ("registers", nofp_registers_info, _("\
> +  c = add_info ("registers", nofp_registers_info, _("\
>  List of integer registers and their contents, for selected stack frame.\n\
>  Register name as argument means describe only that register."));
>    add_info_alias ("r", "registers", 1);
> +  set_cmd_completer (c, reg_or_group_completer);
>  
>    if (xdb_commands)
> -    add_com ("lr", class_info, nofp_registers_info, _("\
> +    {
> +      c = add_com ("lr", class_info, nofp_registers_info, _("\
>  List of integer registers and their contents, for selected stack frame.\n\
>  Register name as argument means describe only that register."));
> -  add_info ("all-registers", all_registers_info, _("\
> +      set_cmd_completer (c, reg_or_group_completer);
> +    }
> +
> +  c = add_info ("all-registers", all_registers_info, _("\
>  List of all registers and their contents, for selected stack frame.\n\
>  Register name as argument means describe only that register."));
> +  set_cmd_completer (c, reg_or_group_completer);
>  
>    add_info ("program", program_info,
>  	    _("Execution status of the program."));
> -- 
> 1.7.9.5

I'd say this patch also needs a testcase :-).  I know that this is
architecture specific, so I'd personally be happy with something very
simple, maybe testing only one or two architectures would be enough.

Other than that, it is fine by me (not an approval).  Thanks for doing
that.

-- 
Sergio
GPG key ID: 0x65FC5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Index: binutils-gdb-review-andreas-completer/gdb/completer.c
===================================================================
--- binutils-gdb-review-andreas-completer.orig/gdb/completer.c
+++ binutils-gdb-review-andreas-completer/gdb/completer.c
@@ -844,45 +844,43 @@ VEC (char_ptr) *
 reg_or_group_completer (struct cmd_list_element *ignore,
 			const char *text, const char *word)
 {
-  VEC (char_ptr) *result = NULL;
-  size_t len = strlen (text);
+  VEC (char_ptr) *result;
   struct frame_info *frame;
   struct gdbarch *gdbarch;
+  const char **regnames;
+  int count = 0, n_regs = 0, n_reggroups = 0;
+  int i;
+  struct reggroup *group;
 
   if (!target_has_registers)
-    return result;
+    return NULL;
 
   frame = get_selected_frame (NULL);
   gdbarch = get_frame_arch (frame);
 
-  {
-    int i;
-    int n_regs = (gdbarch_num_regs (gdbarch)
-		  + gdbarch_num_pseudo_regs (gdbarch));
-
-    for (i = 0; i < n_regs; i++)
-      {
-	const char *reg_name = gdbarch_register_name (gdbarch, i);
-
-	if (reg_name != NULL && strncmp (text, reg_name, len) == 0)
-	  VEC_safe_push (char_ptr, result, xstrdup (reg_name));
-      }
-  }
-
-  {
-    struct reggroup *group;
-
-    for (group = reggroup_next (gdbarch, NULL);
-	 group != NULL;
-	 group = reggroup_next (gdbarch, group))
-      {
-	const char *group_name = reggroup_name (group);
-
-	if (strncmp (text, group_name, len) == 0)
-	  VEC_safe_push (char_ptr, result, xstrdup (group_name));
-      }
-  }
+  n_regs = gdbarch_num_regs (gdbarch) + gdbarch_num_pseudo_regs (gdbarch);
+  for (group = reggroup_next (gdbarch, NULL);
+       group != NULL;
+       group = reggroup_next (gdbarch, group), ++n_reggroups);
+
+  regnames = xmalloc ((n_regs + n_reggroups + 1) * sizeof (char *));
+  for (i = 0; i < n_regs; i++)
+    {
+      const char *reg_name = gdbarch_register_name (gdbarch, i);
+
+      if (reg_name != NULL)
+	regnames[count++] = reg_name;
+    }
+
+  for (group = reggroup_next (gdbarch, NULL);
+       group != NULL;
+       group = reggroup_next (gdbarch, group))
+    regnames[count++] = reggroup_name (group);
 
+  regnames[count] = NULL;
+
+  result = complete_on_enum (regnames, text, word);
+  xfree (regnames);
   return result;
 }