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

  When using the 'complete' command to complete file names we have some
problems.  The suggested completion should include any required
escaping, so if there is a filename '/tmp/aa"bb' (without the single
quotes), then this should be displayed in the completion output like:

  (gdb) complete file /tmp/aa
  file /tmp/aa\"bb

This doesn't currently happen.  We already have some code in
filename_completer (completer.c) that handles adding the trailing '/'
character if the completion result is a directory, so we might be
tempted to add any needed escaping in that function, however if we do
then we run into a problem.

If there are multiple completion results from a 'complete' command,
then in complete_command (cli/cli-cmds.c) we sort the completion
results prior to printing them.

If those results have already had the escaping added then the sort
will be done on the text including the escape characters.  This
means that results from the 'complete' command will appear in a
different order than readline would present them; readline sorts the
results and then adds and required escaping.

I think that the 'complete' command should behave the same as
readline, sort the entries and then add the escaping.  This means that
we need to sort before adding the escaping.

There is a second problem when using the 'complete' command with file
names, that is trailing quote characters.  The addition of a trailing
quote character is a bit complex due (I think) to the structure of
readline itself.

Adding the quote character currently occurs in two places in GDB.  In
completion_tracker::build_completion_result (completer.c) we add the
trailing quote in the case where we only have a single result, and in
complete_command (cli/cli-cmds.c) we add the trailing quote character
if we have more than one result.

With these two cases we ensure that the 'complete' command always adds
a trailing quote (when necessary) to the results it displays.

However, I think that for filename completion we don't always want a
trailing quote.  Consider if we have a file '/tmp/xxx/foo.c' (without
the quotes), and then we do:

  (gdb) complete file /tmp/xx

What should the result of this be?  Well, if we use TAB completion
like this:

  (gdb) file /tmp/xx<TAB>
  (gdb) file /tmp/xxx/

then readline completes the directory 'xxx/', but doesn't try to
complete the filename within the xxx/ directory until we press <TAB>
again.  So I think that the results of the 'complete' command should
share this behaviour:

  (gdb) complete file /tmp/xx
  file /tmp/xxx/

And this is what we get right now, but what if the user adds some
opening quotes?  Right now we get this:

  (gdb) complete file "/tmp/xx
  file "/tmp/xxx/

Which I think is correct, there's no trailing quote added.  This is
because in completion_tracker::build_completion_result the
completion_tracker object doesn't know that the double quote is the
quote_char().  However, this causes a problem, if we do this:

  (gdb) complete file "/tmp/xxx/f
  file "/tmp/xxx/foo.c

We're still missing the trailing quote, even though in this case, when
we've expanded a complete filename, adding the trailing quote would be
the correct thing to do.  The cause is, of course, the same, the
completion_tracker is unaware of what the quote character is, and so
doesn't add the quote when needed.

However, the problem gets worse, if we create an additional file
'/tmp/xxa/bar.c', notice that this is in a different directory.  So
now when we do this:

  (gdb) complete file "/tmp/xx
  file "/tmp/xxa/"
  file "/tmp/xxx/"

Now the trailing quote has been added even though we haven't completed
a full filename.  This is because in complete_command (cli/cli-cmds.c)
we do know what the quote character is, and the trailing quote is
always added.

There are multiple problems here, but at least part of the problem
will be solved by making the result printing in complete_command
smarter.  My plan is to allow different completion functions to modify
how the 'complete' results are printed.

This commit doesn't solve any of the problems listed above.  Instead
this commit is just a refactor.  I've moved the result printing logic
out of complete_command and created a new function
completion_result::print_matches.  And that's it.  Nothing has
functionally changed yet, that will come in later commits when the
print_matches function is made smarter.

There should be no user visible changes after this commit.
---
 gdb/cli/cli-cmds.c | 26 +-------------------------
 gdb/completer.c    | 33 +++++++++++++++++++++++++++++++++
 gdb/completer.h    | 13 +++++++++++++
 3 files changed, 47 insertions(+), 25 deletions(-)
  

