[RFC] fix annoying completion bug on directories

Pedro Alves pedro@codesourcery.com
Sun Jul 6 03:57:00 GMT 2008


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.

<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?

>   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.


>
>   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 ran the testsuite on cygwin, and found one
> change, but in gdb.gdb/selftest.
>   FAIL (timeout) xgdb is at prompt
>
>   I suspect that this is due to a problem with the
> [New thread %s] output, that are now default.
> Because this output appears right after the gdb prompt,
> and thus the match with a gdb prompt exactly at the end
> is not found.

In these cases, in addition to analising the log file, please
rerun the test to confirm it was spurious.  Cygwin testing
is indeed flackier than linux.

Just do make check RUNTESTFLAGS="failingtest.exp" a couple of
times.  If it's always failing now, it's a regression.  If
the failure goes aways, then also retest with with the
patch unapplied.  You should see the flackiness too.  If
not, it may be a regression, but you'll have to judge from
the logs -- it may just be difficult to reproduce flackiness.

>   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"

>
> ChangeLog entry:
>
> 2008-06-06  Pierre Muller  <muller@ics.u-strasbg.fr>
        ^

You have to do something about that calendar :-)

>
> 	* gdb/completer.c: Include gdb_assert.h

Please also update Makefile.in.  Still needed until we have
automatic dependency tracking...

>  /* 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.

>  {
> +  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)
{
  ...
}

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.


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

And please add a comment somewhere describing what this is for.

> +
> +  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).


> +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...

-- 
Pedro Alves



More information about the Gdb-patches mailing list