Provide useful completer for "info registers"

  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.
  

On Wednesday, November 26 2014, I wrote:

[...]
> +  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);

Ops, sorry, this should be:

  result = complete_on_enum (regnames, word, word);

complete_on_enum has such a weird interface that it makes me want to
cry...

Cheers,
  

On Wed, Nov 26 2014, Sergio Durigan Junior wrote:

> On Tuesday, November 25 2014, Andreas Arnez wrote:
>
>> [...]
>> @@ -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 :-/.

First I actually had used 'word' here, but then I noticed that the
completer's notion of words doesn't match how the command parses its
arguments.  If using 'word', the completer behaves like this:

  (gdb) complete info registers hello,g
  info registers hello,general

Which I consider a bit strange.  However, I realize this may not be a
real problem for users, and being able to expand multiple arguments
probably beats this flaw, so I'll use 'word', as suggested.

> [...]
>
> 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.

Thanks for the patch!  Indeed I didn't know about complete_on_enum()
before.  But after weighing pros and cons, I still prefer the "strncmp
dance": It's not longer and needs somewhat less logic, e.g. only two
instead of three loops and no temporary xmalloc'd buffer.  Also, I think
the code is easier to maintain if signal_completer and
reg_or_group_completer use the same approach.

But since it's a short function, I will dissolve the sub-blocks and move
the variable declarations to the top instead, like your patch does.

> 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.

Yes, a test case would probably be adequate.  I'll try it in an
architecture-independent way and include it in the next version.

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

Thanks for looking at this, and for your feedback.  Much appreciated.
  

On Friday, November 28 2014, Andreas Arnez wrote:

>> 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 :-/.
>
> First I actually had used 'word' here, but then I noticed that the
> completer's notion of words doesn't match how the command parses its
> arguments.  If using 'word', the completer behaves like this:
>
>   (gdb) complete info registers hello,g
>   info registers hello,general
>
> Which I consider a bit strange.  However, I realize this may not be a
> real problem for users, and being able to expand multiple arguments
> probably beats this flaw, so I'll use 'word', as suggested.

Yeah.  This is a problem with our completer scheme; you can see this
behavior happening also for other commands:

  (gdb) complete break hello,ma
  break hello,mabort
  break hello,madvise
  ...

And I agree that this may not be a real problem for users (since this
problem exists for a long time apparently).

>> [...]
>>
>> 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.
>
> Thanks for the patch!  Indeed I didn't know about complete_on_enum()
> before.  But after weighing pros and cons, I still prefer the "strncmp
> dance": It's not longer and needs somewhat less logic, e.g. only two
> instead of three loops and no temporary xmalloc'd buffer.  Also, I think
> the code is easier to maintain if signal_completer and
> reg_or_group_completer use the same approach.

Agreed :-).  Although complete_on_enum exists, its interface is in dire
need of a revamp; besides, using it on your function does not make the
code simpler or clearer as you pointed.

> But since it's a short function, I will dissolve the sub-blocks and move
> the variable declarations to the top instead, like your patch does.

Thanks.

>> 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.
>
> Yes, a test case would probably be adequate.  I'll try it in an
> architecture-independent way and include it in the next version.
>
>> Other than that, it is fine by me (not an approval).  Thanks for doing
>> that.
>
> Thanks for looking at this, and for your feedback.  Much appreciated.

No, thank you!  :-)
  

Thanks Andreas,

I think a register completer is a great idea.

On 11/26/2014 08:54 PM, Sergio Durigan Junior wrote:
> 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.

I think $pc, $sp, $fp (the user regs) should work everywhere.

See user-regs.c and std-regs.c.

Actually, looks like the patch misses considering those for completion?

See infcmd.c:registers_info:

      /* A register name?  */
      {
        int regnum = user_reg_map_name_to_regnum (gdbarch, start, end - start);

        if (regnum >= 0)
          {
            /* User registers lie completely outside of the range of
               normal registers.  Catch them early so that the target
               never sees them.  */
            if (regnum >= gdbarch_num_regs (gdbarch)
                          + gdbarch_num_pseudo_regs (gdbarch))
              {

Thanks,
Pedro Alves
  

On Thu, Dec 04 2014, Pedro Alves wrote:

> Thanks Andreas,
>
> I think a register completer is a great idea.

Thanks, I thought so, too ;-)

>
> On 11/26/2014 08:54 PM, Sergio Durigan Junior wrote:
>> 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.
>
> I think $pc, $sp, $fp (the user regs) should work everywhere.
>
> See user-regs.c and std-regs.c.
>
> Actually, looks like the patch misses considering those for completion?
>
> See infcmd.c:registers_info:
>
>       /* A register name?  */
>       {
>         int regnum = user_reg_map_name_to_regnum (gdbarch, start, end - start);
>
>         if (regnum >= 0)
>           {
>             /* User registers lie completely outside of the range of
>                normal registers.  Catch them early so that the target
>                never sees them.  */
>             if (regnum >= gdbarch_num_regs (gdbarch)
>                           + gdbarch_num_pseudo_regs (gdbarch))
>               {

Yes, the patch misses them...  Note that this is consistent with the
list of registers shown by "mt print registers" or "info registers all".
But you're right, the "info registers" command accepts user registers,
so the completer should offer them as well.  Added this in v3.

It seems that user registers can not be listed by any means in GDB.
Most architectures only have the built-in set of user registers ($pc,
$sp, $fp, and $ps), but some define additional ones.  This creates a
difficulty with the test case, which tries to determine the full list of
registers and reggroups the completer is supposed to yield.  Thus I just
added such a command in v3 as well.
  

On 12/10/2014 05:35 PM, Andreas Arnez wrote:

> It seems that user registers can not be listed by any means in GDB.
> Most architectures only have the built-in set of user registers ($pc,
> $sp, $fp, and $ps), but some define additional ones.  This creates a
> difficulty with the test case, which tries to determine the full list of
> registers and reggroups the completer is supposed to yield.  Thus I just
> added such a command in v3 as well.

Sounds excellent, thanks.

Pedro Alves