From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 27777 invoked by alias); 8 Jul 2008 22:16:44 -0000 Received: (qmail 27763 invoked by uid 22791); 8 Jul 2008 22:16:40 -0000 X-Spam-Check-By: sourceware.org Received: from mailhost.u-strasbg.fr (HELO mailhost.u-strasbg.fr) (130.79.200.156) by sourceware.org (qpsmtpd/0.31) with ESMTP; Tue, 08 Jul 2008 22:16:16 +0000 Received: from baal.u-strasbg.fr (baal.u-strasbg.fr [IPv6:2001:660:2402::41]) by mailhost.u-strasbg.fr (8.14.2/jtpda-5.5pre1) with ESMTP id m68MG8In073342 ; Wed, 9 Jul 2008 00:16:08 +0200 (CEST) Received: from mailserver.u-strasbg.fr (ms2.u-strasbg.fr [IPv6:2001:660:2402::142]) by baal.u-strasbg.fr (8.14.0/jtpda-5.5pre1) with ESMTP id m68MG86A035453 ; Wed, 9 Jul 2008 00:16:08 +0200 (CEST) Received: from d620muller ([130.79.244.152]) by mailserver.u-strasbg.fr (8.13.8/jtpda-5.5pre1) with ESMTP id m68MG1ri008692 ; Wed, 9 Jul 2008 00:16:08 +0200 (CEST) From: "Pierre Muller" To: "'Pedro Alves'" , References: <000901c8def8$137b1bf0$3a7153d0$@u-strasbg.fr> <200807060456.49298.pedro@codesourcery.com> In-Reply-To: <200807060456.49298.pedro@codesourcery.com> Subject: [RFA] fix annoying completion bug on directories Date: Tue, 08 Jul 2008 22:16:00 -0000 Message-ID: <000001c8e148$375ac690$a61053b0$@u-strasbg.fr> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable X-Mailer: Microsoft Office Outlook 12.0 Content-Language: en-us X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.0 (mailhost.u-strasbg.fr [IPv6:2001:660:2402::156]); Wed, 09 Jul 2008 00:16:08 +0200 (CEST) X-Virus-Status: Clean Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2008-07/txt/msg00120.txt.bz2 Thanks again for the comments, Pedro. > Objet=A0: Re: [RFC] fix annoying completion bug on directories >=20 > A Sunday 06 July 2008 00:37:22, Pierre Muller wrote: > > I was always annoyed by a small but annoying bug in the > > gdb TAB completion for directories. > > > > If you start gdb without args and type: > > "(gdb) file /usr/inclu" > > and you now type TAB to get the completion, > > you would expect to get > > "(gdb) file /usr/include/" > > but instead you will get > > "(gdb) file /usr/include " > > but if you delete the "de " and type TAB char > > again, you will get the correct answer! > > >=20 > Oh boy, thanks for looking into this! > This has been annoying me a lot as well. Does it also deserve a NEWS entry? =20 > > I went ahead and tried to understand your changes, but beware > that my readline+GDB foo is low. > >=20 > > This all looked like some wrongly set parameter... > > > > It took me a while to figure out what is > > going on and why this bug appears: > > >=20 > ... >=20 >=20 > > This explains why at the second try, > > the word found by _rl_find_completion_word > > is now "/usr/inclu". > > >=20 > Urgl. This means that finding the completion word is > always somewhat broken on the next command, when the next > command should use a different word break character > list? That's what I understood, but I did not really check. > > As the code is rather convoluted, I tried to just > > created a "special" case for which only this variable > > would be set, without really trying to find > > the completions. >=20 > Did you consider breaking complete_line in two? There's more > you could skip that just the completion calls. > Hmm, doesn't look like it would be a maintainable result. Basically > the parsing, command lookups and most of the comments would > have to be duplicated, or a weird abstraction would have to > be employed. Not sure if the result would be better at all. >=20 As you suggested, I put the code into a new static function named complete_line_1, see the new patch version below. > > > > Luckily, there is a hook function inside > > _rl_find_completion_word that allows to > > change the default value of the list of chars > > considered as marking word start/end. > > >=20 > Yay! >=20 > > I found that this works nicely... > > I reran the tests on cygwin without any changes, and moved the discussion about the spurious test results to another email. > > I found no change in the tests dealing with completion, > > but I also don't really know how to add such a test... > > >=20 > Take a look at the completion.exp test for the test that does: > send_gdb "file ./gdb.base/complet\t" >=20 > Maybe you could do something similar? Event this fails > to complete the '/' the first time without your patch: > send_gdb "dir ..\t" I tried to create something... > > > > ChangeLog entry: > > > > 2008-06-06 Pierre Muller > ^ >=20 > You have to do something about that calendar :-) I had to compensate for the stuff I tagged as being in December, I guess. =20 > > > > * gdb/completer.c: Include gdb_assert.h >=20 > Please also update Makefile.in. Still needed until we have > automatic dependency tracking... =20 I added this one. =20 > > /* When completing on command names, we remove '-' from the list of > > word break characters, since we use it in command names. If the > > readline library sees one in any of the current completion > strings, > > @@ -472,20 +475,29 @@ command_completer (char *text, char *wor > > char ** > > complete_line (const char *text, char *line_buffer, int point) >=20 > Please add somewhere describing the change you're making, and why > it's needed. I hope that the new patch answers this too. > > { > > + int only_brkchars =3D 0; > > char **list =3D NULL; > > char *tmp_command, *p; > > + > > /* Pointer within tmp_command which corresponds to text. */ > > char *word; > > struct cmd_list_element *c, *result_list; > > > > + /* If text is NULL, we only want to set the variable > > + current_gdb_completer_word_break_characters. */ > > + if (!text) > > + { > > + only_brkchars =3D 1; > > + text =3D line_buffer; > > + } > > + >=20 > Another alternative would be to add a new parameter to > the function, rename it, and add a new complete_line function > as wrapper to the old one: >=20 > char ** > complete_or_set_brkchars (const char *text, char *line_buffer, > int point, int what_to_do) > { > ... > } That's what I did, but I called it complete_line_1 =20 > char ** > complete_line (const char *text, char *line_buffer, int point) > { > complete_or_set_brkchars (text, line_buffer, complete_please); > } >=20 > static char * > gdb_completion_word_break (void) > { > complete_or_set_brkchars (rl_line_buffer, > rl_line_buffer, > rl_point, set_brkchars_please); > } >=20 >=20 > > - rl_completer_word_break_characters =3D > > + current_gdb_completer_word_break_characters =3D > > gdb_completer_file_name_break_characters; > > } > > - > > + rl_completer_word_break_characters =3D > > + current_gdb_completer_word_break_characters; > > return list; >=20 > You changed a bunch of rl_completer_word_break_characters > to current_gdb_completer_word_break_characters, only to in > the end set them to be the same. If you do it the other way > around, you'd only be adding a single: >=20 > current_gdb_completer_word_break_characters > =3D rl_completer_word_break_characters; >=20 > at the end of complete_line. >=20 I totally removed that new variable, as it wasn't really useful. > > } > > > > +static char * > > +gdb_completion_word_break () > ^ > (void) >=20 > > +{ > > + gdb_assert (complete_line (NULL, rl_line_buffer, rl_point) =3D=3D > NULL); >=20 > Please, no side effects in assert statements. We always build > with them on, but it's bad practice to rely on that. If you want > to keep the assert, do it like: > > char **list =3D complete_line (NULL, rl_line_buffer, rl_point); > gdb_assert (line =3D=3D NULL); Didn't know that rule, code changed accordingly. =20 > And please add a comment somewhere describing what this is for. Done in the comments above complete_line_1 > > + > > + return current_gdb_completer_word_break_characters; > > +} >=20 > from readline.c:complete.c: >=20 > char > _rl_find_completion_word (fp, dp) > int *fp, *dp; > { > int scan, end, found_quote, delimiter, pass_next, isbrk; > char quote_char, *brkchars; >=20 > end =3D rl_point; > found_quote =3D delimiter =3D 0; > quote_char =3D '\0'; >=20 > brkchars =3D 0; > if (rl_completion_word_break_hook) > brkchars =3D (*rl_completion_word_break_hook) (); > if (brkchars =3D=3D 0) > brkchars =3D rl_completer_word_break_characters; >=20 >=20 > It looks like you could get rid of > current_gdb_completer_word_break_characters > entirely, and return either NULL or rl_completer_word_break_characters > in your new hook; or if keeping current_gdb_comp... not set > rl_completer_word_break_characters in complete_line at all? >=20 > Also, doesn't filename_completer always get text =3D=3D word now? > If so, there's some code in there that turns dead (both the > if text < word, and the else clauses). >=20 =20 You seem to be right here, but I would prefer not=20 to touch this for now... > > +void > > +_initialize_completer (void) > > +{ > > + rl_completion_word_break_hook =3D gdb_completion_word_break; >=20 > Did you consider putting this in init_main near the other rl_ > initializations? OTHO, rl_completer_word_break_characters > was indeed already mostly only tweaked in this file, and it > goes in hand with this hook... Done as you suggested. Pierre Muller Pascal language support maintainer for GDB gdb ChangeLog entry: 2008-07-08 Pierre Muller Fix completer problem for filename completion on the first try. * gdb/completer.h (gdb_completion_word_break_characters): New function. * gdb/completer.c: Include gdb_assert.h. (complete_line_1): New static function,=20 contains most of previous complete_line implementation, extended to handle only setting of rl_completer_word_break_characters. (complete_line): Now simply calls complete_line_1 with only_brkchars=3D0. (gdb_completion_word_break): New static function. Calls complete_line_1 with only_brkchars=3D1. * top.c (init_main): Set rl_completion_word_break_hook to gdb_completion_word_break_characters; * Makefile (completer.o): Add gdb_assert_h dependency. =09 =09 Index: gdb/Makefile.in =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D RCS file: /cvs/src/src/gdb/Makefile.in,v retrieving revision 1.1029 diff -u -p -r1.1029 Makefile.in --- gdb/Makefile.in 27 Jun 2008 11:54:21 -0000 1.1029 +++ gdb/Makefile.in 8 Jul 2008 19:36:43 -0000 @@ -2009,7 +2009,7 @@ complaints.o: complaints.c $(defs_h) $(c $(command_h) $(gdbcmd_h) completer.o: completer.c $(defs_h) $(symtab_h) $(gdbtypes_h) $(expression_h) \ $(filenames_h) $(language_h) $(cli_decode_h) $(gdbcmd_h) \ - $(readline_h) $(completer_h) + $(readline_h) $(completer_h) $(gdb_assert_h) copying.o: copying.c $(defs_h) $(command_h) $(gdbcmd_h) corefile.o: corefile.c $(defs_h) $(gdb_string_h) $(inferior_h) $(symtab_h) \ $(command_h) $(gdbcmd_h) $(bfd_h) $(target_h) $(gdbcore_h) \ Index: gdb/completer.h =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D RCS file: /cvs/src/src/gdb/completer.h,v retrieving revision 1.14 diff -u -p -r1.14 completer.h --- gdb/completer.h 6 Jun 2008 20:58:08 -0000 1.14 +++ gdb/completer.h 8 Jul 2008 19:36:43 -0000 @@ -33,6 +33,8 @@ extern char **command_completer (char *, =20 extern char *get_gdb_completer_quote_characters (void); =20 +extern char *gdb_completion_word_break_characters (void); + /* Exported to linespec.c */ =20 extern char *skip_quoted_chars (char *, char *, char *); Index: gdb/completer.c =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D RCS file: /cvs/src/src/gdb/completer.c,v retrieving revision 1.26 diff -u -p -r1.26 completer.c --- gdb/completer.c 9 Jun 2008 19:25:14 -0000 1.26 +++ gdb/completer.c 8 Jul 2008 19:36:43 -0000 @@ -22,6 +22,7 @@ #include "expression.h" #include "filenames.h" /* For DOSish file names. */ #include "language.h" +#include "gdb_assert.h" =20 #include "cli/cli-decode.h" =20 @@ -458,19 +459,19 @@ command_completer (char *text, char *wor "file Make" "file" (word break hard to screw up here) "file ../gdb.stabs/we" "ird" (needs to not break word at slash) */ - -/* Generate completions all at once. Returns a NULL-terminated array - of strings. Both the array and each element are allocated with - xmalloc. It can also return NULL if there are no completions. - - TEXT is the caller's idea of the "word" we are looking at. - - LINE_BUFFER is available to be looked at; it contains the entire text - of the line. POINT is the offset in that line of the cursor. You - should pretend that the line ends at POINT. */ - -char ** -complete_line (const char *text, char *line_buffer, int point) +/* Completion is done in two parts: + - first phase, with only_brkchars=3D1, called by + gdb_completion_word_break_characters function, is used to=20 + determine the correct set of chars that are word delimiters + depending gon the current command in line_buffer. + No completion list should be generated; the return value should be NULL. + This is checked by an assertion in that function. + -second phase, with only_brkchars=3D0, called by complete_line function, + is used to get the list of posible completions. */ +=20 +static char ** +complete_line_1 (const char *text, char *line_buffer, int point,=20 + int only_brkchars) { char **list =3D NULL; char *tmp_command, *p; @@ -484,7 +485,6 @@ complete_line (const char *text, char *l functions, which can be any string) then we will switch to the special word break set for command strings, which leaves out the '-' character used in some commands. */ - rl_completer_word_break_characters =3D current_language->la_word_break_characters(); =20 @@ -547,12 +547,14 @@ complete_line (const char *text, char *l This we can deal with. */ if (result_list) { - list =3D complete_on_cmdlist (*result_list->prefixlist, p, - word); + if (!only_brkchars) + list =3D complete_on_cmdlist (*result_list->prefixlist, p, + word); } else { - list =3D complete_on_cmdlist (cmdlist, p, word); + if (!only_brkchars) + list =3D complete_on_cmdlist (cmdlist, p, word); } /* Ensure that readline does the right thing with respect to inserting quotes. */ @@ -576,7 +578,8 @@ complete_line (const char *text, char *l { /* It is a prefix command; what comes after it is a subcommand (e.g. "info "). */ - list =3D complete_on_cmdlist (*c->prefixlist, p, word); + if (!only_brkchars) + list =3D complete_on_cmdlist (*c->prefixlist, p, word); =20 /* Ensure that readline does the right thing with respect to inserting quotes. */ @@ -585,7 +588,8 @@ complete_line (const char *text, char *l } else if (c->enums) { - list =3D complete_on_enum (c->enums, p, word); + if (!only_brkchars) + list =3D complete_on_enum (c->enums, p, word); rl_completer_word_break_characters =3D gdb_completer_command_word_break_characters; } @@ -621,7 +625,8 @@ complete_line (const char *text, char *l p--) ; } - list =3D (*c->completer) (p, word); + if (!only_brkchars) + list =3D (*c->completer) (p, word); } } else @@ -642,7 +647,8 @@ complete_line (const char *text, char *l break; } =20 - list =3D complete_on_cmdlist (result_list, q, word); + if (!only_brkchars) + list =3D complete_on_cmdlist (result_list, q, word); =20 /* Ensure that readline does the right thing with respect to inserting quotes. */ @@ -662,7 +668,8 @@ complete_line (const char *text, char *l } else if (c->enums) { - list =3D complete_on_enum (c->enums, p, word); + if (!only_brkchars) + list =3D complete_on_enum (c->enums, p, word); } else { @@ -687,7 +694,8 @@ complete_line (const char *text, char *l p--) ; } - list =3D (*c->completer) (p, word); + if (!only_brkchars) + list =3D (*c->completer) (p, word); } } } @@ -695,6 +703,33 @@ complete_line (const char *text, char *l return list; } =20 +/* Get the list of chars that are considered as word breaks + for the current command. */=20 + +char * +gdb_completion_word_break_characters (void) +{ + char ** list; + list =3D complete_line_1 (rl_line_buffer, rl_line_buffer, rl_point, 1); + gdb_assert (list =3D=3D NULL); + return rl_completer_word_break_characters; +} +/* Generate completions all at once. Returns a NULL-terminated array + of strings. Both the array and each element are allocated with + xmalloc. It can also return NULL if there are no completions. + + TEXT is the caller's idea of the "word" we are looking at. + + LINE_BUFFER is available to be looked at; it contains the entire text + of the line. POINT is the offset in that line of the cursor. You + should pretend that the line ends at POINT. */ + +char ** +complete_line (const char *text, char *line_buffer, int point) +{ + return complete_line_1 (text, line_buffer, point, 0); +}=20 + /* Generate completions one by one for the completer. Each time we are called return another potential completion to the caller. line_completion just completes on commands or passes the buck to the Index: gdb/top.c =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D RCS file: /cvs/src/src/gdb/top.c,v retrieving revision 1.143 diff -u -p -r1.143 top.c --- gdb/top.c 10 Jun 2008 11:57:28 -0000 1.143 +++ gdb/top.c 8 Jul 2008 19:36:43 -0000 @@ -1524,6 +1524,7 @@ init_main (void) write_history_p =3D 0; =20 /* Setup important stuff for command line editing. */ + rl_completion_word_break_hook =3D gdb_completion_word_break_characters; rl_completion_entry_function =3D readline_line_completion_function; rl_completer_word_break_characters =3D default_word_break_characters (); rl_completer_quote_characters =3D get_gdb_completer_quote_characters (); gdb/testsuite ChangeLog entry: 2008-07-08 Pierre Muller * gdb.base/completion.exp: Add test for completer problem for=20 filename completion on the first try. Index: gdb.base/completion.exp =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D RCS file: /cvs/src/src/gdb/testsuite/gdb.base/completion.exp,v retrieving revision 1.31 diff -u -r1.31 completion.exp --- gdb.base/completion.exp 9 Jun 2008 19:25:15 -0000 1.31 +++ gdb.base/completion.exp 8 Jul 2008 22:13:16 -0000 @@ -712,23 +712,62 @@ set fullsrcdir [pwd] cd ${mydir} =20 +gdb_exit +gdb_start +gdb_reinitialize_dir $srcdir/$subdir +gdb_load ${binfile} + # If the directory name contains a '+' we must escape it, adding a backslash. # If not, the test below will fail because it will interpret the '+' as a= =20 # regexp operator. We use string_to_regexp for this purpose. =20 -gdb_test "cd ${fullsrcdir}" \ - "Working directory [string_to_regexp ${fullsrcdir}].*" \ - "cd to \${srcdir}" +# Test the file correctly adds a '/' at end of completion +# even the first time a file completion is used + +send_gdb "dir ${srcdir}/gdb.bas\t" + +gdb_expect { + -re "^dir ${srcdir}/gdb.base/$" { + pass "complete dir ${srcdir}/gdb.base/" + } + -re "^dir ${srcdir}/gdb.base $" { + send_gdb "\b\b\t" + gdb_expect { + -re "(.*e/)$" { + set current_line "$expect_out(1,string)" + fail "wrong first, correct second completion for command '${current_line}'" + } + timeout { fail "dir ${srcdir}/gdb.base/ (timeout 2)" } + } + } + timeout { fail "dir ${srcdir}/gdb.base/ (timeout)" } +} + + +send_gdb "complet\t" =20 -send_gdb "complete file ./gdb.base/compl\n" sleep 1 + gdb_expect { - -re "file ./gdb.base/completion\\.exp.*$gdb_prompt $" - { pass "complete-command 'file ./gdb.base/compl'"} - -re ".*$gdb_prompt $" { fail "complete-command 'file ./gdb.base/compl'" } - timeout { fail "(timeout) complete-command 'file ./gdb.base/compl'" } + -re ".*completion\\.exp $" + { pass "complete-command 'dir ${srcdir}/gdb.base/complet'"} + -re ".*$gdb_prompt $"=20 + { fail "complete-command 'dir ${srcdir}/gdb.base/complet'" } + timeout=20 + { fail "(timeout) complete-command 'dir ${srcdir}/gdb.base/complet'" } } =20 +# Press return and don't care about errors + +gdb_test " " ".*" "Back to normal" + +# Change to testsuite source directory +gdb_test "cd ${fullsrcdir}" \ + "Working directory [string_to_regexp ${fullsrcdir}].*" \ + "cd to \${srcdir}" + + + send_gdb "file ./gdb.base/complet\t" sleep 1 gdb_expect { @@ -747,6 +786,8 @@ timeout { fail "(timeout) complete 'file ./gdb.base/complet'" } } =20 +gdb_test " " ".*" "Back to normal" + send_gdb "info func marke\t" sleep 1 gdb_expect { @@ -780,15 +821,15 @@ { send_gdb "\n" gdb_expect { -re "Requires an argument.*child.*parent.*$gdb_prompt $"\ - { pass "complete 'set follow-fork-mode'"} + { pass "complete 'set follow-fork-mode '"} -re "Ambiguous item \"\"\\..*$gdb_prompt $"\ { pass "complete 'set follow-fork-mode'"} - -re ".*$gdb_prompt $" { fail "complete 'set follow-fork-mode'"} - timeout {fail "(timeout) complete 'set follow-fork-mode'"} + -re ".*$gdb_prompt $" { fail "complete 'set follow-fork-mode '"} + timeout {fail "(timeout) complete 'set follow-fork-mode '"} } } - -re ".*$gdb_prompt $" { fail "complete 'set follow-fork-mode'" } - timeout { fail "(timeout) complete 'set follow-fork-mode'" } + -re ".*$gdb_prompt $" { fail "complete 'set follow-fork-mode '" } + timeout { fail "(timeout) complete 'set follow-fork-mode '" } } =20 # Restore globals modified in this test...