> From: Andrew Burgess <aburgess@redhat.com>
> Cc: Andrew Burgess <aburgess@redhat.com>
> Date: Fri, 29 Mar 2024 11:42:28 +0000
> 
> When using the 'complete' command to complete file names we have some
> problems.  The suggested completion should include any required
> escaping, so if there is a filename '/tmp/aa"bb' (without the single
> quotes), then this should be displayed in the completion output like:
> 
>   (gdb) complete file /tmp/aa
>   file /tmp/aa\"bb

Why should it be displayed with the backslash?  And would the
completed name, if passed to a command, also have the backslash added?
If so, it could cause some commands to fail; basically, you are adding
shell file-name semantics into commands that don't work via the shell.

So I'm not sure I understand what is being intended here, and I'm
afraid you will uncover a lot of problems as you go with this (as you
already discovered).

Apologies if I'm missing something important here.
  

On Fri, Mar 29, 2024 at 03:14:07PM +0300, Eli Zaretskii wrote:
> > From: Andrew Burgess <aburgess@redhat.com>
> > Cc: Andrew Burgess <aburgess@redhat.com>
> > Date: Fri, 29 Mar 2024 11:42:28 +0000
> > 
> > When using the 'complete' command to complete file names we have some
> > problems.  The suggested completion should include any required
> > escaping, so if there is a filename '/tmp/aa"bb' (without the single
> > quotes), then this should be displayed in the completion output like:
> > 
> >   (gdb) complete file /tmp/aa
> >   file /tmp/aa\"bb
> 
> Why should it be displayed with the backslash?  And would the
> completed name, if passed to a command, also have the backslash added?
> If so, it could cause some commands to fail; basically, you are adding
> shell file-name semantics into commands that don't work via the shell.

Hi,

The "file" command currently expects "shell file-name semantics":

    $ gdb -q
    (gdb) file test/aa bb/a.out
    test/aa: No such file or directory.
    (gdb) file "test/aa bb/a.out"
    Reading symbols from test/aa bb/a.out...
    (gdb) file test/aa\ bb/a.out
    Load new symbol table from "test/aa bb/a.out"? (y or n) y
    Reading symbols from test/aa bb/a.out...
    (gdb)

The problem Andrew is trying to solve is that the current completer for
this command completes to something that is not a valid input.

Moreover, the completer as it is currently is is not entirely functional
for commands that expect "raw" filenames either.  If you take the same
directory structure, it can complete up to "test/aa bb", but can’t
complete the directory:

    $ gdb -q
    (gdb) complete file test/aa
    file test/aa bb
    (gdb) complete file test/aa bb
    (gdb) complete file test/aa bb/
    (gdb)

Where I do agree with you is that GDB is inconsistent: some command
expect "shell escaped filenames", and some do not.  For example, "set
logging file" will take its argument verbatim and use that:

    (gdb) set logging file "/tmp/test/aa bb/file.txt"
    (gdb) show logging file
    The current logfile is ""/tmp/test/aa bb/file.txt"".
    (gdb) set logging file /tmp/test/aa\ bb/file.txt
    (gdb) show logging file
    The current logfile is "/tmp/test/aa\ bb/file.txt".

Part of the issue you are raising is that both commands use the same
completer, so if the completer works for some commands, it will be wrong
for the others.  I have not reached the end of this series so I am not
sure if this is addressed (I do not expect it is), but I guess we should
either:
1) have 2 filename completers, and make sure that each function using a
   filename completer uses the appropriate one, or
2) have GDB be consistent regarding how commands consume filenames.

I would prefer consistency, but implementing 2 would strictly speaking
be an interface breaking change, so it needs careful consideration.

My experience is that I tend to use "file" / "add-symbol-file" more than
other commands dealing with filenames, so I would prefer to have a
completer that works for those.  Also, the current completer does not
completely work for commands that consume "raw" filenames anyway, so
having a completer that works properly for some commands rather than for
none sounds appealing to me.  That being said, this is just my 2 cent, I
am looking forward to hearing other people’s thoughts on the subject.

Best,
Lancelot.

> 
> So I'm not sure I understand what is being intended here, and I'm
> afraid you will uncover a lot of problems as you go with this (as you
> already discovered).
> 
> Apologies if I'm missing something important here.
  

Eli Zaretskii <eliz@gnu.org> writes:

