Re: [PATCH 2/6] gdb: move display of completion results into completion_result class

[Andrew Burgess <aburgess@redhat.com>]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Andrew Burgess <aburgess@redhat.com>
>> Cc: gdb-patches@sourceware.org
>> Date: Fri, 12 Apr 2024 18:24:31 +0100
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > Next, if 'file' indeed expects shell file-name semantics, then the
>> > question is: which shell?  Should the above behave differently on, for
>> > example, MS-Windows, since file-name quoting there works differently?
>> > For example, escaping whitespace with a backslash will not work on
>> > Windows.
>> 
>> I think calling this "shell file-name semantics" makes this change seem
>> worse than it is.  If we want to call it anything, then it should be
>> called gdb-shell semantics.
>
> What is "gdb-shell"?  Saying "gdb-shell semantics" is only useful if
> the term "gdb-shell" is known and understood by many.  Otherwise, we
> should not use this terminology, because it doesn't explain anything.

It's a name I made up for the thing you type at after a (gdb) prompt.

The point is, it's not a "shell", it's GDB.  It has its own rules.

>
>> > And last, but not least: the manual says about the 'complete' command:
>> >
>> >      This is intended for use by GNU Emacs.
>> >
>> > So one other thing we should consider is how Emacs handles these cases
>> > and what it expects from GDB in response.
>> 
>> Happy to test such a thing.  Can you point me at any instruction/guides
>> for how to trigger completion via emacs?
>
> Invoke either "M-x gdb" or "M-x gud-gdb" from Emacs, and then, after
> the debugging session starts, type TAB to complete file names in
> commands that accept file names.

Thank you.  I got this working and did some tests, I'll write up my
results at the end of this email.

>
>> My hope is that this change will fix things rather than break them.
>> Previously the 'complete' command would output a partial completion that
>> was invalid, that is, if the user passes emacs a short string and then
>> triggers completion, GDB would provide a longer string which was
>> invalid.  If emacs then feeds that longs string back to GDB the command
>> isn't going to work.  After this change that should no longer be the
>> case.
>
> I hope you are right.  Let's see.  In general, Emacs doesn't need any
> quoting when it receives file names from a sub-process (in this case,
> from GDB).

See below.

>
>> >> The problem Andrew is trying to solve is that the current completer for
>> >> this command completes to something that is not a valid input.
>> >
>> > My point is that we need to have a clear idea what is a "valid input"
>> > before we decide whether the current behavior is invalid, and if so,
>> > how to fix it.
>> 
>> I do disagree with the "decide whether the current behaviour is invalid"
>> part of this text.  Completion of files containing whitespace doesn't
>> work right now, so I'd suggest the current behaviour is self-evidently
>> invalid.  But ...
>> 
>> >                 IOW, we should step back and discuss the valid
>> > behavior first.
>> 
>> I'm totally happy to take suggestions on what the working behaviour
>> should look like.
>
> I made one alternative suggestion about completing a file name that
> begins with a quote, for example.  Whether it is a good suggestion
> depends on what we want the behavior to be, so that should preferably
> be discussed before deciding on the solution.

OK.  But right now GDB uses readline for its shell/session.  And
readline completion works in a particular way.  We (GDB) don't get much
say over that unless we want to start seriously hacking on readline.

These patches aren't me coming up with some radical new approach and
trying to push it onto GDB.  This is just me connecting up readline and
GDB a little more fully, and letting readline do its thing.  How tab
completion works is really a readline feature.

Now where I have "invented" things, to some degree, is with the
'complete' command, which, as you point out, emacs uses.  My assumption
was that the 'complete' was used like this:

  1. User types a partial command in emacs buffer,

  2. User invokes completion,

  3. Emacs grabs the partial command and sends it to GDB's 'complete'
  command,

  4. GDB returns a set of partial completions,

  5. Emacs presents each completion to the user, and allows the user to
  select one,

  6. Emacs inserts the selected text into the buffer, extending the
  partially completed command,

  7. User hits <return> and the command is executed.

Having had a dig through gdb-mi.el and gud.el (from the emacs source) I
think I was pretty much correct with my assumptions.

And all the changes I've made to the 'complete' command have been
intended to support this use case, e.g. adding the escapes to the
'complete' command output.  Notice that readline by default does NOT add
the escaping, e.g.:

    (gdb) file /tmp/qqq/aa<tab>
        => file /tmp/qqq/aa\ <tab><tab>
    aa bb/ aa cc/

Here the ' ' in 'aa bb/' and 'aa cc/' is not escaped.  However, I made
sure that if I do:

    (gdb) complete file /tmp/eee/aa
    file /tmp/eee/aa\ bb/
    file /tmp/eee/aa\ cc/
    (gdb)

Then the whitespace is escaped, because if it wasn't and emacs just
dropped the text in without the escaping, then GDB would see two
separate words and do the wrong thing.

OK, so now the bad news.

