Re: [PATCH 11/40] Introduce class completion_tracker & rewrite completion<->readline interaction

[Simon Marchi <simon dot marchi at ericsson dot com>]

On 2017-06-02 08:22 AM, Pedro Alves wrote:
> This patch reworkds the whole completion machinery, and prepares it
> for later enhancements.
> 
> Adds a new "completion_tracker" class that is meant to hold everything
> about the state of the current completion operation.
> 
> This class now has the responsibility of tracking the list of
> completion matches, and checking whether the max completions limit has
> been reached.  You can look at this as this patch starting out by
> C++fying the existing "completion_tracker" in symtab.c (it's just an
> htab_t typedef currently), moving it to completer.h/c, and then making
> it a class/generalizing/enhancing it.
> 
> Unlike with the current tracking, completion_tracker now checks
> whether the limit has been reached on each completion match list
> insertion.  This both simplifies the max-completions handling code
> (maybe_add_completion_enum is gone, for example), and is a
> prerequisite for follow up patches.
> 
> The current completion_tracker is only used for symbol completions,
> and the symbol code gets at the current instance via globals.  This
> patch cleans that up by adding a completion_tracker reference to the
> signature of the completion functions, and passing the tracker around
> everywhere necessary.
> 
> Then, the patch changes how the completion match list is handed over
> to readline.  Currently, we're using the rl_completion_entry_function
> readline entry point, and the patch switches to
> rl_attempted_completion_function.  A following patch will want to let
> GDB itself decide the common completion prefix between all matches
> (what readline calls the "lowest common denominator"), instead of
> having readline compute it, and that's not possible with the
> rl_completion_entry_function entry point.  Also,
> rl_attempted_completion_function lets GDB hand over the match list to
> readline as an array in one go instead of passing down matches one by
> one, so from that angle it's a nicer entry point anyway.
> 
> Lastly, the patch catches exceptions around the readline entry points,
> because we can't let C++ exceptions cross readline.  We handle that in
> the readline input entry point, but the completion entry point isn't
> guarded, so GDB can abort if completion throws.  E.g., in current
> master:
> 
>  (gdb) b -function "fun<tab>
>  terminate called after throwing an instance of 'gdb_exception_RETURN_MASK_ERROR'
>  Aborted (core dumped)
> 
> This patch fixes that.  This will be exercised in the new tests added
> later on in the series.

Hi Pedro,

Sorry for bringing this thread from the dead...

We are trying to migrate to GDB 8.1, and noticed a behavior change in
the Python API starting with this patch.  I'd like to know if this change
was intended (probably not since I don't see it mentioned in the commit log),
if we should consider it a bug because it broke API, and if we should try
to revert to the previous behavior in GDB 8.1.1.

The complete method of a Python command is called twice, once to deduce the
break characters and the second time to do the actual completion.  I am using
the following script to do my tests:

---- 8< ----

class MyCommand(gdb.Command):
    def __init__(self):
        gdb.Command.__init__(self,'mycmd',gdb.COMMAND_USER)

    def invoke(self,argument,from_tty):
        raise gdb.GdbError('not implemented')

    def complete(self,text,word):
        print('\ntext={}, word={}\n'.format(text, word))
        return ['allo', 'bonjour']

MyCommand()

---- 8< ----

And I try the completion like this:

  (gdb) mycmd hello you<tab>

With 7.12 (probably 8.0 too), the two parameters (text and word) contain this:

  break chars phase: text=hello you, word=mycmd hello you
  completion phase:  text=hello you, word=you

And with 8.1:

  break chars phase: text=hello you, word=None
  completion phase:  text=hello you, word=you

The difference is that during the break chars phase, word is None instead of
containing the complete command line.  With 7.12, if you try this

  (gdb) mycmd hello<tab> you

word has an empty or random value, or GDB segfaults.  If you put a lot of
characters after (hello<tab> youuuuuuuuuuuuuuuuuuu) it has a greater chance
of segfaulting.  With 8.1 it does the right thing.

I think the new behavior makes more sense, because during the break chars
phase, we have not split the last word from the line yet (since we don't
know yet where to split, that's what we're trying to figure out).  The
documentation [1] says:

 word holds the last word of the command line; this is computed using a
 word-breaking heuristic.

So the previous behavior was probably buggy, but it still represents
an API break, that's why I'm asking.

At least, I think we should improve the Python doc to say that

 - word can be None (what should the user code return in that case?)
 - if word is not None, it is computed based on what the first invocation returned

What do you think?

Simon

[1] https://sourceware.org/gdb/onlinedocs/gdb/Commands-In-Python.html#index-completion-of-Python-commands