gdbserver: convert program_args to a single string

Message ID f5352c4912607fd71803c2de080a7f56589011e8.1736861934.git.aburgess@redhat.com
State New
Headers
Series gdbserver: convert program_args to a single string |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_gdb_check--master-arm success Test passed
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 success Test passed

Commit Message

Andrew Burgess Jan. 14, 2025, 1:39 p.m. UTC
  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>
---
 gdbserver/linux-low.cc  |  5 ++---
 gdbserver/linux-low.h   |  2 +-
 gdbserver/netbsd-low.cc |  6 ++----
 gdbserver/netbsd-low.h  |  2 +-
 gdbserver/server.cc     | 24 +++++++++++++++++++-----
 gdbserver/target.h      |  6 +++---
 gdbserver/win32-low.cc  |  7 +++----
 gdbserver/win32-low.h   |  2 +-
 8 files changed, 32 insertions(+), 22 deletions(-)


base-commit: 127f733f88717bdb52f03c12c0c2d240bbc892e3
  

Comments

Simon Marchi Jan. 14, 2025, 3:32 p.m. UTC | #1
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
  
Tom Tromey Jan. 14, 2025, 5:23 p.m. UTC | #2
>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

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

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

FWIW I normally consider the "override" keyword to be sufficient
documentation in these cases; though there's also some code with a
comment pointing at the base class.

Tom
  
Simon Marchi Jan. 14, 2025, 5:34 p.m. UTC | #3
On 2025-01-14 12:23, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:
> 
>>> /* 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.  */
> 
> Simon> I think this comment should just be removed, there's no point in
> Simon> repeating the documentation from the base class.
> 
> FWIW I normally consider the "override" keyword to be sufficient
> documentation in these cases; though there's also some code with a
> comment pointing at the base class.

Speaking of this, I don't really know what to do in these situations, I'd like
if we could clarify what our style is.  Let's say you have:

-- base.h

struct base
{
  /* 1 */
  virtual void method ();
};

-- base.c

/* 2 */

void base::method ()
{
}

-- impl.c

struct impl : public base
{
  /* 3 */
  void method () override;
};

/* 4 */

void impl::method ()
{
}

Which comment should you have where?  We typically say "all declarations
and definitions should have a comment", but I feel like it's unnecessary here.

 - #1 is where you should have the documentation about the behavior of
   `method`
 - #2 would be the typical /* See base.h.  */, although I find these
   comments a bit useless
 - As you said, I think the `override` keyword is sufficient to make
   comment #3 unnecessary, unless you want to specify something specific
   to `impl`.  I don't see the need to point to the base class, that's
   just implied by how the language works.
 - I never know what to put for #4, you can't put /* See impl.c.  */,
   since you're already in impl.c.  Again, I feel like a "See whatever"
   comment is a bit useless: if you know C++, you know you need to go
   look at the declaration of struct/class `impl`.

Simon
  
Tom Tromey Jan. 14, 2025, 11:07 p.m. UTC | #4
>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

The approach I normally take is:

Simon> struct base
Simon> {
Simon>   /* 1 */
Simon>   virtual void method ();
Simon> };

... documentation of the contract here.

Simon> -- base.c
Simon> /* 2 */

"See blah.h" here.

