public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* [PATCH] Provide useful completer for "info registers"
@ 2014-11-25 17:28 Andreas Arnez
  2014-11-26 20:54 ` Sergio Durigan Junior
  2014-12-04 17:38 ` Pedro Alves
  0 siblings, 2 replies; 10+ messages in thread
From: Andreas Arnez @ 2014-11-25 17:28 UTC (permalink / raw)
  To: gdb-patches

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.

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);
+  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)
+	  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));
+      }
+  }
+
+  return result;
+}
+
+
 /* 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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Provide useful completer for "info registers"
  2014-11-25 17:28 [PATCH] Provide useful completer for "info registers" Andreas Arnez
@ 2014-11-26 20:54 ` Sergio Durigan Junior
  2014-11-26 21:52   ` Sergio Durigan Junior
                     ` (2 more replies)
  2014-12-04 17:38 ` Pedro Alves
  1 sibling, 3 replies; 10+ messages in thread
From: Sergio Durigan Junior @ 2014-11-26 20:54 UTC (permalink / raw)
  To: Andreas Arnez; +Cc: gdb-patches

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Provide useful completer for "info registers"
  2014-11-26 20:54 ` Sergio Durigan Junior
@ 2014-11-26 21:52   ` Sergio Durigan Junior
  2014-11-28 18:14   ` Andreas Arnez
  2014-12-04 17:34   ` Pedro Alves
  2 siblings, 0 replies; 10+ messages in thread
From: Sergio Durigan Junior @ 2014-11-26 21:52 UTC (permalink / raw)
  To: Andreas Arnez; +Cc: gdb-patches

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,

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Provide useful completer for "info registers"
  2014-11-26 20:54 ` Sergio Durigan Junior
  2014-11-26 21:52   ` Sergio Durigan Junior
@ 2014-11-28 18:14   ` Andreas Arnez
  2014-11-28 20:39     ` Sergio Durigan Junior
  2014-12-04 17:34   ` Pedro Alves
  2 siblings, 1 reply; 10+ messages in thread
From: Andreas Arnez @ 2014-11-28 18:14 UTC (permalink / raw)
  To: Sergio Durigan Junior; +Cc: gdb-patches

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.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Provide useful completer for "info registers"
  2014-11-28 18:14   ` Andreas Arnez
@ 2014-11-28 20:39     ` Sergio Durigan Junior
  0 siblings, 0 replies; 10+ messages in thread
From: Sergio Durigan Junior @ 2014-11-28 20:39 UTC (permalink / raw)
  To: Andreas Arnez; +Cc: gdb-patches

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!  :-)

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Provide useful completer for "info registers"
  2014-11-26 20:54 ` Sergio Durigan Junior
  2014-11-26 21:52   ` Sergio Durigan Junior
  2014-11-28 18:14   ` Andreas Arnez
@ 2014-12-04 17:34   ` Pedro Alves
  2014-12-10 17:36     ` Andreas Arnez
  2 siblings, 1 reply; 10+ messages in thread
From: Pedro Alves @ 2014-12-04 17:34 UTC (permalink / raw)
  To: Sergio Durigan Junior, Andreas Arnez; +Cc: gdb-patches

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Provide useful completer for "info registers"
  2014-11-25 17:28 [PATCH] Provide useful completer for "info registers" Andreas Arnez
  2014-11-26 20:54 ` Sergio Durigan Junior
@ 2014-12-04 17:38 ` Pedro Alves
  2014-12-10 17:48   ` Andreas Arnez
  1 sibling, 1 reply; 10+ messages in thread
From: Pedro Alves @ 2014-12-04 17:38 UTC (permalink / raw)
  To: Andreas Arnez, gdb-patches

Also, ...

On 11/25/2014 05:28 PM, Andreas Arnez wrote:
> +    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));

Not sure under which conditions "len" could be zero here.  If it may,
note that some registers have empty names.  See default_print_registers_info:

      /* If the register name is empty, it is undefined for this
         processor, so don't display anything.  */
      if (gdbarch_register_name (gdbarch, i) == NULL
	  || *(gdbarch_register_name (gdbarch, i)) == '\0')
	continue;

> +      }
> +  }


Thanks,
Pedro Alves

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Provide useful completer for "info registers"
  2014-12-04 17:34   ` Pedro Alves
@ 2014-12-10 17:36     ` Andreas Arnez
  2014-12-10 18:21       ` Pedro Alves
  0 siblings, 1 reply; 10+ messages in thread
From: Andreas Arnez @ 2014-12-10 17:36 UTC (permalink / raw)
  To: Pedro Alves; +Cc: Sergio Durigan Junior, gdb-patches

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.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Provide useful completer for "info registers"
  2014-12-04 17:38 ` Pedro Alves
@ 2014-12-10 17:48   ` Andreas Arnez
  0 siblings, 0 replies; 10+ messages in thread
From: Andreas Arnez @ 2014-12-10 17:48 UTC (permalink / raw)
  To: Pedro Alves; +Cc: gdb-patches

On Thu, Dec 04 2014, Pedro Alves wrote:

> Also, ...
>
> On 11/25/2014 05:28 PM, Andreas Arnez wrote:
>> +    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));
>
> Not sure under which conditions "len" could be zero here.  If it may,
> note that some registers have empty names.  See default_print_registers_info:
>
>       /* If the register name is empty, it is undefined for this
>          processor, so don't display anything.  */
>       if (gdbarch_register_name (gdbarch, i) == NULL
> 	  || *(gdbarch_register_name (gdbarch, i)) == '\0')
> 	continue;
>
>> +      }
>> +  }

Good point.  This is fixed in the new version; registers with empty
names are skipped now.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Provide useful completer for "info registers"
  2014-12-10 17:36     ` Andreas Arnez
@ 2014-12-10 18:21       ` Pedro Alves
  0 siblings, 0 replies; 10+ messages in thread
From: Pedro Alves @ 2014-12-10 18:21 UTC (permalink / raw)
  To: Andreas Arnez; +Cc: Sergio Durigan Junior, gdb-patches

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2014-12-10 18:21 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-11-25 17:28 [PATCH] Provide useful completer for "info registers" Andreas Arnez
2014-11-26 20:54 ` Sergio Durigan Junior
2014-11-26 21:52   ` Sergio Durigan Junior
2014-11-28 18:14   ` Andreas Arnez
2014-11-28 20:39     ` Sergio Durigan Junior
2014-12-04 17:34   ` Pedro Alves
2014-12-10 17:36     ` Andreas Arnez
2014-12-10 18:21       ` Pedro Alves
2014-12-04 17:38 ` Pedro Alves
2014-12-10 17:48   ` Andreas Arnez

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).