My patched GDB doesn't "just work" with emacs.

Remember though, an unpatched GDB doesn't work either when trying to
complete a filename with spaces in, so it's not that I've made things
worse.

I tracked at least one bug down to this function in gud.el:

  (defun gud-gdb-completion-at-point ()
    "Return the data to complete the GDB command before point."
    (let ((end (point))
          (start
           (save-excursion
             (skip-chars-backward "^ " (comint-line-beginning-position))
             (point))))
      ;; FIXME: `gud-gdb-run-command-fetch-lines' has some nasty side-effects on
      ;; the buffer (via `gud-delete-prompt-marker'): it removes the prompt and
      ;; then re-adds it later, thus messing up markers and overlays along the
      ;; way (bug#18282).
      ;; We use an "insert-before" marker for `start', since it's typically right
      ;; after the prompt, which works around the problem, but is a hack (and
      ;; comes with other downsides, e.g. if completion adds text at `start').
      (list (copy-marker start t) end
            (completion-table-dynamic
             (apply-partially gud-gdb-completion-function
                              (buffer-substring (comint-line-beginning-position)
                                                start))))))
                                                
This is trying to figure out everything before the completion word on
the current line and doing the apply-partial with this context on
gud-gdb-completion-function.

So if we are completing 'file /tmp/abc<tab>' then we'd end up doing:

  (apply-partial gud-gdb-completion-function "file ")

The completion context is calculated as running from the start of the
line up to 'start', which is calculated in the 'let'.  And 'start' is
calculated by starting from 'point' and moving backward to the first
whitespace.

That's not going to work for completing any line that contains a space.

I think this function would need to get smarter to understand about
escaping and quoting.

And, finally, some good news.

If a filename _doesn't_ include any spaces, then as far as I can tell,
from my limited testing, everything works fine in emacs, and is now
improved.

On an unpatched GDB, given a file /tmp/qqq/f1, within emacs:

  (gdb) file /tmp/qq<tab>
     => file /tmp/qqq<tab>
     *Sole Completion*

That's it.  At this point emacs will not help me complete this at all.
I have to manually add the '/' and only then will emacs offer the next
level of completion.

Compare this to plain GDB:

  (gdb) file /tmp/qq<tab>
     => file /tmp/qqq/<tag>
     => file /tmp/qqq/f1

With a patched GDB, emacs is now as good as plain GDB.  This is thanks
to me adding the trailing '/' character in the 'complete' command
output.

And on the subject of quotes.  With an unpatched GDB emacs is perfectly
happy completing a file containing quotes (well, as happy as it is with
an unquoted file name), e.g.:

  (gdb) file "/tmp/qq<tab>
     => file "/tmp/qqq

But, when I get to the final file completion, emacs doesn't add the
trailing quote, so:

  (gdb) file "/tmp/qqq/f<tab>
    =>  file "/tmp/qqq/f1

In contrast and *unpatched* GDB at the CLI will add the final quote.

With a patched GDB, emacs is just as happy handling the quotes, only now
it adds the final trailing quote on, just like GDB's CLI:

  (gdb) file "/tmp/qqq/f<tab>
    =>  file "/tmp/qqq/f1"

OK, so summary of where we're at:

  + I think we're restricted to doing file completion at the GDB CLI the
    readline way.  I think trying to force readline to do things
    differently is just going to create maintenance overhead.  Of
    course, if someone wants to support such a thing, I love to see
    their plan.  But for now, I think we should keep it simple, do
    things the readline way.  Of course, if there's real problems that
    this causes then maybe we have to do things differently.  Let me
    know if any problems are found,

  + The complete command changes, as far as I can tell, only improve the
    emacs experience for filenames that _don't_ include white space.
    Nothing appears to be broken, but my testing has been limited.
    Please report any issues found,

  + For filenames that include white space, sadly, emacs doesn't "just
    work".  I've found one issue with the emacs code that isn't helping,
    but I suspect there's likely to be others beyond that.  That said,
    this is NOT a regression.  In fact, I think emacs is getting further
    through the completion process now (GDB is now sending it actual
    completions rather than junk), it's just emacs has never seen
    completions like this before (previous GDB would just send junk), so
    it's not really a surprise that this has exposed some bugs.  I'm
    willing to commit to creating emacs bugs for the completion features
    if/when this patch is merged so that these things can be addressed
    on the emacs side,

  + We are agreed that there's clearly a documentation issue.  However,
    I would argue that the documentation is missing for current
    functionality (i.e. the current rules for quoting and escaping file
    names).  We wouldn't document this stuff as part of "command
    completion", that doesn't really make sense I think.  I will start
    working on a separate patch to try and improve the documentation for
    how file names are quoted / escaped currently,

  + Lancelot has highlighted an issue with filename completion within
    'set' values, I haven't looked into that yet, but will do.

Are there any concerns that I haven't addressed?  I don't want to miss
anything that's concerning you.

Thanks,
Andrew