Simon> struct impl : public base
Simon> {
Simon>   /* 3 */
Simon>   void method () override;

Nothing here.

Simon> /* 4 */

Simon> void impl::method ()

Not sure what I do here in practice.

Simon>  - #1 is where you should have the documentation about the behavior of
Simon>    `method`
Simon>  - #2 would be the typical /* See base.h.  */, although I find these
Simon>    comments a bit useless

Yeah they are.

Over time I've kind of come around to Doug's view which was that the
comments should be on the implementations.  But oh well, kind of late to
change that.

Simon>  - I never know what to put for #4, you can't put /* See impl.c.  */,
Simon>    since you're already in impl.c.  Again, I feel like a "See whatever"
Simon>    comment is a bit useless: if you know C++, you know you need to go
Simon>    look at the declaration of struct/class `impl`.

Yeah.  We could just leave off the comment here I suppose.

Tom
  
Andrew Burgess Jan. 15, 2025, 10:09 a.m. UTC | #5
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
  
Andrew Burgess Jan. 15, 2025, 10:21 a.m. UTC | #6
Simon Marchi <simark@simark.ca> writes:

> On 2025-01-14 12:23, Tom Tromey wrote:
>>>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:
>> 
>>>> /* 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.  */
>> 
>> Simon> I think this comment should just be removed, there's no point in
>> Simon> repeating the documentation from the base class.
>> 
>> FWIW I normally consider the "override" keyword to be sufficient
>> documentation in these cases; though there's also some code with a
>> comment pointing at the base class.
>
> Speaking of this, I don't really know what to do in these situations, I'd like
> if we could clarify what our style is.  Let's say you have:
>
> -- base.h
>
> struct base
> {
>   /* 1 */
>   virtual void method ();
> };
>
> -- base.c
>
> /* 2 */
>
> void base::method ()
> {
> }
>
> -- impl.c
>
> struct impl : public base
> {
>   /* 3 */
>   void method () override;
> };
>
> /* 4 */
>
> void impl::method ()
> {
> }
>
> Which comment should you have where?  We typically say "all declarations
> and definitions should have a comment", but I feel like it's unnecessary here.
>
>  - #1 is where you should have the documentation about the behavior of
>    `method`

As Tom said, the actual documentation could be at #1 or #2.  With C++
implementing more stuff in headers, having the docs at #1 makes more and
more sense I think.

>  - #2 would be the typical /* See base.h.  */, although I find these
>    comments a bit useless

It is a little useless, but I do like the consistency of all functions
having something.  And in some cases, when the declaration is not in the
obvious place, it can help find where to look.

>  - As you said, I think the `override` keyword is sufficient to make
>    comment #3 unnecessary, unless you want to specify something specific
>    to `impl`.  I don't see the need to point to the base class, that's
>    just implied by how the language works.

Agreed.

>  - I never know what to put for #4, you can't put /* See impl.c.  */,
>    since you're already in impl.c.  Again, I feel like a "See whatever"
>    comment is a bit useless: if you know C++, you know you need to go
>    look at the declaration of struct/class `impl`.

In cases where the base class declaration, and the out-of-line
definitions are in the same file I write /* See class declaration.  */
on the definitions (or something similar).

In this example I'd probably go with /* See base.h.  */ as that guides
folk to where the comment can be found.

Just thoughts.

Thanks,
Andrew
  

Patch

diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
index 50ce2b44927..65268a6ee6c 100644
--- a/gdbserver/linux-low.cc
+++ b/gdbserver/linux-low.cc
@@ -974,7 +974,7 @@  linux_ptrace_fun ()
 
 int
 linux_process_target::create_inferior (const char *program,
-				       const std::vector<char *> &program_args)
+				       const std::string &program_args)
 {
   client_state &cs = get_client_state ();
   struct lwp_info *new_lwp;
@@ -984,10 +984,9 @@  linux_process_target::create_inferior (const char *program,
   {
     maybe_disable_address_space_randomization restore_personality
       (cs.disable_randomization);
-    std::string str_program_args = construct_inferior_arguments (program_args);
 
     pid = fork_inferior (program,
-			 str_program_args.c_str (),
+			 program_args.c_str (),
 			 get_environ ()->envp (), linux_ptrace_fun,
 			 NULL, NULL, NULL, NULL);
   }
diff --git a/gdbserver/linux-low.h b/gdbserver/linux-low.h
index 5be00b8c98c..75af38d898f 100644
--- a/gdbserver/linux-low.h
+++ b/gdbserver/linux-low.h
@@ -141,7 +141,7 @@  class linux_process_target : public process_stratum_target
 public:
 
   int create_inferior (const char *program,
-		       const std::vector<char *> &program_args) override;
+		       const std::string &program_args) override;
 
   void post_create_inferior () override;
 
diff --git a/gdbserver/netbsd-low.cc b/gdbserver/netbsd-low.cc
index 04e103563e7..9e7314b02a9 100644
--- a/gdbserver/netbsd-low.cc
+++ b/gdbserver/netbsd-low.cc
@@ -78,11 +78,9 @@  netbsd_ptrace_fun ()
 
 int
 netbsd_process_target::create_inferior (const char *program,
-					const std::vector<char *> &program_args)
+					const std::string &program_args)
 {
-  std::string str_program_args = construct_inferior_arguments (program_args);
-
-  pid_t pid = fork_inferior (program, str_program_args.c_str (),
+  pid_t pid = fork_inferior (program, program_args.c_str (),
 			     get_environ ()->envp (), netbsd_ptrace_fun,
 			     nullptr, nullptr, nullptr, nullptr);
 
diff --git a/gdbserver/netbsd-low.h b/gdbserver/netbsd-low.h
index 53200ddffc4..aef1ce40cb9 100644
--- a/gdbserver/netbsd-low.h
+++ b/gdbserver/netbsd-low.h
@@ -42,7 +42,7 @@  class netbsd_process_target : public process_stratum_target
 public:
 
   int create_inferior (const char *program,
-		       const std::vector<char *> &program_args) override;
+		       const std::string &program_args) override;
 
   void post_create_inferior () override;
 
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.  */
+static std::string program_args;
+
 static std::string wrapper_argv;
 
 /* The PID of the originally created or attached inferior.  Used to
@@ -3484,9 +3497,8 @@  handle_v_run (char *own_buf)
   else
     program_path.set (new_program_name.get ());
 
-  /* Free the old argv and install the new one.  */
-  free_vector_argv (program_args);
-  program_args = new_argv;
+  program_args = construct_inferior_arguments (new_argv);
+  free_vector_argv (new_argv);
 
   try
     {
@@ -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);
 
       /* Wait till we are at first instruction in program.  */
       target_create_inferior (program_path.get (), program_args);
diff --git a/gdbserver/target.h b/gdbserver/target.h
index 3643b9110da..1a013bb83db 100644
--- a/gdbserver/target.h
+++ b/gdbserver/target.h
@@ -77,13 +77,13 @@  class process_stratum_target
   /* Start a new process.
 
      PROGRAM is a path to the program to execute.
-     PROGRAM_ARGS is a standard NULL-terminated array of arguments,
-     to be passed to the inferior as ``argv'' (along with PROGRAM).
+     PROGRAM_ARGS is a string containing all of the arguments that will be
+     used to start the inferior.
 
      Returns the new PID on success, -1 on failure.  Registers the new
      process with the process list.  */
   virtual int create_inferior (const char *program,
-			       const std::vector<char *> &program_args) = 0;
+			       const std::string &program_args) = 0;
 
   /* Do additional setup after a new process is created, including
      exec-wrapper completion.  */
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.  */
 int
 win32_process_target::create_inferior (const char *program,
-				       const std::vector<char *> &program_args)
+				       const std::string &program_args)
 {
   client_state &cs = get_client_state ();
 #ifndef USE_WIN32API
@@ -508,8 +508,7 @@  win32_process_target::create_inferior (const char *program,
   DWORD flags;
   PROCESS_INFORMATION pi;
   DWORD err;
-  std::string str_program_args = construct_inferior_arguments (program_args);
-  char *args = (char *) str_program_args.c_str ();
+  char *args = (char *) program_args.c_str ();
 
   /* win32_wait needs to know we're not attaching.  */
   windows_process.attaching = 0;
diff --git a/gdbserver/win32-low.h b/gdbserver/win32-low.h
index daed16a6ae6..e9dd6adc5a8 100644
--- a/gdbserver/win32-low.h
+++ b/gdbserver/win32-low.h
@@ -101,7 +101,7 @@  class win32_process_target : public process_stratum_target
 public:
 
   int create_inferior (const char *program,
-		       const std::vector<char *> &program_args) override;
+		       const std::string &program_args) override;
 
   int attach (unsigned long pid) override;