>> Date: Sat, 30 Mar 2024 23:30:38 +0000
>> From: Lancelot SIX <lsix@lancelotsix.com>
>> Cc: Andrew Burgess <aburgess@redhat.com>, gdb-patches@sourceware.org
>> 
>> On Fri, Mar 29, 2024 at 03:14:07PM +0300, Eli Zaretskii wrote:
>> > > From: Andrew Burgess <aburgess@redhat.com>
>> > > Cc: Andrew Burgess <aburgess@redhat.com>
>> > > Date: Fri, 29 Mar 2024 11:42:28 +0000
>> > > 
>> > > When using the 'complete' command to complete file names we have some
>> > > problems.  The suggested completion should include any required
>> > > escaping, so if there is a filename '/tmp/aa"bb' (without the single
>> > > quotes), then this should be displayed in the completion output like:
>> > > 
>> > >   (gdb) complete file /tmp/aa
>> > >   file /tmp/aa\"bb
>> > 
>> > Why should it be displayed with the backslash?  And would the
>> > completed name, if passed to a command, also have the backslash added?
>> > If so, it could cause some commands to fail; basically, you are adding
>> > shell file-name semantics into commands that don't work via the shell.
>> 
>> Hi,
>> 
>> The "file" command currently expects "shell file-name semantics":
>
> For starters, this fact, if it is true, is not really documented.

Agreed.  This can be improved.

>
>>     $ gdb -q
>>     (gdb) file test/aa bb/a.out
>>     test/aa: No such file or directory.
>>     (gdb) file "test/aa bb/a.out"
>>     Reading symbols from test/aa bb/a.out...
>>     (gdb) file test/aa\ bb/a.out
>>     Load new symbol table from "test/aa bb/a.out"? (y or n) y
>>     Reading symbols from test/aa bb/a.out...
>>     (gdb)
>
> 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.

That is, this is how GDB quotes filenames.

This syntax isn't getting forwarded to any system shell, it remains
entirely within GDB, so I don't think we'd need to change the behaviour
for MS-Windows vs GNU/Linux.

> Also, Andrew used the 'file' command in his examples, but completion
> in GDB is used in many other commands, including those who don't
> necessarily expect/need shell file-name semantics.

These changes only impact filename completion, anything that doesn't
specifically invoke the filename completion function will not change its
behaviour.  If you see any examples of this then that is 100% a bug in
this work.

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

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.

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

>                  Andrew's patch series started from postulating a
> certain desired result without any such discussion, and I think we
> should discuss these aspects before we decide what is the desired
> result.

Sure.  My experience of proposing changes without patches is that folk
say, "Sure, sounds good, but how _exactly_ will it work.  What _exactly_
will it look like."  And honestly, I'm just not smart enough to answer
those questions without writing the code first.

I'm ALWAYS willing to rework changes based on feedback.  Please don't
think, just because I posted a patch that I'm saying it _has_ to be this
way.

That said, you email raises the idea concerns, but I'm still not clear
exactly _what_ your concerns are (other than emacs compatibility which
has been covered above).

I think there's something else which has been missed from this
discussion, which is super important when we talk about what the
"correct" behaviour should be:  I'm changing _completion_, I'm not
changing how GDB actually reads files names.

Right now if we want to pass a file containing whitespace to the 'file'
command then a user can either quote the filename, or escape the
whitespace.  That is _current_ behaviour, and is not something I'm
touching in this series.

This series makes the completion engine offer completions using those
schemes: if the user didn't include an opening quote then the whitespace
will be escaped.  If the filename did have an opening quote then no
escaping is necessary.  As such, it would seem that unless we want to
change how GDB currently parses filenames[*] then what completion should
do is pretty much prescribed by GDB's parsing rules...

I'll see if I can figure out the emacs thing, but if you do have some
hints that would be great.

Thanks,
Andrew




>
>> That being said, this is just my 2 cent, I am looking forward to
>> hearing other people’s thoughts on the subject.
>
> Same here.
>
> Thanks.
  

Lancelot SIX <lsix@lancelotsix.com> writes:

