[PATCH] gdbserver: convert program_args to a single string

[Simon Marchi]

On 2025-01-14 08:39, Andrew Burgess wrote:
> This commit changes how gdbserver stores the inferior arguments from
> being a vector of separate arguments into a single string with all of
> the arguments combined together.
> 
> Making this change might feel a little strange; intuitively it feels
> like we would be better off storing the arguments as a vector, but
> this change is part of a larger series of work that aims to improve
> GDB's inferior argument handling.  The full series was posted here:
> 
>   https://inbox.sourceware.org/gdb-patches/cover.1730731085.git.aburgess@redhat.com
> 
> But asking people to review a 14 patch series in unreasonable, so I'm
> instead posting the patches in smaller batches.  This patch can stand
> alone, and I do think this change makes sense on its own:
> 
> First, GDB already stores the inferior arguments as a single string,
> so doing this moves gdbserver into line with GDB.  The common code
> into which gdbserver calls requires the arguments to be a single
> string, so currently each target's create_inferior implementation
> merged the arguments anyway, so all this commit really does is move
> the merging up the call stack, and store the merged result rather than
> storing the separate parts.
> 
> However, the biggest reason for why this commit is needed, is an issue
> with passing arguments from GDB to gdbserver when starting a new
> inferior.
> 
> Consider:
> 
>   (gdb) set args $VAR
>   (gdb) run
>   ...
> 
> When using a native target the inferior will see the value of $VAR
> expanded by the shell GDB uses to start the inferior.  However, if
> using an extended-remote target the inferior will see literally $VAR,
> the unexpanded name of the variable, the reason for this is that,
> although GDB sends '$VAR' to gdbserver, when gdbserver receives this,
> it converts this to '\$VAR', which prevents the variable from being
> expanded by the shell.
> 
> The reason for this is that construct_inferior_arguments escapes all
> special shell characters within its arguments, and it is
> construct_inferior_arguments that is used to combine the separate
> arguments into a single string.
> 
> In the future I will change construct_inferior_arguments so that
> it can apply different escaping strategies.  When this happens we will
> want to escape arguments coming from the gdbserver command line
> differently than arguments coming from GDB (via a vRun packet), which
> means we need to call construct_inferior_arguments earlier, at the
> point where we know if the arguments came from the gdbserver command
> line, or from the vRun packet.
> 
> This argument escaping issue is discussed in PR gdb/28392.
> 
> This commit doesn't fix any issues, nor does it change
> construct_inferior_arguments to actually do different escaping, that
> will all come later.  This is purely a restructuring.
> 
> There should be no user visible changes after this commit.
> 
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28392
> 
> Tested-By: Guinevere Larsen <guinevere@redhat.com>

Some suggestions below, but otherwise:

Approved-By: Simon Marchi <simon.marchi@efficios.com>

> diff --git a/gdbserver/server.cc b/gdbserver/server.cc
> index 55898f59556..efe63ae7515 100644
> --- a/gdbserver/server.cc
> +++ b/gdbserver/server.cc
> @@ -121,7 +121,20 @@ private:
>    /* The program name, adjusted if needed.  */
>    std::string m_path;
>  } program_path;
> -static std::vector<char *> program_args;
> +
> +/* All program arguments are merged into a single string.  This is similar
> +   to how GDB manages the inferior arguments, and actually makes our lives
> +   easier; the rules for how arguments are merged into a single string
> +   differ depending on where the arguments come from.  Arguments arriving
> +   form the gdbserver command line are quoted, while arguments arriving
> +   from GDB (via a vRun packet) are already quoted.
> +
> +   NOTE: The comment above is ahead of its time.  The differences between
> +   how the PROGRAM_ARGS string is built up have not yet been implemented.
> +   A later patch in this series will make this change, and remove this
> +   note.  */

I think this is a bit too much for a code comment, it belongs to the
commit message (where it is already well explained).  It would be enough
to state what it is at the current time:

/* All program arguments are merged into a single string.  */

> @@ -4376,8 +4388,10 @@ captured_main (int argc, char *argv[])
>  
>        n = argc - (next_arg - argv);
>        program_path.set (next_arg[0]);
> +      std::vector<char *> temp_arg_vector;
>        for (i = 1; i < n; i++)
> -	program_args.push_back (xstrdup (next_arg[i]));
> +	temp_arg_vector.push_back (next_arg[i]);
> +      program_args = construct_inferior_arguments (temp_arg_vector);

Would that work, using std::vector's constructor that takes two
iterators?

    std::vector<char *> temp_arg_vector (&next_arg[1], &next_arg[argc]);
    program_args = construct_inferior_arguments (temp_arg_vector);

(not sure if the end iterator needs `argc` or `argc - 1`)

or directly:

    program_args = construct_inferior_arguments ({&next_arg[1], &next_arg[argc]});

> diff --git a/gdbserver/win32-low.cc b/gdbserver/win32-low.cc
> index da858b65e6f..139c945a2ba 100644
> --- a/gdbserver/win32-low.cc
> +++ b/gdbserver/win32-low.cc
> @@ -492,12 +492,12 @@ create_process (const char *program, char *args,
>  
>  /* Start a new process.
>     PROGRAM is the program name.
> -   PROGRAM_ARGS is the vector containing the inferior's args.
> +   PROGRAM_ARGS is a string containing all the inferior's arguments.
>     Returns the new PID on success, -1 on failure.  Registers the new
>     process with the process list.  */

I think this comment should just be removed, there's no point in
repeating the documentation from the base class.

Simon