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:

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