From patchwork Wed Nov 26 20:54:10 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Sergio Durigan Junior X-Patchwork-Id: 3958 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: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , 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 To: Andreas Arnez 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 X-IsSubscribed: yes 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. 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; }