> On Fri, Mar 29, 2024 at 03:14:07PM +0300, Eli Zaretskii wrote:
>> > From: Andrew Burgess <aburgess@redhat.com>
>> > Cc: Andrew Burgess <aburgess@redhat.com>
>> > Date: Fri, 29 Mar 2024 11:42:28 +0000
>> > 
>> > When using the 'complete' command to complete file names we have some
>> > problems.  The suggested completion should include any required
>> > escaping, so if there is a filename '/tmp/aa"bb' (without the single
>> > quotes), then this should be displayed in the completion output like:
>> > 
>> >   (gdb) complete file /tmp/aa
>> >   file /tmp/aa\"bb
>> 
>> Why should it be displayed with the backslash?  And would the
>> completed name, if passed to a command, also have the backslash added?
>> If so, it could cause some commands to fail; basically, you are adding
>> shell file-name semantics into commands that don't work via the shell.
>
> Hi,
>
> The "file" command currently expects "shell file-name semantics":
>
>     $ gdb -q
>     (gdb) file test/aa bb/a.out
>     test/aa: No such file or directory.
>     (gdb) file "test/aa bb/a.out"
>     Reading symbols from test/aa bb/a.out...
>     (gdb) file test/aa\ bb/a.out
>     Load new symbol table from "test/aa bb/a.out"? (y or n) y
>     Reading symbols from test/aa bb/a.out...
>     (gdb)
>
> The problem Andrew is trying to solve is that the current completer for
> this command completes to something that is not a valid input.
>
> Moreover, the completer as it is currently is is not entirely functional
> for commands that expect "raw" filenames either.  If you take the same
> directory structure, it can complete up to "test/aa bb", but can’t
> complete the directory:
>
>     $ gdb -q
>     (gdb) complete file test/aa
>     file test/aa bb
>     (gdb) complete file test/aa bb
>     (gdb) complete file test/aa bb/
>     (gdb)
>
> Where I do agree with you is that GDB is inconsistent: some command
> expect "shell escaped filenames", and some do not.  For example, "set
> logging file" will take its argument verbatim and use that:
>
>     (gdb) set logging file "/tmp/test/aa bb/file.txt"
>     (gdb) show logging file
>     The current logfile is ""/tmp/test/aa bb/file.txt"".
>     (gdb) set logging file /tmp/test/aa\ bb/file.txt
>     (gdb) show logging file
>     The current logfile is "/tmp/test/aa\ bb/file.txt".
>
> Part of the issue you are raising is that both commands use the same
> completer, so if the completer works for some commands, it will be wrong
> for the others.  I have not reached the end of this series so I am not
> sure if this is addressed (I do not expect it is), but I guess we should
> either:
> 1) have 2 filename completers, and make sure that each function using a
>    filename completer uses the appropriate one, or
> 2) have GDB be consistent regarding how commands consume filenames.
>
> I would prefer consistency, but implementing 2 would strictly speaking
> be an interface breaking change, so it needs careful consideration.

I wasn't aware of this difference.  I would also rather just have
consistent parsing.

The 'set logging file' command gets away with this parsing because the
filename is always the last thing on the command line, but for other
commands this is not always the case.

We could probably arrange a mostly backward compatible solution for this
case.  I'll see if I can spot any other problem cases.

Thanks,
Andrew
  

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

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

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

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

Thanks.
  

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
  

> From: Andrew Burgess <aburgess@redhat.com>
> Cc: lsix@lancelotsix.com, gdb-patches@sourceware.org
> Date: Fri, 12 Apr 2024 23:20:33 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > 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.

Do we have those rules spelled out somewhere?  In the context of this
discussion, the pertinent rules are those for quoting in file names.
Are they documented?  And does this patch series modify those rules in
some way that users will need to know?

I guess this part:

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

means the answer is NO.  So yes, it would be good to document those
rules in the manual (it would also be useful when someone would like
to fix the bugs in Emacs you mention below).

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

Yes.  Would you mind submitting a bug report about that, using the
command "M-x report-emacs-bug"?

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

SGTM, thanks.

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

No, I don't think you've missed anything.  Thanks for working on this
important feature (I happen to be one of those completion junkies, so
any improvements in this area are welcome here).
  

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Andrew Burgess <aburgess@redhat.com>
>> Cc: lsix@lancelotsix.com, gdb-patches@sourceware.org
>> Date: Fri, 12 Apr 2024 23:20:33 +0100
>> 
>> 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.
>
> Yes.  Would you mind submitting a bug report about that, using the
> command "M-x report-emacs-bug"?

