Message ID | 87ioi1bs3x.fsf@redhat.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 23670 invoked by alias); 26 Nov 2014 20:54:17 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 23660 invoked by uid 89); 26 Nov 2014 20:54:16 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.1 required=5.0 tests=AWL, BAYES_00, SPF_HELO_PASS, SPF_PASS, T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Wed, 26 Nov 2014 20:54:14 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id sAQKsB30031034 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Wed, 26 Nov 2014 15:54:11 -0500 Received: from localhost (dhcp-10-15-16-169.yyz.redhat.com [10.15.16.169]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id sAQKsAFf027355 (version=TLSv1/SSLv3 cipher=AES128-GCM-SHA256 bits=128 verify=NO); Wed, 26 Nov 2014 15:54:10 -0500 From: Sergio Durigan Junior <sergiodj@redhat.com> To: Andreas Arnez <arnez@linux.vnet.ibm.com> Cc: gdb-patches@sourceware.org Subject: Re: [PATCH] Provide useful completer for "info registers" References: <87h9xnqje8.fsf@br87z6lw.de.ibm.com> X-URL: http://blog.sergiodj.net Date: Wed, 26 Nov 2014 15:54:10 -0500 In-Reply-To: <87h9xnqje8.fsf@br87z6lw.de.ibm.com> (Andreas Arnez's message of "Tue, 25 Nov 2014 18:28:47 +0100") Message-ID: <87ioi1bs3x.fsf@redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes |
Commit Message
Sergio Durigan Junior
Nov. 26, 2014, 8:54 p.m. UTC
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.
Comments
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
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; }