Re: [PATCH] gdbserver: convert program_args to a single string

[Andrew Burgess <aburgess@redhat.com>]

Simon Marchi <simark@simark.ca> writes:

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

I made the improvements you suggested, and pushed this.

Thanks,
Andrew


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