I absolutely will file the bug report once this series (or whatever this
series becomes) is merged.  However, I wanted to see if there were
additional bugs hiding in the emacs code, these might indicate changes
that need to be made (or not made) on the GDB side.

So the good news is, that with a little hacking I managed to get emacs
working!

Disclaimer: My emacs lisp knowledge is pretty weak, so please consider
this a proof of concept rather than code I'm actually suggesting should
be merged into emacs.  When I file the bug I'll include this code again,
but by adding it here we can hopefully see what needs to change.

The new and updated emacs code is at the end of this email if anyone
wants to play with emacs and this new completion.  Drop this new code
into a file, load the emacs 'gdb' mode, then eval the new code.  This
adds some new functions, and replaces one existing function.

With this in place I now get completion for filenames containing
whitespace.  Given this directory tree:

  /tmp/eee/
  ├── aa bb
  │   ├── f1
  │   └── f2
  └── aa cc
      ├── g1
      └── g2

  (gdb) file /tmp/eee/aa<TAB>
    =>  file /tmp/eee/aa\ <TAB>
  Possible completions are:		<--- These are offered in a new buffer.
  /tmp/eee/aa\ bb/			<--- I select this entry.
  /tmp/eee/aa\ cc/
    => file /tmp/eee/aa\ bb/<TAB>
    => file /tmp/eee/aa\ bb/f<TAB>
  Possible completions are:		<--- These are offered in a new buffer.
  /tmp/eee/aa\ bb/f1
  /tmp/eee/aa\ bb/f2			<--- I select this entry.
    => file /tmp/eee/aa\ bb/f2<ENTER>
  ... gdb tries to load the file ...

And if also works with quoting:

  (gdb) file "/tmp/eee/a<TAB>
    =>  file "/tmp/eee/aa <TAB>
  Possible completions are:		<--- These are offered in a new buffer.
  "/tmp/eee/aa bb/
  "/tmp/eee/aa cc/			<--- I select this entry.
    =>  file "/tmp/eee/aa cc/<TAB>
    =>  file "/tmp/eee/aa cc/g<TAB>
  Possible completions are:		<--- These are offered in a new buffer.
  "/tmp/eee/aa cc/g1"			<--- I select this entry.
  "/tmp/eee/aa cc/g2"
    =>  file "/tmp/eee/aa cc/g1"<ENTER>
 ... gdb tries to load the file ...

As I said, I'm not claiming that emacs is "fixed", but I think I've
shown what needs to be done to get this working, and also I've
satisfied myself that everything can be made to work.

Thanks,
Andrew



---

;; New function: Return true if the character at POS is escaped, that
;; is, is the character as POS preceeded by a backslash.
(defun gud-gdb-is-char-escaped (pos)
  (char-equal ?\\ (char-before pos)))

;; New function: Move point forward as skip-chars-forward does,
;; passing CHARS and LIMIT to skip-chars-forward.  However, after
;; calling skip-chars-forward, if the character we are looking at is
;; escaped (with a backslash) then skip that character, and call
;; skip-chars-forward again.
(defun gud-gdb-skip-to-unquoted (chars limit)
  (while (and (< (point) limit)
              (progn
                (skip-chars-forward chars limit)
                (if (gud-gdb-is-char-escaped (point))
                    (progn
                      (forward-char)
                      t)
                  nil)))))

