This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
[RFA] fix annoying completion bug on directories
Thanks again for the comments, Pedro.
> Objet?: Re: [RFC] fix annoying completion bug on directories
>
> 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!
> >
>
> Oh boy, thanks for looking into this!
> This has been annoying me a lot as well.
Does it also deserve a NEWS entry?
> <warning note>
> I went ahead and tried to understand your changes, but beware
> that my readline+GDB foo is low.
> </warning note>
>
> > 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:
> >
>
> ...
>
>
> > This explains why at the second try,
> > the word found by _rl_find_completion_word
> > is now "/usr/inclu".
> >
>
> 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.
>
> 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.
>
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.
> >
>
> Yay!
>
> > 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...
> >
>
> Take a look at the completion.exp test for the test that does:
> send_gdb "file ./gdb.base/complet\t"
>
> 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 <muller@ics.u-strasbg.fr>
> ^
>
> You have to do something about that calendar :-)
I had to compensate for the stuff I tagged as being in December, I guess.
> >
> > * gdb/completer.c: Include gdb_assert.h
>
> Please also update Makefile.in. Still needed until we have
> automatic dependency tracking...
I added this one.
> > /* 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)
>
> 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 = 0;
> > char **list = 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 = 1;
> > + text = line_buffer;
> > + }
> > +
>
> 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:
>
> 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
> char **
> complete_line (const char *text, char *line_buffer, int point)
> {
> complete_or_set_brkchars (text, line_buffer, complete_please);
> }
>
> static char *
> gdb_completion_word_break (void)
> {
> complete_or_set_brkchars (rl_line_buffer,
> rl_line_buffer,
> rl_point, set_brkchars_please);
> }
>
>
> > - rl_completer_word_break_characters =
> > + current_gdb_completer_word_break_characters =
> > gdb_completer_file_name_break_characters;
> > }
> > -
> > + rl_completer_word_break_characters =
> > + current_gdb_completer_word_break_characters;
> > return list;
>
> 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:
>
> current_gdb_completer_word_break_characters
> = rl_completer_word_break_characters;
>
> at the end of complete_line.
>
I totally removed that new variable,
as it wasn't really useful.
> > }
> >
> > +static char *
> > +gdb_completion_word_break ()
> ^
> (void)
>
> > +{
> > + gdb_assert (complete_line (NULL, rl_line_buffer, rl_point) ==
> NULL);
>
> 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 = complete_line (NULL, rl_line_buffer, rl_point);
> gdb_assert (line == NULL);
Didn't know that rule, code changed accordingly.
> 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;
> > +}
>
> from readline.c:complete.c:
>
> char
> _rl_find_completion_word (fp, dp)
> int *fp, *dp;
> {
> int scan, end, found_quote, delimiter, pass_next, isbrk;
> char quote_char, *brkchars;
>
> end = rl_point;
> found_quote = delimiter = 0;
> quote_char = '\0';
>
> brkchars = 0;
> if (rl_completion_word_break_hook)
> brkchars = (*rl_completion_word_break_hook) ();
> if (brkchars == 0)
> brkchars = rl_completer_word_break_characters;
>
>
> 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?
>
> Also, doesn't filename_completer always get text == word now?
> If so, there's some code in there that turns dead (both the
> if text < word, and the else clauses).
>
You seem to be right here, but I would prefer not
to touch this for now...
> > +void
> > +_initialize_completer (void)
> > +{
> > + rl_completion_word_break_hook = gdb_completion_word_break;
>
> 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 <muller@ics.u-strasbg.fr>
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,
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=0.
(gdb_completion_word_break): New static function.
Calls complete_line_1 with only_brkchars=1.
* top.c (init_main): Set rl_completion_word_break_hook to
gdb_completion_word_break_characters;
* Makefile (completer.o): Add gdb_assert_h dependency.
Index: gdb/Makefile.in
===================================================================
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
===================================================================
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 *,
extern char *get_gdb_completer_quote_characters (void);
+extern char *gdb_completion_word_break_characters (void);
+
/* Exported to linespec.c */
extern char *skip_quoted_chars (char *, char *, char *);
Index: gdb/completer.c
===================================================================
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"
#include "cli/cli-decode.h"
@@ -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=1, called by
+ gdb_completion_word_break_characters function, is used to
+ 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=0, called by complete_line function,
+ is used to get the list of posible completions. */
+
+static char **
+complete_line_1 (const char *text, char *line_buffer, int point,
+ int only_brkchars)
{
char **list = 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 =
current_language->la_word_break_characters();
@@ -547,12 +547,14 @@ complete_line (const char *text, char *l
This we can deal with. */
if (result_list)
{
- list = complete_on_cmdlist (*result_list->prefixlist, p,
- word);
+ if (!only_brkchars)
+ list = complete_on_cmdlist (*result_list->prefixlist, p,
+ word);
}
else
{
- list = complete_on_cmdlist (cmdlist, p, word);
+ if (!only_brkchars)
+ list = 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 = complete_on_cmdlist (*c->prefixlist, p, word);
+ if (!only_brkchars)
+ list = complete_on_cmdlist (*c->prefixlist, p, word);
/* 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 = complete_on_enum (c->enums, p, word);
+ if (!only_brkchars)
+ list = complete_on_enum (c->enums, p, word);
rl_completer_word_break_characters =
gdb_completer_command_word_break_characters;
}
@@ -621,7 +625,8 @@ complete_line (const char *text, char *l
p--)
;
}
- list = (*c->completer) (p, word);
+ if (!only_brkchars)
+ list = (*c->completer) (p, word);
}
}
else
@@ -642,7 +647,8 @@ complete_line (const char *text, char *l
break;
}
- list = complete_on_cmdlist (result_list, q, word);
+ if (!only_brkchars)
+ list = complete_on_cmdlist (result_list, q, word);
/* 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 = complete_on_enum (c->enums, p, word);
+ if (!only_brkchars)
+ list = complete_on_enum (c->enums, p, word);
}
else
{
@@ -687,7 +694,8 @@ complete_line (const char *text, char *l
p--)
;
}
- list = (*c->completer) (p, word);
+ if (!only_brkchars)
+ list = (*c->completer) (p, word);
}
}
}
@@ -695,6 +703,33 @@ complete_line (const char *text, char *l
return list;
}
+/* Get the list of chars that are considered as word breaks
+ for the current command. */
+
+char *
+gdb_completion_word_break_characters (void)
+{
+ char ** list;
+ list = complete_line_1 (rl_line_buffer, rl_line_buffer, rl_point, 1);
+ gdb_assert (list == 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);
+}
+
/* 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
===================================================================
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 = 0;
/* Setup important stuff for command line editing. */
+ rl_completion_word_break_hook = gdb_completion_word_break_characters;
rl_completion_entry_function = readline_line_completion_function;
rl_completer_word_break_characters = default_word_break_characters ();
rl_completer_quote_characters = get_gdb_completer_quote_characters ();
gdb/testsuite ChangeLog entry:
2008-07-08 Pierre Muller <muller@ics.u-strasbg.fr>
* gdb.base/completion.exp: Add test for completer problem for
filename completion on the first try.
Index: gdb.base/completion.exp
===================================================================
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}
+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
# regexp operator. We use string_to_regexp for this purpose.
-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"
-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 $"
+ { fail "complete-command 'dir ${srcdir}/gdb.base/complet'" }
+ timeout
+ { fail "(timeout) complete-command 'dir
${srcdir}/gdb.base/complet'" }
}
+# 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'" }
}
+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 '"
}
}
# Restore globals modified in this test...