Re: [PATCH] gdb/gdbserver: pass inferior arguments as a single string

[Kevin Buettner <kevinb@redhat.com>]

Hi Andrew,

I have a few commit log nits - see (inline) below.

On Tue, 13 May 2025 11:31:04 +0100
Andrew Burgess <aburgess@redhat.com> wrote:

> GDB holds the inferior arguments as a single string.  Currently when
> GDB needs to pass the inferior arguments to a remote target as part of
> a vRun packet, this is done by splitting the single argument string
> into its component arguments by calling gdb::remote_args::split, which
> uses the gdb_argv class to split the arguments for us.
> 
> The same gdb_argv class is used when the user has asked GDB/gdbserver
> to start the inferior without first invoking a shell; the gdb_argv
> class is used to split the argument string into it component
> arguments, and each is passed as a separate argument to the execve
> call which spawns the inferior.
> 
> There is however, a problem with using gdb_argv to split the arguments
> before passing them to a remote target.  To understand this problem we
> must first understand how gdb_argv is used when invoking an inferior
> without a shell.
> 
> And to understand how gdb_argv is used to start an inferior without a
> shell, I feel we need to first look at an example of starting an
> inferior with a shell.
> 
> Consider these two cases:
> 
>   (a)  (gdb) set args \$VAR
>   (b)  (gdb) set args $VAR
> 
> When starting with a shell, in case (a) the user expects the inferior
> to receive a literal '$VAR' string as an argument, while in case (b)
> the user expects to see the shell expanded value of the variable $VAR.
> 
> If the user does 'set startup-with-shell off', then in (a) GDB will
> strip the '\' while splitting the arguments, and the inferior will be
> passed a literal '$VAR'.  In (b) there is no '\' to strip, so also in
> this case the inferior will receive a literal '$VAR', remember
> startup-with-shell is off, so there is no shell than can ever expand

s/than/that/

> $VAR.
> 
> Notice, that when startup-with-shell is off, we end up with a many to
> one mapping, both (a) and (b) result in the literal string $VAR being
> passed to the inferior.  I think this is the correct behaviour in this
> case.
> 
> However, as we use gdb_argv to split the remote arguments we have the
> same many to one mapping within the vRun packet.  But the vRun packet
> will be used when startup-with-shell is both on and off.  What this
> means is that when gdbserver receives a vRun packet containing '$VAR'
> it doesn't know if GDB actually had '$VAR', or if GDB had '\$VAR'.
> And this is a huge problem.
> 
> We can address this by making the argument splitting for remote
> targets smarter, and I do have patches that try to do this in this
> series:
> 
>   https://inbox.sourceware.org/gdb-patches/cover.1730731085.git.aburgess@redhat.com
> 
> That series was pretty long, and wasn't getting reviewed, so I'm
> pulling the individual patches out and posting them separately.
> 
> This patch doesn't try to make remote argument splitting.  I think

s/make/do/

> that splitting and then joining the arguments is a mistake which can
> only introduce problems.  The patch in the above series which tries to
> make the splitting and joining "smarter" handles unquoted, single
> quoted, and double quoted strings.  But doesn't really address

s/But doesn't/But that doesn't/

> parameter substitution, command substitution, or arithmetic expansion.
> And even if we did try to address these cases, what rules exactly
> would we implement?  Probably POSIX shell rules, but what if the
> remote target doesn't have a POSIX shell?  The only reason we're
> talking about which shell rules to follow is because the splitting and
> joining logic needs to mirror those rules.  If we stop splitting and
> joining then we no longer need to care about the target's shell.
> 
> Clearly, for backward compatibility we need to maintain some degree of
> argument splitting and joining as we currently have; and that's why I
> have a later patch (see the series above) that tries to improve that
> splitting and joining a little.  But I think, what we should really
> do, is add a new feature flag (as used by the qSupported packet) and,
> if GDB and the remote target agree, we should pass the inferior
> arguments as a single string.
> 
> This solves all our problems.  In the startup with shell case, we no
> longer need to worry about splitting at all.  The arguments are passed
> unmodified to the remote target, that can then pass the arguments to
> the shell directly.
> 
> In the 'startup-with-shell off' case it is now up to the remote target
> to split the arguments, though in gdbserver we already did this, so
> nothing really changes in this case.  And if the remote target doesn't
> have a POSIX shell, well GDB just doesn't need to worry about it!
> 
> Something similar to this was originally suggested in this series:
> 
>   https://inbox.sourceware.org/gdb-patches/20211022071933.3478427-1-m.weghorn@posteo.de/
> 
> though this series didn't try to maintain backward compatibility,
> which I think is an issue that my patch solves.  Additionally, this
> series only passed the arguments as a single string in some cases,
> I've simplified this so that, when GDB and the remote agree, the
> arguments are always passed as a single string.  I think this is a
> little cleaner.
> 
> I've also added documentation and some tests with this commit,
> including ensuring that we test both the new single string approach,
> and the fallback split/join approach.
> 
> I've credited the author of the referenced series as co-author as they
> did come to a similar conclusion, though I think my implementation is
> different enough that I'm happy to list myself as primary author.
> 
> Co-Authored-By: Michael Weghorn <m.weghorn@posteo.de>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28392
> 
> Reviewed-By: Eli Zaretskii <eliz@gnu.org>
> Tested-By: Guinevere Larsen <guinevere@redhat.com>

Thanks for the detailed explanation.  This makes sense to me.

I also tested this patch and found no problems.

Approved-by: Kevin Buettner <kevinb@redhat.com>