;; New function: Move point forward skipping over a 'word'.  A word
;; can be quoted, starts with a single or double quote, in which case
;; the corresponding quote marks the end of the word.  Or a word can
;; be unquoted, in which case the word ends at the next white space.
;;
;; Never move forward past LIMIT.
;;
;; In an unquoted word white space can be escaped.
;;
;; In a double quoted word, double quotes can be escaped.
(defun gud-gdb-forward-maybe-quoted-word (limit)
  (cond ((char-equal ?\' (char-after))
         (progn
           (forward-char)
           (skip-chars-forward "^'" limit)))
        ((char-equal ?\" (char-after))
         (progn
           (forward-char)
           (gud-gdb-skip-to-unquoted "^\"" limit)))
        (t
         (progn
           (gud-gdb-skip-to-unquoted "^[:space:]" limit)))))

;; Assuming point is at the start of a line, return the position for
;; the start of a completion word.  The point will be moved by calling
;; this function.
;;
;; Never search past LIMIT.
;;
;; The completion word is the last word on the line, the word might be
;; quoted though.
(defun gud-gdb-find-completion-word (limit)
  (let ((completion-word-start nil))
    (while (< (point) limit)
      (setf completion-word-start (point))
      (gud-gdb-forward-maybe-quoted-word limit)
      (skip-chars-forward " " limit))

    completion-word-start))

;; This replaces an existing emacs function.  I've changed the logic
;; for finding the completion word.
(defun gud-gdb-completion-at-point ()
  "Return the data to complete the GDB command before point."
  (let* ((end (point))
        (start
         (save-excursion
           (goto-char (comint-line-beginning-position))
           (gud-gdb-find-completion-word end))))
    ;; 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))))))
  

> From: Andrew Burgess <aburgess@redhat.com>
> Cc: lsix@lancelotsix.com, gdb-patches@sourceware.org
> Date: Sat, 13 Apr 2024 10:09:52 +0100
> 
> > Yes.  Would you mind submitting a bug report about that, using the
> > command "M-x report-emacs-bug"?
> 
> I absolutely will file the bug report once this series (or whatever this
> series becomes) is merged.  However, I wanted to see if there were
> additional bugs hiding in the emacs code, these might indicate changes
> that need to be made (or not made) on the GDB side.
> 
> So the good news is, that with a little hacking I managed to get emacs
> working!
> 
> Disclaimer: My emacs lisp knowledge is pretty weak, so please consider
> this a proof of concept rather than code I'm actually suggesting should
> be merged into emacs.  When I file the bug I'll include this code again,
> but by adding it here we can hopefully see what needs to change.
> 
> The new and updated emacs code is at the end of this email if anyone
> wants to play with emacs and this new completion.  Drop this new code
> into a file, load the emacs 'gdb' mode, then eval the new code.  This
> adds some new functions, and replaces one existing function.
> 
> With this in place I now get completion for filenames containing
> whitespace.  Given this directory tree:
> 
>   /tmp/eee/
>   ├── aa bb
>   │   ├── f1
>   │   └── f2
>   └── aa cc
>       ├── g1
>       └── g2
> 
>   (gdb) file /tmp/eee/aa<TAB>
>     =>  file /tmp/eee/aa\ <TAB>
>   Possible completions are:		<--- These are offered in a new buffer.
>   /tmp/eee/aa\ bb/			<--- I select this entry.
>   /tmp/eee/aa\ cc/
>     => file /tmp/eee/aa\ bb/<TAB>
>     => file /tmp/eee/aa\ bb/f<TAB>
>   Possible completions are:		<--- These are offered in a new buffer.
>   /tmp/eee/aa\ bb/f1
>   /tmp/eee/aa\ bb/f2			<--- I select this entry.
>     => file /tmp/eee/aa\ bb/f2<ENTER>
>   ... gdb tries to load the file ...
> 
> And if also works with quoting:
> 
>   (gdb) file "/tmp/eee/a<TAB>
>     =>  file "/tmp/eee/aa <TAB>
>   Possible completions are:		<--- These are offered in a new buffer.
>   "/tmp/eee/aa bb/
>   "/tmp/eee/aa cc/			<--- I select this entry.
>     =>  file "/tmp/eee/aa cc/<TAB>
>     =>  file "/tmp/eee/aa cc/g<TAB>
>   Possible completions are:		<--- These are offered in a new buffer.
>   "/tmp/eee/aa cc/g1"			<--- I select this entry.
>   "/tmp/eee/aa cc/g2"
>     =>  file "/tmp/eee/aa cc/g1"<ENTER>
>  ... gdb tries to load the file ...
> 
> As I said, I'm not claiming that emacs is "fixed", but I think I've
> shown what needs to be done to get this working, and also I've
> satisfied myself that everything can be made to work.

Thanks, please include these patches with your bug report, so that our
completion experts could review them.