Patches for PR 25893 "gdbserver incorrectly handles program args containing space"

Patches for PR 25893 "gdbserver incorrectly handles program args containing space"

[Michael Weghorn]

Hi,

I have taken a look why gdbserver currently cannot correctly handle
program args containing spaces (PR 25893, [1]) and am sending patches
that implement a potential solution for this in follow-up emails.

This is the first time I'm trying to contribute to gdb and I haven't
completed an FSF copyright assignment so far.
I'm happy about any feedback on the patches, and also other hints in case
I've missed anything when reading the contribution guideline.

If anybody has a better solution for the bug, please also go ahead and
I'll be happy to just drop my patches.

Thanks in advance and best regards,
Michael

[1] https://sourceware.org/bugzilla/show_bug.cgi?id=25893

[PATCH 0/4] gdb: Move construct_inferior_arguments to gdbsupport

[Michael Weghorn]

This moves the function construct_inferior_arguments from
gdb/inferior.h and gdb/infcmd.c to gdbsupport/common-inferior.{h,cc}.

The intention is to use it from gdbserver in a follow-up commit.

gdb/ChangeLog:

2020-04-29  Michael Weghorn  <m.weghorn@posteo.de>

        * infcmd.c, inferior.h: (construct_inferior_arguments):
          Moved function from here to gdbsupport/common-inferior.{h,cc}

gdbsupport/ChangeLog:

2020-04-29  Michael Weghorn  <m.weghorn@posteo.de>

        * common-inferior.h, common-inferior.cc: (construct_inferior_arguments):
        Move function here from gdb/infcmd.c, gdb/inferior.h
---
 gdb/infcmd.c                  | 124 ----------------------------------
 gdb/inferior.h                |   2 -
 gdbsupport/common-inferior.cc | 123 +++++++++++++++++++++++++++++++++
 gdbsupport/common-inferior.h  |   2 +
 4 files changed, 125 insertions(+), 126 deletions(-)

diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 9bbb413d4e..8f7482347c 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -259,130 +259,6 @@ server's cwd if remote debugging.\n"));
 			"when starting the inferior is \"%s\".\n"), cwd);
 }
 
-\f
-/* Compute command-line string given argument vector.  This does the
-   same shell processing as fork_inferior.  */
-
-char *
-construct_inferior_arguments (int argc, char **argv)
-{
-  char *result;
-
-  /* ARGC should always be at least 1, but we double check this
-     here.  This is also needed to silence -Werror-stringop
-     warnings.  */
-  gdb_assert (argc > 0);
-
-  if (startup_with_shell)
-    {
-#ifdef __MINGW32__
-      /* This holds all the characters considered special to the
-	 Windows shells.  */
-      static const char special[] = "\"!&*|[]{}<>?`~^=;, \t\n";
-      static const char quote = '"';
-#else
-      /* This holds all the characters considered special to the
-	 typical Unix shells.  We include `^' because the SunOS
-	 /bin/sh treats it as a synonym for `|'.  */
-      static const char special[] = "\"!#$&*()\\|[]{}<>?'`~^; \t\n";
-      static const char quote = '\'';
-#endif
-      int i;
-      int length = 0;
-      char *out, *cp;
-
-      /* We over-compute the size.  It shouldn't matter.  */
-      for (i = 0; i < argc; ++i)
-	length += 3 * strlen (argv[i]) + 1 + 2 * (argv[i][0] == '\0');
-
-      result = (char *) xmalloc (length);
-      out = result;
-
-      for (i = 0; i < argc; ++i)
-	{
-	  if (i > 0)
-	    *out++ = ' ';
-
-	  /* Need to handle empty arguments specially.  */
-	  if (argv[i][0] == '\0')
-	    {
-	      *out++ = quote;
-	      *out++ = quote;
-	    }
-	  else
-	    {
-#ifdef __MINGW32__
-	      int quoted = 0;
-
-	      if (strpbrk (argv[i], special))
-		{
-		  quoted = 1;
-		  *out++ = quote;
-		}
-#endif
-	      for (cp = argv[i]; *cp; ++cp)
-		{
-		  if (*cp == '\n')
-		    {
-		      /* A newline cannot be quoted with a backslash (it
-			 just disappears), only by putting it inside
-			 quotes.  */
-		      *out++ = quote;
-		      *out++ = '\n';
-		      *out++ = quote;
-		    }
-		  else
-		    {
-#ifdef __MINGW32__
-		      if (*cp == quote)
-#else
-		      if (strchr (special, *cp) != NULL)
-#endif
-			*out++ = '\\';
-		      *out++ = *cp;
-		    }
-		}
-#ifdef __MINGW32__
-	      if (quoted)
-		*out++ = quote;
-#endif
-	    }
-	}
-      *out = '\0';
-    }
-  else
-    {
-      /* In this case we can't handle arguments that contain spaces,
-	 tabs, or newlines -- see breakup_args().  */
-      int i;
-      int length = 0;
-
-      for (i = 0; i < argc; ++i)
-	{
-	  char *cp = strchr (argv[i], ' ');
-	  if (cp == NULL)
-	    cp = strchr (argv[i], '\t');
-	  if (cp == NULL)
-	    cp = strchr (argv[i], '\n');
-	  if (cp != NULL)
-	    error (_("can't handle command-line "
-		     "argument containing whitespace"));
-	  length += strlen (argv[i]) + 1;
-	}
-
-      result = (char *) xmalloc (length);
-      result[0] = '\0';
-      for (i = 0; i < argc; ++i)
-	{
-	  if (i > 0)
-	    strcat (result, " ");
-	  strcat (result, argv[i]);
-	}
-    }
-
-  return result;
-}
-\f
 
 /* This function strips the '&' character (indicating background
    execution) that is added as *the last* of the arguments ARGS of a
diff --git a/gdb/inferior.h b/gdb/inferior.h
index 1ac51369df..95af474eed 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -184,8 +184,6 @@ extern void child_interrupt (struct target_ops *self);
    STARTUP_INFERIOR.  */
 extern ptid_t gdb_startup_inferior (pid_t pid, int num_traps);
 
-extern char *construct_inferior_arguments (int, char **);
-
 /* From infcmd.c */
 
 /* Initial inferior setup.  Determines the exec file is not yet known,
diff --git a/gdbsupport/common-inferior.cc b/gdbsupport/common-inferior.cc
index ed16e89a52..71b9a11e02 100644
--- a/gdbsupport/common-inferior.cc
+++ b/gdbsupport/common-inferior.cc
@@ -24,3 +24,126 @@
 /* See common-inferior.h.  */
 
 bool startup_with_shell = true;
+
+/* Compute command-line string given argument vector.  This does the
+   same shell processing as fork_inferior.  */
+
+char *
+construct_inferior_arguments (int argc, char **argv)
+{
+  char *result;
+
+  /* ARGC should always be at least 1, but we double check this
+     here.  This is also needed to silence -Werror-stringop
+     warnings.  */
+  gdb_assert (argc > 0);
+
+  if (startup_with_shell)
+    {
+#ifdef __MINGW32__
+      /* This holds all the characters considered special to the
+	 Windows shells.  */
+      static const char special[] = "\"!&*|[]{}<>?`~^=;, \t\n";
+      static const char quote = '"';
+#else
+      /* This holds all the characters considered special to the
+	 typical Unix shells.  We include `^' because the SunOS
+	 /bin/sh treats it as a synonym for `|'.  */
+      static const char special[] = "\"!#$&*()\\|[]{}<>?'`~^; \t\n";
+      static const char quote = '\'';
+#endif
+      int i;
+      int length = 0;
+      char *out, *cp;
+
+      /* We over-compute the size.  It shouldn't matter.  */
+      for (i = 0; i < argc; ++i)
+	length += 3 * strlen (argv[i]) + 1 + 2 * (argv[i][0] == '\0');
+
+      result = (char *) xmalloc (length);
+      out = result;
+
+      for (i = 0; i < argc; ++i)
+	{
+	  if (i > 0)
+	    *out++ = ' ';
+
+	  /* Need to handle empty arguments specially.  */
+	  if (argv[i][0] == '\0')
+	    {
+	      *out++ = quote;
+	      *out++ = quote;
+	    }
+	  else
+	    {
+#ifdef __MINGW32__
+	      int quoted = 0;
+
+	      if (strpbrk (argv[i], special))
+		{
+		  quoted = 1;
+		  *out++ = quote;
+		}
+#endif
+	      for (cp = argv[i]; *cp; ++cp)
+		{
+		  if (*cp == '\n')
+		    {
+		      /* A newline cannot be quoted with a backslash (it
+			 just disappears), only by putting it inside
+			 quotes.  */
+		      *out++ = quote;
+		      *out++ = '\n';
+		      *out++ = quote;
+		    }
+		  else
+		    {
+#ifdef __MINGW32__
+		      if (*cp == quote)
+#else
+		      if (strchr (special, *cp) != NULL)
+#endif
+			*out++ = '\\';
+		      *out++ = *cp;
+		    }
+		}
+#ifdef __MINGW32__
+	      if (quoted)
+		*out++ = quote;
+#endif
+	    }
+	}
+      *out = '\0';
+    }
+  else
+    {
+      /* In this case we can't handle arguments that contain spaces,
+	 tabs, or newlines -- see breakup_args().  */
+      int i;
+      int length = 0;
+
+      for (i = 0; i < argc; ++i)
+	{
+	  char *cp = strchr (argv[i], ' ');
+	  if (cp == NULL)
+	    cp = strchr (argv[i], '\t');
+	  if (cp == NULL)
+	    cp = strchr (argv[i], '\n');
+	  if (cp != NULL)
+	    error (_("can't handle command-line "
+		     "argument containing whitespace"));
+	  length += strlen (argv[i]) + 1;
+	}
+
+      result = (char *) xmalloc (length);
+      result[0] = '\0';
+      for (i = 0; i < argc; ++i)
+	{
+	  if (i > 0)
+	    strcat (result, " ");
+	  strcat (result, argv[i]);
+	}
+    }
+
+  return result;
+}
diff --git a/gdbsupport/common-inferior.h b/gdbsupport/common-inferior.h
index 3c8e87aee6..eb60c8f13b 100644
--- a/gdbsupport/common-inferior.h
+++ b/gdbsupport/common-inferior.h
@@ -58,4 +58,6 @@ extern void set_inferior_cwd (const char *cwd);
    the target is started up with a shell.  */
 extern bool startup_with_shell;
 
+extern char *construct_inferior_arguments (int, char **);
+
 #endif /* COMMON_COMMON_INFERIOR_H */
-- 
2.26.2

[PATCH 1/4] gdbsupport: Extend construct_inferior_arguments to allow handling all stringify_args cases

[Michael Weghorn]

Allow construct_inferior_arguments to handle zero args
and have it return a std::string, similar to how
stringify_argv in gdbsupport/common-utils does.

Also, add a const qualifier for the second parameter,
since it is only read, not written to.

The intention is to replace some existing uses of
stringify_argv by construct_inferior_arguments
in a subsequent step, since construct_inferior_arguments
properly handles special characters, while stringify_argv
doesn't.

2020-04-29  Michael Weghorn  <m.weghorn@posteo.de>

        * common-inferior.cc, common-inferior.h (construct_inferior_arguments):
        Adapt to handle zero args and return a std::string.
        Adapt call site.
---
 gdb/infcmd.c                  |  5 ++---
 gdbsupport/common-inferior.cc | 19 +++++++++++--------
 gdbsupport/common-inferior.h  |  2 +-
 3 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 8f7482347c..7ad931f9b4 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -151,12 +151,11 @@ get_inferior_args (void)
 {
   if (current_inferior ()->argc != 0)
     {
-      char *n;
+      std::string n;
 
       n = construct_inferior_arguments (current_inferior ()->argc,
 					current_inferior ()->argv);
-      set_inferior_args (n);
-      xfree (n);
+      set_inferior_args (n.c_str());
     }
 
   if (current_inferior ()->args == NULL)
diff --git a/gdbsupport/common-inferior.cc b/gdbsupport/common-inferior.cc
index 71b9a11e02..3f117d5ef0 100644
--- a/gdbsupport/common-inferior.cc
+++ b/gdbsupport/common-inferior.cc
@@ -28,15 +28,15 @@ bool startup_with_shell = true;
 /* Compute command-line string given argument vector.  This does the
    same shell processing as fork_inferior.  */
 
-char *
-construct_inferior_arguments (int argc, char **argv)
+std::string
+construct_inferior_arguments (int argc, char * const *argv)
 {
-  char *result;
+  gdb_assert (argc >= 0);
+  if (argc == 0 || argv[0] == NULL) {
+      return "";
+  }
 
-  /* ARGC should always be at least 1, but we double check this
-     here.  This is also needed to silence -Werror-stringop
-     warnings.  */
-  gdb_assert (argc > 0);
+  char *result;
 
   if (startup_with_shell)
     {
@@ -145,5 +145,8 @@ construct_inferior_arguments (int argc, char **argv)
 	}
     }
 
-  return result;
+  std::string str_result(result);
+  xfree (result);
+
+  return str_result;
 }
diff --git a/gdbsupport/common-inferior.h b/gdbsupport/common-inferior.h
index eb60c8f13b..0b11e7d6a5 100644
--- a/gdbsupport/common-inferior.h
+++ b/gdbsupport/common-inferior.h
@@ -58,6 +58,6 @@ extern void set_inferior_cwd (const char *cwd);
    the target is started up with a shell.  */
 extern bool startup_with_shell;
 
-extern char *construct_inferior_arguments (int, char **);
+extern std::string construct_inferior_arguments (int, char * const *);
 
 #endif /* COMMON_COMMON_INFERIOR_H */
-- 
2.26.2

[PATCH 2/4] gdbserver: Don't add extra NULL to program args

[Michael Weghorn]

The vector holding the program args is passed as a parameter
to target_create_inferior, which then passes it to
stringify_argv for all platforms, where any NULL entry in
the vector is ignored, so there seems to be no reason
to actually add one after all.

(Since the intention is to replace uses of stringify_argv with
construct_inferior_arguments in a follow-up commit and that
function doesn't currently handle such NULL arguments, it
would otherwise have to be extended.)

gdbserver/ChangeLog:

2020-04-29  Michael Weghorn  <m.weghorn@posteo.de>

        * server.cc (captured_main): No longer insert extra NULL
        element to args vector.
---
 gdbserver/server.cc | 1 -
 1 file changed, 1 deletion(-)

diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index 0672f9bc4d..239511c104 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -3815,7 +3815,6 @@ captured_main (int argc, char *argv[])
       program_path.set (make_unique_xstrdup (next_arg[0]));
       for (i = 1; i < n; i++)
 	program_args.push_back (xstrdup (next_arg[i]));
-      program_args.push_back (NULL);
 
       /* Wait till we are at first instruction in program.  */
       target_create_inferior (program_path.get (), program_args);
-- 
2.26.2

[PATCH 3/4] [PR gdbserver/25893]: Use construct_inferior_arguments which handles special chars

[Michael Weghorn]

Use the construct_inferior_arguments function instead of
stringify_argv to construct a string from the program
arguments in those places where that one is then passed
to fork_inferior, since construct_inferior_arguments
properly takes care of special characters, while
stringify_argv does not.
Using construct_inferior_arguments seems "natural", since its
documentation also mentions that it "does the
same shell processing as fork_inferior".

This makes gdbserver properly handle program args containing special
characters, e.g. (example from PR25893)

  $ gdbserver localhost:50505 myprogram "hello world"

now properly handles "hello world" as a single arg, not two separate
ones ("hello", "world").

I'm not sure regarding the two remaining uses of function stringify_argv
(in win32_process_target::create_inferior and
nto_process_target::create_inferior) whose result is not passed to
fork_inferior and don't know how the arg processing in the creation
of processes on those targets exactly works, so left them unchanged
for now.

2020-04-29  Michael Weghorn  <m.weghorn@posteo.de>

        PR gdbserver/25893
        * linux-low.cc (linux_process_target::create_inferior),
        lynx-low.cc (lynx_process_target::create_inferior): Use
        construct_inferior_arguments instead of stringify_argv
        to get string representation which properly escapes
        special characters.
---
 gdbserver/linux-low.cc | 2 +-
 gdbserver/lynx-low.cc  | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
index 3cd8d5594d..3f3ffc6c43 100644
--- a/gdbserver/linux-low.cc
+++ b/gdbserver/linux-low.cc
@@ -984,7 +984,7 @@ linux_process_target::create_inferior (const char *program,
   {
     maybe_disable_address_space_randomization restore_personality
       (cs.disable_randomization);
-    std::string str_program_args = stringify_argv (program_args);
+    std::string str_program_args = construct_inferior_arguments (program_args.size(), program_args.data());
 
     pid = fork_inferior (program,
 			 str_program_args.c_str (),
diff --git a/gdbserver/lynx-low.cc b/gdbserver/lynx-low.cc
index 9aa140c129..61f692f0c0 100644
--- a/gdbserver/lynx-low.cc
+++ b/gdbserver/lynx-low.cc
@@ -253,7 +253,7 @@ lynx_process_target::create_inferior (const char *program,
 				      const std::vector<char *> &program_args)
 {
   int pid;
-  std::string str_program_args = stringify_argv (program_args);
+  std::string str_program_args = construct_inferior_arguments (program_args.size(), program_args.data());;
 
   lynx_debug ("create_inferior ()");
 
-- 
2.26.2

[PATCH 4/4] [PR gdbserver/25893]: Add gdbserver test for argument with space in it

[Michael Weghorn]

This adds a test to call gdbserver with a program
that is passed an argument containing a space and
checks that argv[1] actually contains the value
of this arg.

gdb/testsuite/ChangeLog:

2020-04-29  Michael Weghorn  <m.weghorn@posteo.de>

        PR gdbserver/25893
        * gdb.server/arg-with-space.exp: New test that
        covers running program with arg containing
        a space with gdbserver
---
 gdb/testsuite/gdb.server/arg-with-space.exp | 41 +++++++++++++++++++++
 1 file changed, 41 insertions(+)
 create mode 100644 gdb/testsuite/gdb.server/arg-with-space.exp

diff --git a/gdb/testsuite/gdb.server/arg-with-space.exp b/gdb/testsuite/gdb.server/arg-with-space.exp
new file mode 100644
index 0000000000..d768b7ca0f
--- /dev/null
+++ b/gdb/testsuite/gdb.server/arg-with-space.exp
@@ -0,0 +1,41 @@
+# This testcase is part of GDB, the GNU debugger.
+#
+# Copyright 2020 Free Software Foundation, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Test gdbserver to properly handle a program arg with a space in it
+# as a single argument.
+
+load_lib gdbserver-support.exp
+
+standard_testfile "normal.c"
+
+if {[skip_gdbserver_tests]} {
+    return 0
+}
+
+if {[gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } {
+    return -1
+}
+clean_restart ${testfile}
+
+gdbserver_run "\"hello world\""
+
+gdb_breakpoint main
+gdb_continue main
+
+# check that program arg was passed properly
+gdb_test "print argc" "2"
+gdb_test "print argv\[1\]" "\"hello world\""
-- 
2.26.2

Re: [PATCH 1/4] gdbsupport: Extend construct_inferior_arguments to allow handling all stringify_args cases

[Christian Biesinger]

On Wed, Apr 29, 2020 at 6:17 AM Michael Weghorn via Gdb-patches
<gdb-patches@sourceware.org> wrote:
>
> Allow construct_inferior_arguments to handle zero args
> and have it return a std::string, similar to how
> stringify_argv in gdbsupport/common-utils does.
>
> Also, add a const qualifier for the second parameter,
> since it is only read, not written to.
>
> The intention is to replace some existing uses of
> stringify_argv by construct_inferior_arguments
> in a subsequent step, since construct_inferior_arguments
> properly handles special characters, while stringify_argv
> doesn't.

Can you clarify which revision your patch is against? My files look
nothing like the ones you are patching.

> 2020-04-29  Michael Weghorn  <m.weghorn@posteo.de>
>
>         * common-inferior.cc, common-inferior.h (construct_inferior_arguments):
>         Adapt to handle zero args and return a std::string.
>         Adapt call site.
> ---
>  gdb/infcmd.c                  |  5 ++---
>  gdbsupport/common-inferior.cc | 19 +++++++++++--------
>  gdbsupport/common-inferior.h  |  2 +-
>  3 files changed, 14 insertions(+), 12 deletions(-)
>
> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> index 8f7482347c..7ad931f9b4 100644
> --- a/gdb/infcmd.c
> +++ b/gdb/infcmd.c
> @@ -151,12 +151,11 @@ get_inferior_args (void)
>  {
>    if (current_inferior ()->argc != 0)
>      {
> -      char *n;
> +      std::string n;

While changing this, I would just move the declaration to the line
where it is first used.

>
>        n = construct_inferior_arguments (current_inferior ()->argc,
>                                         current_inferior ()->argv);
> -      set_inferior_args (n);
> -      xfree (n);
> +      set_inferior_args (n.c_str());
>      }
>
>    if (current_inferior ()->args == NULL)
> diff --git a/gdbsupport/common-inferior.cc b/gdbsupport/common-inferior.cc
> index 71b9a11e02..3f117d5ef0 100644
> --- a/gdbsupport/common-inferior.cc
> +++ b/gdbsupport/common-inferior.cc
> @@ -28,15 +28,15 @@ bool startup_with_shell = true;
>  /* Compute command-line string given argument vector.  This does the
>     same shell processing as fork_inferior.  */
>
> -char *
> -construct_inferior_arguments (int argc, char **argv)
> +std::string
> +construct_inferior_arguments (int argc, char * const *argv)
>  {
> -  char *result;
> +  gdb_assert (argc >= 0);
> +  if (argc == 0 || argv[0] == NULL) {
> +      return "";
> +  }
>
> -  /* ARGC should always be at least 1, but we double check this
> -     here.  This is also needed to silence -Werror-stringop
> -     warnings.  */
> -  gdb_assert (argc > 0);
> +  char *result;
>
>    if (startup_with_shell)
>      {
> @@ -145,5 +145,8 @@ construct_inferior_arguments (int argc, char **argv)
>         }
>      }
>
> -  return result;
> +  std::string str_result(result);
> +  xfree (result);
> +
> +  return str_result;
>  }
> diff --git a/gdbsupport/common-inferior.h b/gdbsupport/common-inferior.h
> index eb60c8f13b..0b11e7d6a5 100644
> --- a/gdbsupport/common-inferior.h
> +++ b/gdbsupport/common-inferior.h
> @@ -58,6 +58,6 @@ extern void set_inferior_cwd (const char *cwd);
>     the target is started up with a shell.  */
>  extern bool startup_with_shell;
>
> -extern char *construct_inferior_arguments (int, char **);
> +extern std::string construct_inferior_arguments (int, char * const *);
>
>  #endif /* COMMON_COMMON_INFERIOR_H */
> --
> 2.26.2
>

Re: Patches for PR 25893 "gdbserver incorrectly handles program args containing space"

[Simon Marchi]

On 2020-04-29 7:16 a.m., Michael Weghorn via Gdb-patches wrote:
> Hi,
> 
> I have taken a look why gdbserver currently cannot correctly handle
> program args containing spaces (PR 25893, [1]) and am sending patches
> that implement a potential solution for this in follow-up emails.
> 
> This is the first time I'm trying to contribute to gdb and I haven't
> completed an FSF copyright assignment so far.
> I'm happy about any feedback on the patches, and also other hints in case
> I've missed anything when reading the contribution guideline.
> 
> If anybody has a better solution for the bug, please also go ahead and
> I'll be happy to just drop my patches.
> 
> Thanks in advance and best regards,
> Michael
> 
> [1] https://sourceware.org/bugzilla/show_bug.cgi?id=25893

Hi Michael,

Thanks for the patches.  You've sent the patches using git-send-email, so that's
already a very good start :).

For your copyright assignment, if you haven't started the process already, take
a look at this form.  The instructions are included.

  http://git.savannah.gnu.org/cgit/gnulib.git/tree/doc/Copyright/request-assign.future

We'll try to review your patches soon, but given the volume of patches, there can
be some delay.  If you have no response after two weeks, please ping your patchset,
and then weekly after that.  That will keep it alive.

Thanks,

Simon

Re: [PATCH 1/4] gdbsupport: Extend construct_inferior_arguments to allow handling all stringify_args cases

[Christian Biesinger]

On Wed, Apr 29, 2020 at 10:25 AM Christian Biesinger
<cbiesinger@google.com> wrote:
>
> On Wed, Apr 29, 2020 at 6:17 AM Michael Weghorn via Gdb-patches
> <gdb-patches@sourceware.org> wrote:
> >
> > Allow construct_inferior_arguments to handle zero args
> > and have it return a std::string, similar to how
> > stringify_argv in gdbsupport/common-utils does.
> >
> > Also, add a const qualifier for the second parameter,
> > since it is only read, not written to.
> >
> > The intention is to replace some existing uses of
> > stringify_argv by construct_inferior_arguments
> > in a subsequent step, since construct_inferior_arguments
> > properly handles special characters, while stringify_argv
> > doesn't.
>
> Can you clarify which revision your patch is against? My files look
> nothing like the ones you are patching.

Oh, I see now that this patch applies on top of the other patch series you sent.

> > 2020-04-29  Michael Weghorn  <m.weghorn@posteo.de>
> >
> >         * common-inferior.cc, common-inferior.h (construct_inferior_arguments):
> >         Adapt to handle zero args and return a std::string.
> >         Adapt call site.
> > ---
> >  gdb/infcmd.c                  |  5 ++---
> >  gdbsupport/common-inferior.cc | 19 +++++++++++--------
> >  gdbsupport/common-inferior.h  |  2 +-
> >  3 files changed, 14 insertions(+), 12 deletions(-)
> >
> > diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> > index 8f7482347c..7ad931f9b4 100644
> > --- a/gdb/infcmd.c
> > +++ b/gdb/infcmd.c
> > @@ -151,12 +151,11 @@ get_inferior_args (void)
> >  {
> >    if (current_inferior ()->argc != 0)
> >      {
> > -      char *n;
> > +      std::string n;
>
> While changing this, I would just move the declaration to the line
> where it is first used.
>
> >
> >        n = construct_inferior_arguments (current_inferior ()->argc,
> >                                         current_inferior ()->argv);
> > -      set_inferior_args (n);
> > -      xfree (n);
> > +      set_inferior_args (n.c_str());
> >      }
> >
> >    if (current_inferior ()->args == NULL)
> > diff --git a/gdbsupport/common-inferior.cc b/gdbsupport/common-inferior.cc
> > index 71b9a11e02..3f117d5ef0 100644
> > --- a/gdbsupport/common-inferior.cc
> > +++ b/gdbsupport/common-inferior.cc
> > @@ -28,15 +28,15 @@ bool startup_with_shell = true;
> >  /* Compute command-line string given argument vector.  This does the
> >     same shell processing as fork_inferior.  */
> >
> > -char *
> > -construct_inferior_arguments (int argc, char **argv)
> > +std::string
> > +construct_inferior_arguments (int argc, char * const *argv)
> >  {
> > -  char *result;
> > +  gdb_assert (argc >= 0);
> > +  if (argc == 0 || argv[0] == NULL) {
> > +      return "";
> > +  }
> >
> > -  /* ARGC should always be at least 1, but we double check this
> > -     here.  This is also needed to silence -Werror-stringop
> > -     warnings.  */
> > -  gdb_assert (argc > 0);
> > +  char *result;
> >
> >    if (startup_with_shell)
> >      {
> > @@ -145,5 +145,8 @@ construct_inferior_arguments (int argc, char **argv)
> >         }
> >      }
> >
> > -  return result;
> > +  std::string str_result(result);
> > +  xfree (result);
> > +
> > +  return str_result;
> >  }
> > diff --git a/gdbsupport/common-inferior.h b/gdbsupport/common-inferior.h
> > index eb60c8f13b..0b11e7d6a5 100644
> > --- a/gdbsupport/common-inferior.h
> > +++ b/gdbsupport/common-inferior.h
> > @@ -58,6 +58,6 @@ extern void set_inferior_cwd (const char *cwd);
> >     the target is started up with a shell.  */
> >  extern bool startup_with_shell;
> >
> > -extern char *construct_inferior_arguments (int, char **);
> > +extern std::string construct_inferior_arguments (int, char * const *);
> >
> >  #endif /* COMMON_COMMON_INFERIOR_H */
> > --
> > 2.26.2
> >

[PATCH v2 1/4] gdbsupport: Extend construct_inferior_arguments to allow handling all stringify_args cases

[Michael Weghorn]

On 29/04/2020 17.25, Christian Biesinger wrote:
>> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
>> index 8f7482347c..7ad931f9b4 100644
>> --- a/gdb/infcmd.c
>> +++ b/gdb/infcmd.c
>> @@ -151,12 +151,11 @@ get_inferior_args (void)
>>  {
>>    if (current_inferior ()->argc != 0)
>>      {
>> -      char *n;
>> +      std::string n;
>
> While changing this, I would just move the declaration to the line
> where it is first used.
>

Done now. A new version of the patch follows. (I haven't updated the
other patches of the series, since those remain unchanged.)

---

Allow construct_inferior_arguments to handle zero args
and have it return a std::string, similar to how
stringify_argv in gdbsupport/common-utils does.

Also, add a const qualifier for the second parameter,
since it is only read, not written to.

The intention is to replace some existing uses of
stringify_argv by construct_inferior_arguments
in a subsequent step, since construct_inferior_arguments
properly handles special characters, while stringify_argv
doesn't.

2020-04-29  Michael Weghorn  <m.weghorn@posteo.de>

        * common-inferior.cc, common-inferior.h (construct_inferior_arguments):
        Adapt to handle zero args and return a std::string.
        Adapt call site.
---
 gdb/infcmd.c                  |  7 ++-----
 gdbsupport/common-inferior.cc | 19 +++++++++++--------
 gdbsupport/common-inferior.h  |  2 +-
 3 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 8f7482347c..3ac64b3164 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -151,12 +151,9 @@ get_inferior_args (void)
 {
   if (current_inferior ()->argc != 0)
     {
-      char *n;
-
-      n = construct_inferior_arguments (current_inferior ()->argc,
+      std::string n = construct_inferior_arguments (current_inferior ()->argc,
 					current_inferior ()->argv);
-      set_inferior_args (n);
-      xfree (n);
+      set_inferior_args (n.c_str());
     }

   if (current_inferior ()->args == NULL)
diff --git a/gdbsupport/common-inferior.cc b/gdbsupport/common-inferior.cc
index 71b9a11e02..3f117d5ef0 100644
--- a/gdbsupport/common-inferior.cc
+++ b/gdbsupport/common-inferior.cc
@@ -28,15 +28,15 @@ bool startup_with_shell = true;
 /* Compute command-line string given argument vector.  This does the
    same shell processing as fork_inferior.  */

-char *
-construct_inferior_arguments (int argc, char **argv)
+std::string
+construct_inferior_arguments (int argc, char * const *argv)
 {
-  char *result;
+  gdb_assert (argc >= 0);
+  if (argc == 0 || argv[0] == NULL) {
+      return "";
+  }

-  /* ARGC should always be at least 1, but we double check this
-     here.  This is also needed to silence -Werror-stringop
-     warnings.  */
-  gdb_assert (argc > 0);
+  char *result;

   if (startup_with_shell)
     {
@@ -145,5 +145,8 @@ construct_inferior_arguments (int argc, char **argv)
 	}
     }

-  return result;
+  std::string str_result(result);
+  xfree (result);
+
+  return str_result;
 }
diff --git a/gdbsupport/common-inferior.h b/gdbsupport/common-inferior.h
index eb60c8f13b..0b11e7d6a5 100644
--- a/gdbsupport/common-inferior.h
+++ b/gdbsupport/common-inferior.h
@@ -58,6 +58,6 @@ extern void set_inferior_cwd (const char *cwd);
    the target is started up with a shell.  */
 extern bool startup_with_shell;

-extern char *construct_inferior_arguments (int, char **);
+extern std::string construct_inferior_arguments (int, char * const *);

 #endif /* COMMON_COMMON_INFERIOR_H */
-- 
2.26.2

Re: Patches for PR 25893 "gdbserver incorrectly handles program args containing space"

[Michael Weghorn]

On 29/04/2020 17.27, Simon Marchi wrote:
> On 2020-04-29 7:16 a.m., Michael Weghorn via Gdb-patches wrote:
>> Hi,
>>
>> I have taken a look why gdbserver currently cannot correctly handle
>> program args containing spaces (PR 25893, [1]) and am sending patches
>> that implement a potential solution for this in follow-up emails.
>>
>> This is the first time I'm trying to contribute to gdb and I haven't
>> completed an FSF copyright assignment so far.
>> I'm happy about any feedback on the patches, and also other hints in case
>> I've missed anything when reading the contribution guideline.
>>
>> If anybody has a better solution for the bug, please also go ahead and
>> I'll be happy to just drop my patches.
>>
>> Thanks in advance and best regards,
>> Michael
>>
>> [1] https://sourceware.org/bugzilla/show_bug.cgi?id=25893
> 
> Hi Michael,
> 
> Thanks for the patches.  You've sent the patches using git-send-email, so that's
> already a very good start :).
> 
> For your copyright assignment, if you haven't started the process already, take
> a look at this form.  The instructions are included.
> 
>   http://git.savannah.gnu.org/cgit/gnulib.git/tree/doc/Copyright/request-assign.future
> 
> We'll try to review your patches soon, but given the volume of patches, there can
> be some delay.  If you have no response after two weeks, please ping your patchset,
> and then weekly after that.  That will keep it alive.
> 
> Thanks,
> 
> Simon
> 

Hi Simon,

thanks. :)

I have now filled the form and sent it to assign@gnu.org.

Best regards,
Michael

Re: [PATCH 1/4] gdbsupport: Extend construct_inferior_arguments to allow handling all stringify_args cases

[Simon Marchi]

On 2020-04-29 11:45 a.m., Christian Biesinger via Gdb-patches wrote:
> On Wed, Apr 29, 2020 at 10:25 AM Christian Biesinger
> <cbiesinger@google.com> wrote:
>>
>> On Wed, Apr 29, 2020 at 6:17 AM Michael Weghorn via Gdb-patches
>> <gdb-patches@sourceware.org> wrote:
>>>
>>> Allow construct_inferior_arguments to handle zero args
>>> and have it return a std::string, similar to how
>>> stringify_argv in gdbsupport/common-utils does.
>>>
>>> Also, add a const qualifier for the second parameter,
>>> since it is only read, not written to.
>>>
>>> The intention is to replace some existing uses of
>>> stringify_argv by construct_inferior_arguments
>>> in a subsequent step, since construct_inferior_arguments
>>> properly handles special characters, while stringify_argv
>>> doesn't.
>>
>> Can you clarify which revision your patch is against? My files look
>> nothing like the ones you are patching.
> 
> Oh, I see now that this patch applies on top of the other patch series you sent.

I was also confused, normally the patch 0/N is the cover letter, not a patch
to apply.

Simon

Re: [PATCH 0/4] gdb: Move construct_inferior_arguments to gdbsupport

[Simon Marchi]

On 2020-04-29 7:16 a.m., Michael Weghorn via Gdb-patches wrote:
> This moves the function construct_inferior_arguments from
> gdb/inferior.h and gdb/infcmd.c to gdbsupport/common-inferior.{h,cc}.
> 
> The intention is to use it from gdbserver in a follow-up commit.
> 
> gdb/ChangeLog:
> 
> 2020-04-29  Michael Weghorn  <m.weghorn@posteo.de>
> 
>         * infcmd.c, inferior.h: (construct_inferior_arguments):
>           Moved function from here to gdbsupport/common-inferior.{h,cc}
> 
> gdbsupport/ChangeLog:
> 
> 2020-04-29  Michael Weghorn  <m.weghorn@posteo.de>
> 
>         * common-inferior.h, common-inferior.cc: (construct_inferior_arguments):
>         Move function here from gdb/infcmd.c, gdb/inferior.h
> ---
>  gdb/infcmd.c                  | 124 ----------------------------------
>  gdb/inferior.h                |   2 -
>  gdbsupport/common-inferior.cc | 123 +++++++++++++++++++++++++++++++++
>  gdbsupport/common-inferior.h  |   2 +
>  4 files changed, 125 insertions(+), 126 deletions(-)
> 
> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> index 9bbb413d4e..8f7482347c 100644
> --- a/gdb/infcmd.c
> +++ b/gdb/infcmd.c
> @@ -259,130 +259,6 @@ server's cwd if remote debugging.\n"));
>  			"when starting the inferior is \"%s\".\n"), cwd);
>  }
>  
> -\f
> -/* Compute command-line string given argument vector.  This does the
> -   same shell processing as fork_inferior.  */
> -
> -char *
> -construct_inferior_arguments (int argc, char **argv)
> -{
> -  char *result;
> -
> -  /* ARGC should always be at least 1, but we double check this
> -     here.  This is also needed to silence -Werror-stringop
> -     warnings.  */
> -  gdb_assert (argc > 0);
> -
> -  if (startup_with_shell)
> -    {
> -#ifdef __MINGW32__
> -      /* This holds all the characters considered special to the
> -	 Windows shells.  */
> -      static const char special[] = "\"!&*|[]{}<>?`~^=;, \t\n";
> -      static const char quote = '"';
> -#else
> -      /* This holds all the characters considered special to the
> -	 typical Unix shells.  We include `^' because the SunOS
> -	 /bin/sh treats it as a synonym for `|'.  */
> -      static const char special[] = "\"!#$&*()\\|[]{}<>?'`~^; \t\n";
> -      static const char quote = '\'';
> -#endif
> -      int i;
> -      int length = 0;
> -      char *out, *cp;
> -
> -      /* We over-compute the size.  It shouldn't matter.  */
> -      for (i = 0; i < argc; ++i)
> -	length += 3 * strlen (argv[i]) + 1 + 2 * (argv[i][0] == '\0');
> -
> -      result = (char *) xmalloc (length);
> -      out = result;
> -
> -      for (i = 0; i < argc; ++i)
> -	{
> -	  if (i > 0)
> -	    *out++ = ' ';
> -
> -	  /* Need to handle empty arguments specially.  */
> -	  if (argv[i][0] == '\0')
> -	    {
> -	      *out++ = quote;
> -	      *out++ = quote;
> -	    }
> -	  else
> -	    {
> -#ifdef __MINGW32__
> -	      int quoted = 0;
> -
> -	      if (strpbrk (argv[i], special))
> -		{
> -		  quoted = 1;
> -		  *out++ = quote;
> -		}
> -#endif
> -	      for (cp = argv[i]; *cp; ++cp)
> -		{
> -		  if (*cp == '\n')
> -		    {
> -		      /* A newline cannot be quoted with a backslash (it
> -			 just disappears), only by putting it inside
> -			 quotes.  */
> -		      *out++ = quote;
> -		      *out++ = '\n';
> -		      *out++ = quote;
> -		    }
> -		  else
> -		    {
> -#ifdef __MINGW32__
> -		      if (*cp == quote)
> -#else
> -		      if (strchr (special, *cp) != NULL)
> -#endif
> -			*out++ = '\\';
> -		      *out++ = *cp;
> -		    }
> -		}
> -#ifdef __MINGW32__
> -	      if (quoted)
> -		*out++ = quote;
> -#endif
> -	    }
> -	}
> -      *out = '\0';
> -    }
> -  else
> -    {
> -      /* In this case we can't handle arguments that contain spaces,
> -	 tabs, or newlines -- see breakup_args().  */
> -      int i;
> -      int length = 0;
> -
> -      for (i = 0; i < argc; ++i)
> -	{
> -	  char *cp = strchr (argv[i], ' ');
> -	  if (cp == NULL)
> -	    cp = strchr (argv[i], '\t');
> -	  if (cp == NULL)
> -	    cp = strchr (argv[i], '\n');
> -	  if (cp != NULL)
> -	    error (_("can't handle command-line "
> -		     "argument containing whitespace"));
> -	  length += strlen (argv[i]) + 1;
> -	}
> -
> -      result = (char *) xmalloc (length);
> -      result[0] = '\0';
> -      for (i = 0; i < argc; ++i)
> -	{
> -	  if (i > 0)
> -	    strcat (result, " ");
> -	  strcat (result, argv[i]);
> -	}
> -    }
> -
> -  return result;
> -}
> -\f
>  
>  /* This function strips the '&' character (indicating background
>     execution) that is added as *the last* of the arguments ARGS of a
> diff --git a/gdb/inferior.h b/gdb/inferior.h
> index 1ac51369df..95af474eed 100644
> --- a/gdb/inferior.h
> +++ b/gdb/inferior.h
> @@ -184,8 +184,6 @@ extern void child_interrupt (struct target_ops *self);
>     STARTUP_INFERIOR.  */
>  extern ptid_t gdb_startup_inferior (pid_t pid, int num_traps);
>  
> -extern char *construct_inferior_arguments (int, char **);
> -
>  /* From infcmd.c */
>  
>  /* Initial inferior setup.  Determines the exec file is not yet known,
> diff --git a/gdbsupport/common-inferior.cc b/gdbsupport/common-inferior.cc
> index ed16e89a52..71b9a11e02 100644
> --- a/gdbsupport/common-inferior.cc
> +++ b/gdbsupport/common-inferior.cc
> @@ -24,3 +24,126 @@
>  /* See common-inferior.h.  */
>  
>  bool startup_with_shell = true;
> +
> +/* Compute command-line string given argument vector.  This does the
> +   same shell processing as fork_inferior.  */

Let's just do a bit of spring cleaning and align this code with our current
standards while moving it.  Move the function comment to the .h, and in the.cc
file write:

  /* See common-inferior.h. */

Other than that, this patch LGTM.

Simon

Re: [PATCH v2 1/4] gdbsupport: Extend construct_inferior_arguments to allow handling all stringify_args cases

[Simon Marchi]

Hi Michael,

I think the patch looks good, there are just a few nits to fix, and a suggestion.

> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> index 8f7482347c..3ac64b3164 100644
> --- a/gdb/infcmd.c
> +++ b/gdb/infcmd.c
> @@ -151,12 +151,9 @@ get_inferior_args (void)
>  {
>    if (current_inferior ()->argc != 0)
>      {
> -      char *n;
> -
> -      n = construct_inferior_arguments (current_inferior ()->argc,
> +      std::string n = construct_inferior_arguments (current_inferior ()->argc,
>  					current_inferior ()->argv);

Align the last line with the parenthesis, like it was before the change.

> -      set_inferior_args (n);
> -      xfree (n);
> +      set_inferior_args (n.c_str());

Space before parenthesis.

>      }
> 
>    if (current_inferior ()->args == NULL)
> diff --git a/gdbsupport/common-inferior.cc b/gdbsupport/common-inferior.cc
> index 71b9a11e02..3f117d5ef0 100644
> --- a/gdbsupport/common-inferior.cc
> +++ b/gdbsupport/common-inferior.cc
> @@ -28,15 +28,15 @@ bool startup_with_shell = true;
>  /* Compute command-line string given argument vector.  This does the
>     same shell processing as fork_inferior.  */
> 
> -char *
> -construct_inferior_arguments (int argc, char **argv)
> +std::string
> +construct_inferior_arguments (int argc, char * const *argv)
>  {
> -  char *result;
> +  gdb_assert (argc >= 0);
> +  if (argc == 0 || argv[0] == NULL) {
> +      return "";
> +  }

Omit the braces when there is just one single line statement.

When there are braces, places them like this:

  if (condition)
    {
      stmt1;
      stmt2;
    }

Did you actually encounter the case (argc > 0 && argv[0] == NULL)?  In
other words, does it really constitute a valid input to this function?

> 
> -  /* ARGC should always be at least 1, but we double check this
> -     here.  This is also needed to silence -Werror-stringop
> -     warnings.  */
> -  gdb_assert (argc > 0);
> +  char *result;
> 
>    if (startup_with_shell)
>      {
> @@ -145,5 +145,8 @@ construct_inferior_arguments (int argc, char **argv)
>  	}
>      }
> 
> -  return result;
> +  std::string str_result(result);

Space before parenthesis.

If we are going to return a string anyway, I'd vote for building the result
into an std::string directly.  You can remove the part that tries to guess
the length of the string, and use the appropriate std::string methods /
operators to append to it.

If you do this, I'm thinking that the `argc == 0` check at the beginning of
the function might be unnecessary, as the code could handle it naturally:
all the loops will be skipped and we'll return an empty string.

Thanks,

Simon

Re: [PATCH 2/4] gdbserver: Don't add extra NULL to program args

[Simon Marchi]

On 2020-04-29 7:16 a.m., Michael Weghorn via Gdb-patches wrote:
> The vector holding the program args is passed as a parameter
> to target_create_inferior, which then passes it to
> stringify_argv for all platforms, where any NULL entry in
> the vector is ignored, so there seems to be no reason
> to actually add one after all.
> 
> (Since the intention is to replace uses of stringify_argv with
> construct_inferior_arguments in a follow-up commit and that
> function doesn't currently handle such NULL arguments, it
> would otherwise have to be extended.)

Thanks, this LGTM.

And a big thank you for writing informative commit messages up front, those
help a lot understand the need for your changes.

Simon

Re: [PATCH 3/4] [PR gdbserver/25893]: Use construct_inferior_arguments which handles special chars

[Simon Marchi]

On 2020-04-29 7:16 a.m., Michael Weghorn via Gdb-patches wrote:
> Use the construct_inferior_arguments function instead of
> stringify_argv to construct a string from the program
> arguments in those places where that one is then passed
> to fork_inferior, since construct_inferior_arguments
> properly takes care of special characters, while
> stringify_argv does not.
> Using construct_inferior_arguments seems "natural", since its
> documentation also mentions that it "does the
> same shell processing as fork_inferior".
> 
> This makes gdbserver properly handle program args containing special
> characters, e.g. (example from PR25893)
> 
>   $ gdbserver localhost:50505 myprogram "hello world"
> 
> now properly handles "hello world" as a single arg, not two separate
> ones ("hello", "world").
> 
> I'm not sure regarding the two remaining uses of function stringify_argv
> (in win32_process_target::create_inferior and
> nto_process_target::create_inferior) whose result is not passed to
> fork_inferior and don't know how the arg processing in the creation
> of processes on those targets exactly works, so left them unchanged
> for now.

I'd say, do the change the best you can for win32 and nto.  We'll be
able to test win32.  Regarding nto, it hasn't seen patches for years,
it most likely doesn't even build.  And the toolchain not being easily
available, we can't do much about it.  I think we should actually
consider removing this port, unless somebody steps up to maintain it.
Same for lynx.

This will allow removing stringify_argv, and avoid having duplicate
broken functionality of construct_inferior_arguments.

> 2020-04-29  Michael Weghorn  <m.weghorn@posteo.de>
> 
>         PR gdbserver/25893
>         * linux-low.cc (linux_process_target::create_inferior),
>         lynx-low.cc (lynx_process_target::create_inferior): Use
>         construct_inferior_arguments instead of stringify_argv
>         to get string representation which properly escapes
>         special characters.
> ---
>  gdbserver/linux-low.cc | 2 +-
>  gdbserver/lynx-low.cc  | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
> index 3cd8d5594d..3f3ffc6c43 100644
> --- a/gdbserver/linux-low.cc
> +++ b/gdbserver/linux-low.cc
> @@ -984,7 +984,7 @@ linux_process_target::create_inferior (const char *program,
>    {
>      maybe_disable_address_space_randomization restore_personality
>        (cs.disable_randomization);
> -    std::string str_program_args = stringify_argv (program_args);
> +    std::string str_program_args = construct_inferior_arguments (program_args.size(), program_args.data());

This line is too long, see below.

>  
>      pid = fork_inferior (program,
>  			 str_program_args.c_str (),
> diff --git a/gdbserver/lynx-low.cc b/gdbserver/lynx-low.cc
> index 9aa140c129..61f692f0c0 100644
> --- a/gdbserver/lynx-low.cc
> +++ b/gdbserver/lynx-low.cc
> @@ -253,7 +253,7 @@ lynx_process_target::create_inferior (const char *program,
>  				      const std::vector<char *> &program_args)
>  {
>    int pid;
> -  std::string str_program_args = stringify_argv (program_args);
> +  std::string str_program_args = construct_inferior_arguments (program_args.size(), program_args.data());;

Extra semi-colon, and spaces before parenthesis.  The line should fit in 80 columns, here's how
we'd typically break it:

  std::string str_program_args
    = construct_inferior_arguments (program_args.size (), program_args.data ());

(with the equal sign on the second line, I know that may seem odd)

If you are game, you could make another preparatory patch (anywhere in the series
before this one) that would make construct_inferior_arguments take a
gdb::array_view<char *> parameter.  That would allow you to call it more simply
here:

  construct_inferior_arguments (program_args);

Simon

Re: [PATCH 3/4] [PR gdbserver/25893]: Use construct_inferior_arguments which handles special chars

[Simon Marchi]

On 2020-05-07 4:09 p.m., Simon Marchi wrote:
> On 2020-04-29 7:16 a.m., Michael Weghorn via Gdb-patches wrote:
>> Use the construct_inferior_arguments function instead of
>> stringify_argv to construct a string from the program
>> arguments in those places where that one is then passed
>> to fork_inferior, since construct_inferior_arguments
>> properly takes care of special characters, while
>> stringify_argv does not.
>> Using construct_inferior_arguments seems "natural", since its
>> documentation also mentions that it "does the
>> same shell processing as fork_inferior".
>>
>> This makes gdbserver properly handle program args containing special
>> characters, e.g. (example from PR25893)
>>
>>   $ gdbserver localhost:50505 myprogram "hello world"
>>
>> now properly handles "hello world" as a single arg, not two separate
>> ones ("hello", "world").
>>
>> I'm not sure regarding the two remaining uses of function stringify_argv
>> (in win32_process_target::create_inferior and
>> nto_process_target::create_inferior) whose result is not passed to
>> fork_inferior and don't know how the arg processing in the creation
>> of processes on those targets exactly works, so left them unchanged
>> for now.
> 
> I'd say, do the change the best you can for win32 and nto.  We'll be
> able to test win32.  Regarding nto, it hasn't seen patches for years,
> it most likely doesn't even build.  And the toolchain not being easily
> available, we can't do much about it.  I think we should actually
> consider removing this port, unless somebody steps up to maintain it.
> Same for lynx.
> 
> This will allow removing stringify_argv, and avoid having duplicate
> broken functionality of construct_inferior_arguments.

While trying out stuff to review patch 4, I noticed a crash introduced by this patch.

In one terminal, run gdbserver with:

$ ./gdbserver --once --multi :1234

In another, run GDB with:

$ ./gdb /bin/ls -ex 'set pagination off' -ex 'set remote exec-file /bin/ls' -ex 'target extended-remote :1234' -ex 'run hello'

Doing so segfaults gdbserver for me.  Could you look into it?

The commands above try to run a new process through gdbserver.  The args are passed
to GDB using the remote protocol.  You can add -ex 'set debug remote 1' before the run
part to see the exchanged packets, the args are hex-encoded in the vRun packet.

Simon

Re: [PATCH 4/4] [PR gdbserver/25893]: Add gdbserver test for argument with space in it

[Simon Marchi]

On 2020-04-29 7:16 a.m., Michael Weghorn via Gdb-patches wrote:
> This adds a test to call gdbserver with a program
> that is passed an argument containing a space and
> checks that argv[1] actually contains the value
> of this arg.
> 
> gdb/testsuite/ChangeLog:
> 
> 2020-04-29  Michael Weghorn  <m.weghorn@posteo.de>
> 
>         PR gdbserver/25893
>         * gdb.server/arg-with-space.exp: New test that
>         covers running program with arg containing
>         a space with gdbserver
> ---
>  gdb/testsuite/gdb.server/arg-with-space.exp | 41 +++++++++++++++++++++
>  1 file changed, 41 insertions(+)
>  create mode 100644 gdb/testsuite/gdb.server/arg-with-space.exp
> 
> diff --git a/gdb/testsuite/gdb.server/arg-with-space.exp b/gdb/testsuite/gdb.server/arg-with-space.exp
> new file mode 100644
> index 0000000000..d768b7ca0f
> --- /dev/null
> +++ b/gdb/testsuite/gdb.server/arg-with-space.exp
> @@ -0,0 +1,41 @@
> +# This testcase is part of GDB, the GNU debugger.
> +#
> +# Copyright 2020 Free Software Foundation, Inc.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +# Test gdbserver to properly handle a program arg with a space in it
> +# as a single argument.
> +
> +load_lib gdbserver-support.exp
> +
> +standard_testfile "normal.c"
> +
> +if {[skip_gdbserver_tests]} {
> +    return 0
> +}
> +
> +if {[gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } {
> +    return -1
> +}
> +clean_restart ${testfile}
> +
> +gdbserver_run "\"hello world\""
> +
> +gdb_breakpoint main
> +gdb_continue main
> +
> +# check that program arg was passed properly
> +gdb_test "print argc" "2"
> +gdb_test "print argv\[1\]" "\"hello world\""
> -- 
> 2.26.2
> 

It would be nice to write a test in such a way that it can cover this use case
when using GDB (without GDBserver) too.

It's possible to run the same test using plain GDB with:

  make check TESTS="gdb.base/foo.exp"

or

  make check TESTS="gdb.base/foo.exp" RUNTESTFLAGS="--target_board=unix"

unix is the default DejaGNU (runtest) board, so these two are equivalent.

It's also possible to run a test using gdbserver as a target:

  make check TESTS="gdb.base/foo.exp" RUNTESTFLAGS="--target_board=native-gdbserver"
  make check TESTS="gdb.base/foo.exp" RUNTESTFLAGS="--target_board=native-extended-gdbserver"

So it should be possible to write a test that runs a process with arguments
containing spaces.  However, it's not currently possible with the testsuite
to pass arguments to a test program using the native-gdbserver board.  These
arguments would have to be forwarded all the way to gdbserver's command line,
exactly like when you start gdbserver by hand.  This will require a bit of
testsuite refactoring, I'll try to look into it.

With native-extended-gdbserver, the args could be passed to the `run` command,
just like the native debugging case.  It's by playing with this that I noticed
gdbserver crashing with this patchset applied when trying to remotely run a
process with arguments.

Simon

[PATCH 1/2] gdb/testsuite: support passing inferior arguments with native-gdbserver board

[Simon Marchi]

This patch makes it possible to run tests requiring passing arguments to
the inferior with the native-gdbserver board.  The end goal is to write
a test that verifies passing arguments to the inferior works, and to
have that test exercise inferior arguments passed on the gdbserver
command line, when using the native-gdbserver target board (in addition
to the other boards).  This is done in the next patch.

With the native-gdbserver target board, gdbserver is started in
gdb_reload (implemented in config/gdbserver.exp), called in gdb_run_cmd.
gdb_run_cmd already supposedly accepts inferior arguments (although that
feature does not seem to be used anywhere), which it passes to the `run`
command, for non-stub target boards.  I've changed gdb_run_cmd so that
it forwards these arguments to gdb_reload as well.  gdb_reload passes
them to gdbserver_run, and they eventually make their way to the
gdbserver command line.

gdb_run_cmd currently accepts `args` (the varargs of tcl), which means
it receives inferior arguments as a list.  This won't work with
arguments with spaces, because they will end up being formatted with
curly braces like this:

    % set args [list hello "with spaces" world]
    hello {with spaces} world
    % puts "run $args"
    run hello {with spaces} world

I've changed it to accept a single string that is passed to `run` and
gdb_reload.  I've done the same change in gdb_start_cmd and
gdb_starti_cmd, although these two are not used with native-gdbserver.

I've changed all gdb_reload implementations in the tree to accept a new
inferior_args argument, although most of them don't do anything with it
(and don't need to).  People maintaining target boards out of tree will
need to do the same.

I found two tests to adjust to avoid adding new failures or errors.
These tests needed new [use_gdb_stub] checks, because they rely on
having GDB run new processes.  These are guarded by a [target_info
exists noargs], which made them get skipped on native-gdbserver.  But
now that the native-gdbserver board supports args, this is no longer
enough.

Note that with this change, noargs and use_gdb_stub are orthogonal.  It
took me a moment to grasp this, so I thought I would spell out the
different possible situations:

- !noargs and !use_gdb_stub: inferior process started by gdb, can pass
  args
- noargs and !use_gdb_stub: inferior process started by gdb (perhaps
  through extended-remote protocol, the simulator, some other target),
  but that target doesn't support inferior arguments
- noargs and use_gdb_stub: inferior process started by some other
  program to which GDB connects using the remote protocol, that program
  does not support passing args to the inferior process
- !noargs and use_gdb_stub: inferior process started by some other
  program to which GDB connects u sing the remote protocol, that program
  supports passing args to the inferior process

gdb/testsuite/ChangeLog:

	* lib/gdb.exp (gdb_run_cmd): Change argument from args to
	inferior_args.  Pass it to gdb_reload.
	(gdb_start_cmd, gdb_starti_cmd): Change argument from args to
	inferior_args.
	(gdb_reload): Add inferior_args argument.
	* config/gdbserver.exp (gdb_reload): Add inferior_args argument,
	pass it to gdbserver_run.
	* boards/native-gdbserver.exp: Do not set noargs.
	* boards/native-extended-gdbserver.exp (gdb_reload): Add
	inferior_args argument.
	* boards/stdio-gdbserver-base.exp (gdb_reload): Likewise.
	* gdb.base/a2-run.exp: Check for use_gdb_stub.
	* gdb.base/args.exp: Likewise.
---
 .../boards/native-extended-gdbserver.exp      |  2 +-
 gdb/testsuite/boards/native-gdbserver.exp     |  3 --
 gdb/testsuite/boards/stdio-gdbserver-base.exp |  2 +-
 gdb/testsuite/config/gdbserver.exp            | 12 +++----
 gdb/testsuite/gdb.base/a2-run.exp             | 11 +++++-
 gdb/testsuite/gdb.base/args.exp               |  6 ++++
 gdb/testsuite/lib/gdb.exp                     | 34 +++++++++++++------
 7 files changed, 46 insertions(+), 24 deletions(-)

diff --git a/gdb/testsuite/boards/native-extended-gdbserver.exp b/gdb/testsuite/boards/native-extended-gdbserver.exp
index 465c1cc8ab8a..3d5e78211462 100644
--- a/gdb/testsuite/boards/native-extended-gdbserver.exp
+++ b/gdb/testsuite/boards/native-extended-gdbserver.exp
@@ -102,7 +102,7 @@ proc gdb_file_cmd { arg } {
     return [extended_gdbserver_load_last_file]
 }
 
-proc gdb_reload { } {
+proc gdb_reload { {inferior_args {}} } {
     return [extended_gdbserver_load_last_file]
 }
 
diff --git a/gdb/testsuite/boards/native-gdbserver.exp b/gdb/testsuite/boards/native-gdbserver.exp
index f9a507261fff..823f5cfa3ad1 100644
--- a/gdb/testsuite/boards/native-gdbserver.exp
+++ b/gdb/testsuite/boards/native-gdbserver.exp
@@ -27,9 +27,6 @@ load_board_description "local-board"
 # This gdbserver can only run a process once per session.
 set_board_info gdb,do_reload_on_run 1
 
-# There's no support for argument-passing (yet).
-set_board_info noargs 1
-
 set_board_info use_gdb_stub 1
 set_board_info exit_is_reliable 1
 
diff --git a/gdb/testsuite/boards/stdio-gdbserver-base.exp b/gdb/testsuite/boards/stdio-gdbserver-base.exp
index aafcf6991b2c..720e2e823fa8 100644
--- a/gdb/testsuite/boards/stdio-gdbserver-base.exp
+++ b/gdb/testsuite/boards/stdio-gdbserver-base.exp
@@ -45,7 +45,7 @@ proc make_gdbserver_stdio_port {} {
     return "| [get_target_remote_pipe_cmd]"
 }
 
-proc gdb_reload { } {
+proc gdb_reload { {inferior_args {}} } {
     return [gdb_target_cmd "remote" [make_gdbserver_stdio_port]]
 }
 
diff --git a/gdb/testsuite/config/gdbserver.exp b/gdb/testsuite/config/gdbserver.exp
index cb053b63e4c0..a4e935f214d2 100644
--- a/gdb/testsuite/config/gdbserver.exp
+++ b/gdb/testsuite/config/gdbserver.exp
@@ -36,12 +36,8 @@
 #	Unles you have a gdbserver that can handle multiple sessions.
 #
 #   set_board_info noargs 1
-#	At present there is no provision in the remote protocol
-#	for passing arguments.  This test framework does not
-#	address the issue, so it's best to set this variable
-#	in your baseboard configuration file.  
-#	FIXME: there's no reason why the test harness couldn't
-#	pass commandline args when it spawns gdbserver.
+#	Set this if the board does not support passing arguments to the
+#	inferior process.
 #
 #   set_board_info gdb,noinferiorio 1
 #	Neither the traditional gdbserver nor the one in libremote
@@ -77,8 +73,8 @@ proc gdbserver_gdb_load { } {
     return [gdbserver_spawn ""]
 }
 
-proc gdb_reload { } {
-    return [gdbserver_run ""]
+proc gdb_reload { {inferior_args {}} } {
+    return [gdbserver_run $inferior_args]
 }
 
 proc gdb_reconnect { } {
diff --git a/gdb/testsuite/gdb.base/a2-run.exp b/gdb/testsuite/gdb.base/a2-run.exp
index 2e8ed60ec650..ea8f7ec95f49 100644
--- a/gdb/testsuite/gdb.base/a2-run.exp
+++ b/gdb/testsuite/gdb.base/a2-run.exp
@@ -135,7 +135,7 @@ gdb_run_cmd 5
 gdb_test_stdio "" "120" "" "run \"$testfile\" with arg"
 
 # Run again with same arguments.
-gdb_run_cmd
+gdb_run_cmd 5
 
 setup_xfail "arm-*-coff"
 gdb_test_stdio "" "120" "" "run \"$testfile\" again with same args"
@@ -147,6 +147,15 @@ gdb_run_cmd
 
 gdb_test_stdio "" "usage:  factorial <number>" "" "run after setting args to nil"
 
+# The remaining tests pass inferior arguments through GDB, so doesn't
+# work with stub targets, where GDB connects to debug an already started
+# process.
+
+if [use_gdb_stub] {
+    verbose "Skipping rest of a2-run.exp because target is a stub."
+    return
+}
+
 # Use "set args" command to specify an argument and run again.
 gdb_test_no_output "set args 6"
 
diff --git a/gdb/testsuite/gdb.base/args.exp b/gdb/testsuite/gdb.base/args.exp
index 6c416fcfc8ee..ee75445c0b7e 100644
--- a/gdb/testsuite/gdb.base/args.exp
+++ b/gdb/testsuite/gdb.base/args.exp
@@ -23,6 +23,12 @@ if [target_info exists noargs] {
     return
 }
 
+# This test requires starting new inferior processes, skip it if the target
+# board is a stub.
+if [use_gdb_stub] {
+    return
+}
+
 standard_testfile
 
 if {[build_executable $testfile.exp $testfile \
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 3c6f0d76d67d..ec77cfb0d318 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -245,10 +245,13 @@ proc target_can_use_run_cmd {} {
 # Using ``.*$'' could swallow up output that we attempt to match
 # elsewhere.
 #
+# INFERIOR_ARGS is passed as arguments to the start command, so may contain
+# inferior arguments.
+#
 # N.B. This function does not wait for gdb to return to the prompt,
 # that is the caller's responsibility.
 
-proc gdb_run_cmd {args} {
+proc gdb_run_cmd { {inferior_args {}} } {
     global gdb_prompt use_gdb_stub
 
     foreach command [gdb_init_commands] {
@@ -264,7 +267,7 @@ proc gdb_run_cmd {args} {
 
     if $use_gdb_stub {
 	if [target_info exists gdb,do_reload_on_run] {
-	    if { [gdb_reload] != 0 } {
+	    if { [gdb_reload $inferior_args] != 0 } {
 		return
 	    }
 	    send_gdb "continue\n"
@@ -309,7 +312,7 @@ proc gdb_run_cmd {args} {
 		    send_gdb "y\n" answer
 		}
 		-re "The program is not being run.*$gdb_prompt $" {
-		    if { [gdb_reload] != 0 } {
+		    if { [gdb_reload $inferior_args] != 0 } {
 			return
 		    }
 		    send_gdb "jump *$start\n"
@@ -324,11 +327,11 @@ proc gdb_run_cmd {args} {
     }
 
     if [target_info exists gdb,do_reload_on_run] {
-	if { [gdb_reload] != 0 } {
+	if { [gdb_reload $inferior_args] != 0 } {
 	    return
 	}
     }
-    send_gdb "run $args\n"
+    send_gdb "run $inferior_args\n"
 # This doesn't work quite right yet.
 # Use -notransfer here so that test cases (like chng-sym.exp)
 # may test for additional start-up messages.
@@ -347,10 +350,13 @@ proc gdb_run_cmd {args} {
 # Generic start command.  Return 0 if we could start the program, -1
 # if we could not.
 #
+# INFERIOR_ARGS is passed as arguments to the start command, so may contain
+# inferior arguments.
+#
 # N.B. This function does not wait for gdb to return to the prompt,
 # that is the caller's responsibility.
 
-proc gdb_start_cmd {args} {
+proc gdb_start_cmd { {inferior_args {}} } {
     global gdb_prompt use_gdb_stub
 
     foreach command [gdb_init_commands] {
@@ -368,7 +374,7 @@ proc gdb_start_cmd {args} {
 	return -1
     }
 
-    send_gdb "start $args\n"
+    send_gdb "start $inferior_args\n"
     # Use -notransfer here so that test cases (like chng-sym.exp)
     # may test for additional start-up messages.
     gdb_expect 60 {
@@ -386,10 +392,13 @@ proc gdb_start_cmd {args} {
 # Generic starti command.  Return 0 if we could start the program, -1
 # if we could not.
 #
+# INFERIOR_ARGS is passed as arguments to the starti command, so may contain
+# inferior arguments.
+#
 # N.B. This function does not wait for gdb to return to the prompt,
 # that is the caller's responsibility.
 
-proc gdb_starti_cmd {args} {
+proc gdb_starti_cmd { {inferior_args {}} } {
     global gdb_prompt use_gdb_stub
 
     foreach command [gdb_init_commands] {
@@ -407,7 +416,7 @@ proc gdb_starti_cmd {args} {
 	return -1
     }
 
-    send_gdb "starti $args\n"
+    send_gdb "starti $inferior_args\n"
     gdb_expect 60 {
 	-re "The program .* has been started already.*y or n. $" {
 	    send_gdb "y\n" answer
@@ -4817,8 +4826,13 @@ proc gdb_load { arg } {
 # either the first time or after already starting the program once,
 # for remote targets.  Most files that override gdb_load should now
 # override this instead.
+#
+# INFERIOR_ARGS contains the arguments to pass to the inferiors, as a
+# single string to get interpreted by a shell.  If the target board
+# overriding gdb_reload is a "stub", then it should arrange things such
+# these arguments make their way to the inferior process.
 
-proc gdb_reload { } {
+proc gdb_reload { {inferior_args {}} } {
     # For the benefit of existing configurations, default to gdb_load.
     # Specifying no file defaults to the executable currently being
     # debugged.
-- 
2.26.2

[PATCH 2/2] gdb/testsuite: add inferior arguments test

[Simon Marchi]

Add a test for verifying different methods of passing arguments to the
inferior: the start, starti and run commands, as well as `set args`.

All these methods work naturally when using the unix or
native-extended-gdbserver target boards.  Since those are non-stub
boards, GDB runs new inferiors and therefore pass arguments to them.
With target boards where GDB connects to a stub, for example with
native-gdbserver, they don't really make sense.  The inferior process is
already started when GDB connects.

However, the "run" method is still tested with stub targets, because the
gdb_run_cmd procedure is adapted for stub targets.  Instead of issuing
the `run` command, it spawns whatever program is supposed to bring up
the stub (gdbserver, for example) using gdb_reload and makes GDB connect
to it.  So this allows us to exercise argument passing through the
gdbserver command line, when testing with the native-gdbserver board.

Note that there is already a gdb.base/args.exp, but this tests
specifically the --args switch of GDB.  Perhaps it could be integrated
in this new test, as a new "method".

gdb/testsuite/ChangeLog:

	* lib/gdb.exp (gdb_run_cmd): Return success or failure.
	* gdb.base/inferior-args.exp: New file.
	* gdb.base/inferior-args.c: New file.
---
 gdb/testsuite/gdb.base/inferior-args.c   |   8 ++
 gdb/testsuite/gdb.base/inferior-args.exp | 126 +++++++++++++++++++++++
 gdb/testsuite/lib/gdb.exp                |  21 ++--
 3 files changed, 147 insertions(+), 8 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/inferior-args.c
 create mode 100644 gdb/testsuite/gdb.base/inferior-args.exp

diff --git a/gdb/testsuite/gdb.base/inferior-args.c b/gdb/testsuite/gdb.base/inferior-args.c
new file mode 100644
index 000000000000..74ca58b50852
--- /dev/null
+++ b/gdb/testsuite/gdb.base/inferior-args.c
@@ -0,0 +1,8 @@
+#include <stdio.h>
+
+int main(int argc, char **argv) {
+	for (int i = 0; i < argc; i++)
+		printf("[%d] %s\n", i, argv[i]);
+
+	return 0;
+}
diff --git a/gdb/testsuite/gdb.base/inferior-args.exp b/gdb/testsuite/gdb.base/inferior-args.exp
new file mode 100644
index 000000000000..dcda102fe52b
--- /dev/null
+++ b/gdb/testsuite/gdb.base/inferior-args.exp
@@ -0,0 +1,126 @@
+# Copyright 2020 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Test running an inferior with arguments.
+
+# This does not work on boards that don't support inferior arguments.
+if [target_info exists noargs] then {
+    verbose "skipping gdb.base/inferior-args.exp because of noargs"
+    return
+}
+
+standard_testfile .c
+
+if {[build_executable "failed to prepare" $testfile $srcfile debug] == -1} {
+    return
+}
+
+clean_restart $binfile
+
+proc do_test { method } {
+    global binfile hex
+
+    # The second arg is an empty string on purpose.
+    set inferior_args { "first arg" "" "third-arg" }
+
+    clean_restart $binfile
+
+    if { $method == "start" } {
+	# The start command does not make sense for a stub.
+	if { [use_gdb_stub] } {
+	    return;
+	}
+
+	if { [gdb_start_cmd $inferior_args] < 0 } {
+	    fail "could not issue start command"
+	    return -1
+	}
+
+	# Consume up to the GDB prompt after the stop.
+	gdb_test "" ".*main.*" "stop at main"
+
+    } elseif { $method == "starti" } {
+	# The starti command does not make sense for a stub.
+	if { [use_gdb_stub] } {
+	    return;
+	}
+
+	if { [gdb_starti_cmd $inferior_args] < 0 } {
+	    fail "could not issue start command"
+	    return -1
+	}
+
+	# Consume up to the GDB prompt after the stop.
+	gdb_test "" "" "stop at first instruction"
+
+	# Put a breakpoint and continue until main.
+	if { ![gdb_breakpoint "main" message] } {
+	    fail "could not set breakpoint on main"
+	    return -1
+	}
+
+	if { [gdb_continue "main"] != 0 } {
+	    fail "could not continue to main"
+	    return -1
+	}
+
+    } elseif { $method == "run" } {
+	if { ![gdb_breakpoint "main" message] } {
+	    fail "could not set breakpoint on main"
+	    return -1
+	}
+
+	# The run command does not make sense for a stub, but GDB_RUN_CMD
+	# does the right thing when the target is a stub (start the stub,
+	# connect to it, and "continue").
+	#
+	# This allows us to test arguments passed on the gdbserver command
+	# line.
+	if { [gdb_run_cmd $inferior_args] < 0 } {
+	    fail "could not run"
+	    return -1
+	}
+
+	# Consume up to the GDB prompt after the stop.
+	gdb_test "" ".*main.*" "stop at main"
+
+    } elseif { $method == "set args" } {
+	# Using "set args" does not make sense with a stub.
+	if { [use_gdb_stub] } {
+	    return;
+	}
+
+	gdb_test_no_output "set args $inferior_args"
+
+	if { ![runto_main] } {
+	    fail "could not run to main"
+	    return -1
+	}
+
+    } else {
+	error "invalid method $method"
+    }
+
+    # Now that we are stopped at main, inspect argc/argv.
+    gdb_test "print argc" " = 4"
+    gdb_test "print argv\[0\]" " = $hex \".*\""
+    gdb_test "print argv\[1\]" " = $hex \"first arg\""
+    gdb_test "print argv\[2\]" " = $hex \"\""
+    gdb_test "print argv\[3\]" " = $hex \"third-arg\""
+}
+
+foreach_with_prefix method { "start" "starti" "run" "set args" } {
+    do_test $method
+}
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index ec77cfb0d318..a5eeba6b3cd6 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -241,6 +241,8 @@ proc target_can_use_run_cmd {} {
 
 # Generic run command.
 #
+# Return 0 if we could start the program, -1 if we could not.
+#
 # The second pattern below matches up to the first newline *only*.
 # Using ``.*$'' could swallow up output that we attempt to match
 # elsewhere.
@@ -268,14 +270,14 @@ proc gdb_run_cmd { {inferior_args {}} } {
     if $use_gdb_stub {
 	if [target_info exists gdb,do_reload_on_run] {
 	    if { [gdb_reload $inferior_args] != 0 } {
-		return
+		return -1
 	    }
 	    send_gdb "continue\n"
 	    gdb_expect 60 {
 		-re "Continu\[^\r\n\]*\[\r\n\]" {}
 		default {}
 	    }
-	    return
+	    return 0
 	}
 
 	if [target_info exists gdb,start_symbol] {
@@ -291,7 +293,7 @@ proc gdb_run_cmd { {inferior_args {}} } {
 	    # clever and not send a command when it has failed.
 	    if [expr $start_attempt > 3] {
 		perror "Jump to start() failed (retry count exceeded)"
-		return
+		return -1
 	    }
 	    set start_attempt [expr $start_attempt + 1]
 	    gdb_expect 30 {
@@ -300,7 +302,7 @@ proc gdb_run_cmd { {inferior_args {}} } {
 		}
 		-re "No symbol \"_start\" in current.*$gdb_prompt $" {
 		    perror "Can't find start symbol to run in gdb_run"
-		    return
+		    return -1
 		}
 		-re "No symbol \"start\" in current.*$gdb_prompt $" {
 		    send_gdb "jump *_start\n"
@@ -313,22 +315,23 @@ proc gdb_run_cmd { {inferior_args {}} } {
 		}
 		-re "The program is not being run.*$gdb_prompt $" {
 		    if { [gdb_reload $inferior_args] != 0 } {
-			return
+			return -1
 		    }
 		    send_gdb "jump *$start\n"
 		}
 		timeout {
 		    perror "Jump to start() failed (timeout)"
-		    return
+		    return -1
 		}
 	    }
 	}
-	return
+
+	return 0
     }
 
     if [target_info exists gdb,do_reload_on_run] {
 	if { [gdb_reload $inferior_args] != 0 } {
-	    return
+	    return -1
 	}
     }
     send_gdb "run $inferior_args\n"
@@ -345,6 +348,8 @@ proc gdb_run_cmd { {inferior_args {}} } {
 	    # There is no more input expected.
 	}
     }
+
+    return 0
 }
 
 # Generic start command.  Return 0 if we could start the program, -1
-- 
2.26.2

Re: [PATCH 4/4] [PR gdbserver/25893]: Add gdbserver test for argument with space in it

[Simon Marchi]

On 2020-05-07 6:15 p.m., Simon Marchi wrote:
> It would be nice to write a test in such a way that it can cover this use case
> when using GDB (without GDBserver) too.
> 
> It's possible to run the same test using plain GDB with:
> 
>   make check TESTS="gdb.base/foo.exp"
> 
> or
> 
>   make check TESTS="gdb.base/foo.exp" RUNTESTFLAGS="--target_board=unix"
> 
> unix is the default DejaGNU (runtest) board, so these two are equivalent.
> 
> It's also possible to run a test using gdbserver as a target:
> 
>   make check TESTS="gdb.base/foo.exp" RUNTESTFLAGS="--target_board=native-gdbserver"
>   make check TESTS="gdb.base/foo.exp" RUNTESTFLAGS="--target_board=native-extended-gdbserver"
> 
> So it should be possible to write a test that runs a process with arguments
> containing spaces.  However, it's not currently possible with the testsuite
> to pass arguments to a test program using the native-gdbserver board.  These
> arguments would have to be forwarded all the way to gdbserver's command line,
> exactly like when you start gdbserver by hand.  This will require a bit of
> testsuite refactoring, I'll try to look into it.
> 
> With native-extended-gdbserver, the args could be passed to the `run` command,
> just like the native debugging case.  It's by playing with this that I noticed
> gdbserver crashing with this patchset applied when trying to remotely run a
> process with arguments.
> 
> Simon

Hi Michael,

Can you please check the patches I proposed for such a test?

  https://sourceware.org/pipermail/gdb-patches/2020-May/168262.html
  https://sourceware.org/pipermail/gdb-patches/2020-May/168263.html

Your fix should make this work:

  make check TESTS="gdb.base/inferior-args.exp" RUNTESTFLAGS="--target_board=native-gdbserver"

But these two should work as well:

  make check TESTS="gdb.base/inferior-args.exp" RUNTESTFLAGS="--target_board=unix"
  make check TESTS="gdb.base/inferior-args.exp" RUNTESTFLAGS="--target_board=native-extended-gdbserver"

Simon

Re: [PATCH 4/4] [PR gdbserver/25893]: Add gdbserver test for argument with space in it

[Michael Weghorn]

On 10/05/2020 18.09, Simon Marchi wrote:
> On 2020-05-07 6:15 p.m., Simon Marchi wrote:
>> It would be nice to write a test in such a way that it can cover this use case
>> when using GDB (without GDBserver) too.
>>
>> It's possible to run the same test using plain GDB with:
>>
>>   make check TESTS="gdb.base/foo.exp"
>>
>> or
>>
>>   make check TESTS="gdb.base/foo.exp" RUNTESTFLAGS="--target_board=unix"
>>
>> unix is the default DejaGNU (runtest) board, so these two are equivalent.
>>
>> It's also possible to run a test using gdbserver as a target:
>>
>>   make check TESTS="gdb.base/foo.exp" RUNTESTFLAGS="--target_board=native-gdbserver"
>>   make check TESTS="gdb.base/foo.exp" RUNTESTFLAGS="--target_board=native-extended-gdbserver"
>>
>> So it should be possible to write a test that runs a process with arguments
>> containing spaces.  However, it's not currently possible with the testsuite
>> to pass arguments to a test program using the native-gdbserver board.  These
>> arguments would have to be forwarded all the way to gdbserver's command line,
>> exactly like when you start gdbserver by hand.  This will require a bit of
>> testsuite refactoring, I'll try to look into it.
>>
>> With native-extended-gdbserver, the args could be passed to the `run` command,
>> just like the native debugging case.  It's by playing with this that I noticed
>> gdbserver crashing with this patchset applied when trying to remotely run a
>> process with arguments.
>>
>> Simon
> 
> Hi Michael,
> 

Hi Simon,

> Can you please check the patches I proposed for such a test?
> 
>   https://sourceware.org/pipermail/gdb-patches/2020-May/168262.html
>   https://sourceware.org/pipermail/gdb-patches/2020-May/168263.html
> 
> Your fix should make this work:
> 
>   make check TESTS="gdb.base/inferior-args.exp" RUNTESTFLAGS="--target_board=native-gdbserver"

Indeed, this works fine. Thanks a lot for extending the test suite.

> 
> But these two should work as well:
> 
>   make check TESTS="gdb.base/inferior-args.exp" RUNTESTFLAGS="--target_board=unix"
>   make check TESTS="gdb.base/inferior-args.exp" RUNTESTFLAGS="--target_board=native-extended-gdbserver"

These don't work yet with the last patch series. I'll recheck once I
have finished updating the patch series and in particular fixed the
segfault you mentioned earlier.

I hope to be able to send an updated patch series tomorrow.

Do I understand correctly that I can just drop my patch adding the test,
since this is now already covered in a better way by the test case added
in your second patch?

Michael

Re: [PATCH 4/4] [PR gdbserver/25893]: Add gdbserver test for argument with space in it

[Simon Marchi]

On 2020-05-11 10:33 a.m., Michael Weghorn wrote:
> These don't work yet with the last patch series. I'll recheck once I
> have finished updating the patch series and in particular fixed the
> segfault you mentioned earlier.
> 
> I hope to be able to send an updated patch series tomorrow.
> 
> Do I understand correctly that I can just drop my patch adding the test,
> since this is now already covered in a better way by the test case added
> in your second patch?

I think so.  You can insert my two patches in your series, as the test depends
on the fix.

If you haven't found already: after running the `make check` commands, the
transcript is found in testsuite/gdb.log.

Simon

[PATCH v3 1/6] gdb: Move construct_inferior_arguments to gdbsupport

[Michael Weghorn]

This moves the function construct_inferior_arguments from
gdb/inferior.h and gdb/infcmd.c to gdbsupport/common-inferior.{h,cc}.
While at it, also move the function's comment to the header file
to align with current standards.

The intention is to use it from gdbserver in a follow-up commit.

gdb/ChangeLog:

2020-05-12  Michael Weghorn  <m.weghorn@posteo.de>

        * infcmd.c, inferior.h: (construct_inferior_arguments):
          Moved function from here to gdbsupport/common-inferior.{h,cc}

gdbsupport/ChangeLog:

2020-05-12  Michael Weghorn  <m.weghorn@posteo.de>

        * common-inferior.h, common-inferior.cc: (construct_inferior_arguments):
        Move function here from gdb/infcmd.c, gdb/inferior.h
---
 gdb/infcmd.c                  | 124 ----------------------------------
 gdb/inferior.h                |   2 -
 gdbsupport/common-inferior.cc | 122 +++++++++++++++++++++++++++++++++
 gdbsupport/common-inferior.h  |   4 ++
 4 files changed, 126 insertions(+), 126 deletions(-)

diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 9bbb413d4e..8f7482347c 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -259,130 +259,6 @@ server's cwd if remote debugging.\n"));
 			"when starting the inferior is \"%s\".\n"), cwd);
 }
 
-\f
-/* Compute command-line string given argument vector.  This does the
-   same shell processing as fork_inferior.  */
-
-char *
-construct_inferior_arguments (int argc, char **argv)
-{
-  char *result;
-
-  /* ARGC should always be at least 1, but we double check this
-     here.  This is also needed to silence -Werror-stringop
-     warnings.  */
-  gdb_assert (argc > 0);
-
-  if (startup_with_shell)
-    {
-#ifdef __MINGW32__
-      /* This holds all the characters considered special to the
-	 Windows shells.  */
-      static const char special[] = "\"!&*|[]{}<>?`~^=;, \t\n";
-      static const char quote = '"';
-#else
-      /* This holds all the characters considered special to the
-	 typical Unix shells.  We include `^' because the SunOS
-	 /bin/sh treats it as a synonym for `|'.  */
-      static const char special[] = "\"!#$&*()\\|[]{}<>?'`~^; \t\n";
-      static const char quote = '\'';
-#endif
-      int i;
-      int length = 0;
-      char *out, *cp;
-
-      /* We over-compute the size.  It shouldn't matter.  */
-      for (i = 0; i < argc; ++i)
-	length += 3 * strlen (argv[i]) + 1 + 2 * (argv[i][0] == '\0');
-
-      result = (char *) xmalloc (length);
-      out = result;
-
-      for (i = 0; i < argc; ++i)
-	{
-	  if (i > 0)
-	    *out++ = ' ';
-
-	  /* Need to handle empty arguments specially.  */
-	  if (argv[i][0] == '\0')
-	    {
-	      *out++ = quote;
-	      *out++ = quote;
-	    }
-	  else
-	    {
-#ifdef __MINGW32__
-	      int quoted = 0;
-
-	      if (strpbrk (argv[i], special))
-		{
-		  quoted = 1;
-		  *out++ = quote;
-		}
-#endif
-	      for (cp = argv[i]; *cp; ++cp)
-		{
-		  if (*cp == '\n')
-		    {
-		      /* A newline cannot be quoted with a backslash (it
-			 just disappears), only by putting it inside
-			 quotes.  */
-		      *out++ = quote;
-		      *out++ = '\n';
-		      *out++ = quote;
-		    }
-		  else
-		    {
-#ifdef __MINGW32__
-		      if (*cp == quote)
-#else
-		      if (strchr (special, *cp) != NULL)
-#endif
-			*out++ = '\\';
-		      *out++ = *cp;
-		    }
-		}
-#ifdef __MINGW32__
-	      if (quoted)
-		*out++ = quote;
-#endif
-	    }
-	}
-      *out = '\0';
-    }
-  else
-    {
-      /* In this case we can't handle arguments that contain spaces,
-	 tabs, or newlines -- see breakup_args().  */
-      int i;
-      int length = 0;
-
-      for (i = 0; i < argc; ++i)
-	{
-	  char *cp = strchr (argv[i], ' ');
-	  if (cp == NULL)
-	    cp = strchr (argv[i], '\t');
-	  if (cp == NULL)
-	    cp = strchr (argv[i], '\n');
-	  if (cp != NULL)
-	    error (_("can't handle command-line "
-		     "argument containing whitespace"));
-	  length += strlen (argv[i]) + 1;
-	}
-
-      result = (char *) xmalloc (length);
-      result[0] = '\0';
-      for (i = 0; i < argc; ++i)
-	{
-	  if (i > 0)
-	    strcat (result, " ");
-	  strcat (result, argv[i]);
-	}
-    }
-
-  return result;
-}
-\f
 
 /* This function strips the '&' character (indicating background
    execution) that is added as *the last* of the arguments ARGS of a
diff --git a/gdb/inferior.h b/gdb/inferior.h
index 1ac51369df..95af474eed 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -184,8 +184,6 @@ extern void child_interrupt (struct target_ops *self);
    STARTUP_INFERIOR.  */
 extern ptid_t gdb_startup_inferior (pid_t pid, int num_traps);
 
-extern char *construct_inferior_arguments (int, char **);
-
 /* From infcmd.c */
 
 /* Initial inferior setup.  Determines the exec file is not yet known,
diff --git a/gdbsupport/common-inferior.cc b/gdbsupport/common-inferior.cc
index ed16e89a52..a7d631f357 100644
--- a/gdbsupport/common-inferior.cc
+++ b/gdbsupport/common-inferior.cc
@@ -24,3 +24,125 @@
 /* See common-inferior.h.  */
 
 bool startup_with_shell = true;
+
+/* See common-inferior.h.  */
+
+char *
+construct_inferior_arguments (int argc, char **argv)
+{
+  char *result;
+
+  /* ARGC should always be at least 1, but we double check this
+     here.  This is also needed to silence -Werror-stringop
+     warnings.  */
+  gdb_assert (argc > 0);
+
+  if (startup_with_shell)
+    {
+#ifdef __MINGW32__
+      /* This holds all the characters considered special to the
+	 Windows shells.  */
+      static const char special[] = "\"!&*|[]{}<>?`~^=;, \t\n";
+      static const char quote = '"';
+#else
+      /* This holds all the characters considered special to the
+	 typical Unix shells.  We include `^' because the SunOS
+	 /bin/sh treats it as a synonym for `|'.  */
+      static const char special[] = "\"!#$&*()\\|[]{}<>?'`~^; \t\n";
+      static const char quote = '\'';
+#endif
+      int i;
+      int length = 0;
+      char *out, *cp;
+
+      /* We over-compute the size.  It shouldn't matter.  */
+      for (i = 0; i < argc; ++i)
+	length += 3 * strlen (argv[i]) + 1 + 2 * (argv[i][0] == '\0');
+
+      result = (char *) xmalloc (length);
+      out = result;
+
+      for (i = 0; i < argc; ++i)
+	{
+	  if (i > 0)
+	    *out++ = ' ';
+
+	  /* Need to handle empty arguments specially.  */
+	  if (argv[i][0] == '\0')
+	    {
+	      *out++ = quote;
+	      *out++ = quote;
+	    }
+	  else
+	    {
+#ifdef __MINGW32__
+	      int quoted = 0;
+
+	      if (strpbrk (argv[i], special))
+		{
+		  quoted = 1;
+		  *out++ = quote;
+		}
+#endif
+	      for (cp = argv[i]; *cp; ++cp)
+		{
+		  if (*cp == '\n')
+		    {
+		      /* A newline cannot be quoted with a backslash (it
+			 just disappears), only by putting it inside
+			 quotes.  */
+		      *out++ = quote;
+		      *out++ = '\n';
+		      *out++ = quote;
+		    }
+		  else
+		    {
+#ifdef __MINGW32__
+		      if (*cp == quote)
+#else
+		      if (strchr (special, *cp) != NULL)
+#endif
+			*out++ = '\\';
+		      *out++ = *cp;
+		    }
+		}
+#ifdef __MINGW32__
+	      if (quoted)
+		*out++ = quote;
+#endif
+	    }
+	}
+      *out = '\0';
+    }
+  else
+    {
+      /* In this case we can't handle arguments that contain spaces,
+	 tabs, or newlines -- see breakup_args().  */
+      int i;
+      int length = 0;
+
+      for (i = 0; i < argc; ++i)
+	{
+	  char *cp = strchr (argv[i], ' ');
+	  if (cp == NULL)
+	    cp = strchr (argv[i], '\t');
+	  if (cp == NULL)
+	    cp = strchr (argv[i], '\n');
+	  if (cp != NULL)
+	    error (_("can't handle command-line "
+		     "argument containing whitespace"));
+	  length += strlen (argv[i]) + 1;
+	}
+
+      result = (char *) xmalloc (length);
+      result[0] = '\0';
+      for (i = 0; i < argc; ++i)
+	{
+	  if (i > 0)
+	    strcat (result, " ");
+	  strcat (result, argv[i]);
+	}
+    }
+
+  return result;
+}
diff --git a/gdbsupport/common-inferior.h b/gdbsupport/common-inferior.h
index 3c8e87aee6..ee87bc75a3 100644
--- a/gdbsupport/common-inferior.h
+++ b/gdbsupport/common-inferior.h
@@ -58,4 +58,8 @@ extern void set_inferior_cwd (const char *cwd);
    the target is started up with a shell.  */
 extern bool startup_with_shell;
 
+/* Compute command-line string given argument vector. This does the
+   same shell processing as fork_inferior.  */
+extern char *construct_inferior_arguments (int, char **);
+
 #endif /* COMMON_COMMON_INFERIOR_H */
-- 
2.26.2

[PATCH v3 2/6] gdbsupport: Adapt construct_inferior_arguments

[Michael Weghorn]

Allow construct_inferior_arguments to handle zero args
and have it return a std::string, similar to how
stringify_argv in gdbsupport/common-utils does.

Also, add a const qualifier for the second parameter,
since it is only read, not written to.

The intention is to replace existing uses of
stringify_argv by construct_inferior_arguments
in a subsequent step, since construct_inferior_arguments
properly handles special characters, while stringify_argv
doesn't.

2020-05-12  Michael Weghorn  <m.weghorn@posteo.de>

        * common-inferior.cc, common-inferior.h (construct_inferior_arguments):
        Adapt to handle zero args and return a std::string.
        Adapt call site.
---
 gdb/infcmd.c                  |  9 ++----
 gdbsupport/common-inferior.cc | 55 ++++++++++++-----------------------
 gdbsupport/common-inferior.h  |  2 +-
 3 files changed, 23 insertions(+), 43 deletions(-)

diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 8f7482347c..a7c037837c 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -151,12 +151,9 @@ get_inferior_args (void)
 {
   if (current_inferior ()->argc != 0)
     {
-      char *n;
-
-      n = construct_inferior_arguments (current_inferior ()->argc,
-					current_inferior ()->argv);
-      set_inferior_args (n);
-      xfree (n);
+      std::string n = construct_inferior_arguments (current_inferior ()->argc,
+					            current_inferior ()->argv);
+      set_inferior_args (n.c_str ());
     }
 
   if (current_inferior ()->args == NULL)
diff --git a/gdbsupport/common-inferior.cc b/gdbsupport/common-inferior.cc
index a7d631f357..cadbd50e9c 100644
--- a/gdbsupport/common-inferior.cc
+++ b/gdbsupport/common-inferior.cc
@@ -27,15 +27,12 @@ bool startup_with_shell = true;
 
 /* See common-inferior.h.  */
 
-char *
-construct_inferior_arguments (int argc, char **argv)
+std::string
+construct_inferior_arguments (int argc, char * const *argv)
 {
-  char *result;
+  gdb_assert (argc >= 0);
 
-  /* ARGC should always be at least 1, but we double check this
-     here.  This is also needed to silence -Werror-stringop
-     warnings.  */
-  gdb_assert (argc > 0);
+  std::string result;
 
   if (startup_with_shell)
     {
@@ -52,48 +49,39 @@ construct_inferior_arguments (int argc, char **argv)
       static const char quote = '\'';
 #endif
       int i;
-      int length = 0;
-      char *out, *cp;
-
-      /* We over-compute the size.  It shouldn't matter.  */
-      for (i = 0; i < argc; ++i)
-	length += 3 * strlen (argv[i]) + 1 + 2 * (argv[i][0] == '\0');
-
-      result = (char *) xmalloc (length);
-      out = result;
 
       for (i = 0; i < argc; ++i)
 	{
 	  if (i > 0)
-	    *out++ = ' ';
+	    result += ' ';
 
 	  /* Need to handle empty arguments specially.  */
 	  if (argv[i][0] == '\0')
 	    {
-	      *out++ = quote;
-	      *out++ = quote;
+	      result += quote;
+	      result += quote;
 	    }
 	  else
 	    {
 #ifdef __MINGW32__
-	      int quoted = 0;
+	      bool quoted = 0;
 
 	      if (strpbrk (argv[i], special))
 		{
-		  quoted = 1;
-		  *out++ = quote;
+		  quoted = true;
+		  result += quote;
 		}
 #endif
-	      for (cp = argv[i]; *cp; ++cp)
+	      for (char *cp = argv[i]; *cp; ++cp)
 		{
 		  if (*cp == '\n')
 		    {
 		      /* A newline cannot be quoted with a backslash (it
 			 just disappears), only by putting it inside
 			 quotes.  */
-		      *out++ = quote;
-		      *out++ = '\n';
-		      *out++ = quote;
+		      result += quote;
+		      result += '\n';
+		      result += quote;
 		    }
 		  else
 		    {
@@ -102,24 +90,22 @@ construct_inferior_arguments (int argc, char **argv)
 #else
 		      if (strchr (special, *cp) != NULL)
 #endif
-			*out++ = '\\';
-		      *out++ = *cp;
+			result += '\\';
+		      result += *cp;
 		    }
 		}
 #ifdef __MINGW32__
 	      if (quoted)
-		*out++ = quote;
+		result += quote;
 #endif
 	    }
 	}
-      *out = '\0';
     }
   else
     {
       /* In this case we can't handle arguments that contain spaces,
 	 tabs, or newlines -- see breakup_args().  */
       int i;
-      int length = 0;
 
       for (i = 0; i < argc; ++i)
 	{
@@ -131,16 +117,13 @@ construct_inferior_arguments (int argc, char **argv)
 	  if (cp != NULL)
 	    error (_("can't handle command-line "
 		     "argument containing whitespace"));
-	  length += strlen (argv[i]) + 1;
 	}
 
-      result = (char *) xmalloc (length);
-      result[0] = '\0';
       for (i = 0; i < argc; ++i)
 	{
 	  if (i > 0)
-	    strcat (result, " ");
-	  strcat (result, argv[i]);
+	    result += " ";
+	  result += argv[i];
 	}
     }
 
diff --git a/gdbsupport/common-inferior.h b/gdbsupport/common-inferior.h
index ee87bc75a3..5e9fc8b0b9 100644
--- a/gdbsupport/common-inferior.h
+++ b/gdbsupport/common-inferior.h
@@ -60,6 +60,6 @@ extern bool startup_with_shell;
 
 /* Compute command-line string given argument vector. This does the
    same shell processing as fork_inferior.  */
-extern char *construct_inferior_arguments (int, char **);
+extern std::string construct_inferior_arguments (int, char * const *);
 
 #endif /* COMMON_COMMON_INFERIOR_H */
-- 
2.26.2

[PATCH v3 3/6] gdbserver: Don't add extra NULL to program args

[Michael Weghorn]

The vector holding the program args is passed as a parameter
to target_create_inferior, which then passes it to
stringify_argv for all platforms, where any NULL entry in
the vector is ignored, so there seems to be no reason
to actually add one after all.

(Since the intention is to replace uses of stringify_argv with
construct_inferior_arguments in a follow-up commit and that
function doesn't currently handle such NULL arguments, it
would otherwise have to be extended.)

gdbserver/ChangeLog:

2020-05-12  Michael Weghorn  <m.weghorn@posteo.de>

        * server.cc (captured_main), (handle_v_run): No longer
        insert extra NULL element to args vector.
---
 gdbserver/server.cc | 2 --
 1 file changed, 2 deletions(-)

diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index 0672f9bc4d..27d0931f79 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -3015,7 +3015,6 @@ handle_v_run (char *own_buf)
       if (*next_p)
 	next_p++;
     }
-  new_argv.push_back (NULL);
 
   if (new_program_name == NULL)
     {
@@ -3815,7 +3814,6 @@ captured_main (int argc, char *argv[])
       program_path.set (make_unique_xstrdup (next_arg[0]));
       for (i = 1; i < n; i++)
 	program_args.push_back (xstrdup (next_arg[i]));
-      program_args.push_back (NULL);
 
       /* Wait till we are at first instruction in program.  */
       target_create_inferior (program_path.get (), program_args);
-- 
2.26.2

[PATCH v3 4/6] nto_process_target::create_inferior: Pass args as char **

[Michael Weghorn]

Both, [1] and [2] suggest that the fifth parameter
to the 'spawnp' function is of type 'char * const argv[]',
so just pass the args contained in the vector as
an array right away, rather than converting that
to a C string first and passing that one.

With commit 2090129c36c7e582943b7d300968d19b46160d84
("Share fork_inferior et al with gdbserver",
2016-12-22) the type had changed from 'char **'
to 'char *', but I can't see an apparent reason for
that, and 'nto_procfs_target::create_inferior'
(in gdb/nto-procfs.c) also passes a 'char **' to
'spawnp' instead.

I do not know much about that target and cannot actually
test this, however.
The main motivation to look at this was identifying
and replacing the remaining uses of the 'stringify_argv'
function which does not properly do escaping.

[1] https://support.sas.com/documentation/onlinedoc/sasc/doc750/html/lr2/zid-9360.htm
[2] https://www.ibm.com/support/knowledgecenter/ssw_ibm_i_74/apis/spawnp.htm

gdbserver/ChangeLog:

2020-05-12  Michael Weghorn  <m.weghorn@posteo.de>

        * nto-low.cc (nto_process_target::create_inferior): Pass
        argv to spawnp function as char **.
---
 gdbserver/nto-low.cc | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/gdbserver/nto-low.cc b/gdbserver/nto-low.cc
index 642fe9ffd2..a88ad02f64 100644
--- a/gdbserver/nto-low.cc
+++ b/gdbserver/nto-low.cc
@@ -357,7 +357,6 @@ nto_process_target::create_inferior (const char *program,
   struct inheritance inherit;
   pid_t pid;
   sigset_t set;
-  std::string str_program_args = stringify_argv (program_args);
 
   TRACE ("%s %s\n", __func__, program);
   /* Clear any pending SIGUSR1's but keep the behavior the same.  */
@@ -371,7 +370,7 @@ nto_process_target::create_inferior (const char *program,
   inherit.flags |= SPAWN_SETGROUP | SPAWN_HOLD;
   inherit.pgroup = SPAWN_NEWPGROUP;
   pid = spawnp (program, 0, NULL, &inherit,
-		(char *) str_program_args.c_str (), 0);
+		program_args.data (), 0);
   sigprocmask (SIG_BLOCK, &set, NULL);
 
   if (pid == -1)
-- 
2.26.2

[PATCH v3 5/6] [PR gdbserver/25893]: Use construct_inferior_arguments which handles special chars

[Michael Weghorn]

Use the construct_inferior_arguments function instead of
stringify_argv to construct a string from the program
arguments in those places where that one is then passed
to fork_inferior (linux-low, lyn-low), since
construct_inferior_arguments properly takes care of
special characters, while stringify_argv does not.
Using construct_inferior_arguments seems "natural", since its
documentation also mentions that it "does the
same shell processing as fork_inferior".

Since construct_inferior_args has been extended to do
proper quoting for Windows shells in commit
5d60742e2dd3c9b475dce54b56043a358751bbb8
("Fix quoting of special characters for the MinGW build.",
2012-06-12), use it for the Windows case as well.
(I could not test that case myself, though.)

Adapt handling of empty args in function 'handle_v_run'
in gdbserver/server.cc to just insert an empty string
for an empty arg, since that one is now properly handled
in 'construct_inferior_arguments' already (and inserting
a "''" string in 'handle_v_run' would otherwise
cause that one to be treated as a string literally
containing two quote characters, which
'construct_inferior_args' would preserve by adding
extra escaping).

This makes gdbserver properly handle program args containing special
characters (like spaces), e.g. (example from PR25893)

  $ gdbserver localhost:50505 myprogram "hello world"

now properly handles "hello world" as a single arg, not two separate
ones ("hello", "world").

2020-05-12  Michael Weghorn  <m.weghorn@posteo.de>

        PR gdbserver/25893
        * linux-low.cc (linux_process_target::create_inferior),
        lynx-low.cc (lynx_process_target::create_inferior),
        win32-low.cc (win32_process_target::create_inferior): Use
        construct_inferior_arguments instead of stringify_argv
        to get string representation which properly escapes
        special characters.
        * server.cc (handle_v_run): Just pass empty program arg
        as such, since any further processing is now handled via
        construct_inferior_arguments
---
 gdbserver/linux-low.cc | 3 ++-
 gdbserver/lynx-low.cc  | 3 ++-
 gdbserver/server.cc    | 2 +-
 gdbserver/win32-low.cc | 3 ++-
 4 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
index 3cd8d5594d..5479f02f42 100644
--- a/gdbserver/linux-low.cc
+++ b/gdbserver/linux-low.cc
@@ -984,7 +984,8 @@ linux_process_target::create_inferior (const char *program,
   {
     maybe_disable_address_space_randomization restore_personality
       (cs.disable_randomization);
-    std::string str_program_args = stringify_argv (program_args);
+    std::string str_program_args
+      = construct_inferior_arguments (program_args.size (), program_args.data ());
 
     pid = fork_inferior (program,
 			 str_program_args.c_str (),
diff --git a/gdbserver/lynx-low.cc b/gdbserver/lynx-low.cc
index 9aa140c129..361ee5b48f 100644
--- a/gdbserver/lynx-low.cc
+++ b/gdbserver/lynx-low.cc
@@ -253,7 +253,8 @@ lynx_process_target::create_inferior (const char *program,
 				      const std::vector<char *> &program_args)
 {
   int pid;
-  std::string str_program_args = stringify_argv (program_args);
+  std::string str_program_args
+    = construct_inferior_arguments (program_args.size (), program_args.data ());
 
   lynx_debug ("create_inferior ()");
 
diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index 27d0931f79..0313d18bb2 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -2957,7 +2957,7 @@ handle_v_run (char *own_buf)
       else if (p == next_p)
 	{
 	  /* Empty argument.  */
-	  new_argv.push_back (xstrdup ("''"));
+	  new_argv.push_back (xstrdup (""));
 	}
       else
 	{
diff --git a/gdbserver/win32-low.cc b/gdbserver/win32-low.cc
index 4eb63b7ca2..7a012ca81d 100644
--- a/gdbserver/win32-low.cc
+++ b/gdbserver/win32-low.cc
@@ -693,7 +693,8 @@ win32_process_target::create_inferior (const char *program,
   DWORD flags;
   PROCESS_INFORMATION pi;
   DWORD err;
-  std::string str_program_args = stringify_argv (program_args);
+  std::string str_program_args
+    = construct_inferior_arguments (program_args.size (), program_args.data ());
   char *args = (char *) str_program_args.c_str ();
 
   /* win32_wait needs to know we're not attaching.  */
-- 
2.26.2

[PATCH v3 6/6] gdbsupport: Drop now unused function 'stringify_argv'

[Michael Weghorn]

The function did not properly escape special characters
and all uses have been replaced in previous commits, so
drop the now unused function.

gdbsupport/ChangeLog:

2020-05-12  Michael Weghorn  <m.weghorn@posteo.de>

        * common-utils.cc, common-utils.h (stringify_argv): Drop
        now unused function stringify_argv
---
 gdbsupport/common-utils.cc | 23 -----------------------
 gdbsupport/common-utils.h  |  4 ----
 2 files changed, 27 deletions(-)

diff --git a/gdbsupport/common-utils.cc b/gdbsupport/common-utils.cc
index ed05d619c7..b5e4d2928e 100644
--- a/gdbsupport/common-utils.cc
+++ b/gdbsupport/common-utils.cc
@@ -375,29 +375,6 @@ free_vector_argv (std::vector<char *> &v)
 
 /* See gdbsupport/common-utils.h.  */
 
-std::string
-stringify_argv (const std::vector<char *> &args)
-{
-  std::string ret;
-
-  if (!args.empty () && args[0] != NULL)
-    {
-      for (auto s : args)
-	if (s != NULL)
-	  {
-	    ret += s;
-	    ret += ' ';
-	  }
-
-      /* Erase the last whitespace.  */
-      ret.erase (ret.end () - 1);
-    }
-
-  return ret;
-}
-
-/* See gdbsupport/common-utils.h.  */
-
 ULONGEST
 align_up (ULONGEST v, int n)
 {
diff --git a/gdbsupport/common-utils.h b/gdbsupport/common-utils.h
index ba03427c6f..30ee412365 100644
--- a/gdbsupport/common-utils.h
+++ b/gdbsupport/common-utils.h
@@ -154,10 +154,6 @@ extern const char *skip_to_space (const char *inp);
    freeing all the elements.  */
 extern void free_vector_argv (std::vector<char *> &v);
 
-/* Given a vector of arguments ARGV, return a string equivalent to
-   joining all the arguments with a whitespace separating them.  */
-extern std::string stringify_argv (const std::vector<char *> &argv);
-
 /* Return true if VALUE is in [LOW, HIGH].  */
 
 template <typename T>
-- 
2.26.2

Re: [PATCH 1/4] gdbsupport: Extend construct_inferior_arguments to allow handling all stringify_args cases

[Michael Weghorn]

On 07/05/2020 21.29, Simon Marchi wrote:
> On 2020-04-29 11:45 a.m., Christian Biesinger via Gdb-patches wrote:
>> On Wed, Apr 29, 2020 at 10:25 AM Christian Biesinger
>> <cbiesinger@google.com> wrote:
>>>
>>> Can you clarify which revision your patch is against? My files look
>>> nothing like the ones you are patching.
>>
>> Oh, I see now that this patch applies on top of the other patch series you sent.
> 
> I was also confused, normally the patch 0/N is the cover letter, not a patch
> to apply.
> 
> Simon
> 

Sorry for the confusion! It's the first time I'm contributing to a
project using this workflow and I misunderstood how to properly do it.

For the new version of the patch series, the patches now actually start
with "PATCH v3 1/6".
This also means that the patch numbers are no longer the same as in the
previous version, which was no longer the case due to added/removed patches.

I've declared all patches as "v3", so all patches in the series have the
same version number again. (previous "[PATCH v2 1/4]" had v2 already,
while the others didn't.)

I hope this is OK, but am happy to hear about a better way to do this in
the future.

Michael

Re: [PATCH 0/4] gdb: Move construct_inferior_arguments to gdbsupport

[Michael Weghorn]

On 07/05/2020 21.31, Simon Marchi wrote:
>> diff --git a/gdbsupport/common-inferior.cc b/gdbsupport/common-inferior.cc
>> index ed16e89a52..71b9a11e02 100644
>> --- a/gdbsupport/common-inferior.cc
>> +++ b/gdbsupport/common-inferior.cc
>> @@ -24,3 +24,126 @@
>>  /* See common-inferior.h.  */
>>  
>>  bool startup_with_shell = true;
>> +
>> +/* Compute command-line string given argument vector.  This does the
>> +   same shell processing as fork_inferior.  */
> 
> Let's just do a bit of spring cleaning and align this code with our current
> standards while moving it.  Move the function comment to the .h, and in the.cc
> file write:
> 
>   /* See common-inferior.h. */
> 
> Other than that, this patch LGTM.
> 
> Simon
> 

I've moved the comment in the new version,
https://sourceware.org/pipermail/gdb-patches/2020-May/168346.html

Michael

Re: [PATCH v2 1/4] gdbsupport: Extend construct_inferior_arguments to allow handling all stringify_args cases

[Michael Weghorn]

On 07/05/2020 21.49, Simon Marchi wrote:
> Hi Michael,
> 
> I think the patch looks good, there are just a few nits to fix, and a suggestion.
> 
>> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
>> index 8f7482347c..3ac64b3164 100644
>> --- a/gdb/infcmd.c
>> +++ b/gdb/infcmd.c
>> @@ -151,12 +151,9 @@ get_inferior_args (void)
>>  {
>>    if (current_inferior ()->argc != 0)
>>      {
>> -      char *n;
>> -
>> -      n = construct_inferior_arguments (current_inferior ()->argc,
>> +      std::string n = construct_inferior_arguments (current_inferior ()->argc,
>>  					current_inferior ()->argv);
> 
> Align the last line with the parenthesis, like it was before the change.

Done in the new version:
https://sourceware.org/pipermail/gdb-patches/2020-May/168347.html

> 
>> -      set_inferior_args (n);
>> -      xfree (n);
>> +      set_inferior_args (n.c_str());
> 
> Space before parenthesis.
> 

Also done.

>>      }
>>
>>    if (current_inferior ()->args == NULL)
>> diff --git a/gdbsupport/common-inferior.cc b/gdbsupport/common-inferior.cc
>> index 71b9a11e02..3f117d5ef0 100644
>> --- a/gdbsupport/common-inferior.cc
>> +++ b/gdbsupport/common-inferior.cc
>> @@ -28,15 +28,15 @@ bool startup_with_shell = true;
>>  /* Compute command-line string given argument vector.  This does the
>>     same shell processing as fork_inferior.  */
>>
>> -char *
>> -construct_inferior_arguments (int argc, char **argv)
>> +std::string
>> +construct_inferior_arguments (int argc, char * const *argv)
>>  {
>> -  char *result;
>> +  gdb_assert (argc >= 0);
>> +  if (argc == 0 || argv[0] == NULL) {
>> +      return "";
>> +  }
> 
> Omit the braces when there is just one single line statement.
> 
> When there are braces, places them like this:
> 
>   if (condition)
>     {
>       stmt1;
>       stmt2;
>     }
> 
> Did you actually encounter the case (argc > 0 && argv[0] == NULL)?  In
> other words, does it really constitute a valid input to this function?
> 

The gdbserver code used to add an extra NULL arg, so that case would
have been hit by just replacing the existing 'stringify_argv' calls by
'construct_inferior_args' (as done in the follow-up patch [1]) and
starting a program without args (like 'gdbserver localhost:50505
./main'). I had only added the patch to actually remove that extra arg
later [2]. With that patch in place that should actually not have
happened any more, so was indeed unnecessary.

In any case, I have removed that in the next version along with the
switch to using a std::string for the result right away as suggested.

[1] https://sourceware.org/pipermail/gdb-patches/2020-April/168038.html
[2] https://sourceware.org/pipermail/gdb-patches/2020-April/168040.html


>>
>> -  /* ARGC should always be at least 1, but we double check this
>> -     here.  This is also needed to silence -Werror-stringop
>> -     warnings.  */
>> -  gdb_assert (argc > 0);
>> +  char *result;
>>
>>    if (startup_with_shell)
>>      {
>> @@ -145,5 +145,8 @@ construct_inferior_arguments (int argc, char **argv)
>>  	}
>>      }
>>
>> -  return result;
>> +  std::string str_result(result);
> 
> Space before parenthesis.

Done.

> 
> If we are going to return a string anyway, I'd vote for building the result
> into an std::string directly.  You can remove the part that tries to guess
> the length of the string, and use the appropriate std::string methods /
> operators to append to it.
> 
> If you do this, I'm thinking that the `argc == 0` check at the beginning of
> the function might be unnecessary, as the code could handle it naturally:
> all the loops will be skipped and we'll return an empty string.

True, thanks for the hint. I have adapted this accordingly.

Michael

Re: [PATCH 2/4] gdbserver: Don't add extra NULL to program args

[Michael Weghorn]

On 07/05/2020 21.54, Simon Marchi wrote:
> On 2020-04-29 7:16 a.m., Michael Weghorn via Gdb-patches wrote:
>> The vector holding the program args is passed as a parameter
>> to target_create_inferior, which then passes it to
>> stringify_argv for all platforms, where any NULL entry in
>> the vector is ignored, so there seems to be no reason
>> to actually add one after all.
>>
>> (Since the intention is to replace uses of stringify_argv with
>> construct_inferior_arguments in a follow-up commit and that
>> function doesn't currently handle such NULL arguments, it
>> would otherwise have to be extended.)
> 
> Thanks, this LGTM.

I actually missed another occurence where the same was done and have
removed that as well in the next version:
https://sourceware.org/pipermail/gdb-patches/2020-May/168348.html

This was the cause of the segfault you had encountered and mentioned for
the next patch.

Michael

Re: [PATCH 3/4] [PR gdbserver/25893]: Use construct_inferior_arguments which handles special chars

[Michael Weghorn]

On 07/05/2020 22.09, Simon Marchi wrote:
> On 2020-04-29 7:16 a.m., Michael Weghorn via Gdb-patches wrote:
>> Use the construct_inferior_arguments function instead of
>> stringify_argv to construct a string from the program
>> arguments in those places where that one is then passed
>> to fork_inferior, since construct_inferior_arguments
>> properly takes care of special characters, while
>> stringify_argv does not.
>> Using construct_inferior_arguments seems "natural", since its
>> documentation also mentions that it "does the
>> same shell processing as fork_inferior".
>>
>> This makes gdbserver properly handle program args containing special
>> characters, e.g. (example from PR25893)
>>
>>   $ gdbserver localhost:50505 myprogram "hello world"
>>
>> now properly handles "hello world" as a single arg, not two separate
>> ones ("hello", "world").
>>
>> I'm not sure regarding the two remaining uses of function stringify_argv
>> (in win32_process_target::create_inferior and
>> nto_process_target::create_inferior) whose result is not passed to
>> fork_inferior and don't know how the arg processing in the creation
>> of processes on those targets exactly works, so left them unchanged
>> for now.
> 
> I'd say, do the change the best you can for win32 and nto.  We'll be
> able to test win32.  Regarding nto, it hasn't seen patches for years,
> it most likely doesn't even build.  And the toolchain not being easily
> available, we can't do much about it.  I think we should actually
> consider removing this port, unless somebody steps up to maintain it.
> Same for lynx.
> 
> This will allow removing stringify_argv, and avoid having duplicate
> broken functionality of construct_inferior_arguments.
> 

As far as I understand, win32 should already be covered, so I have
switched that to using 'construct_inferior_arguments' as well in the
next version of the patch:
https://sourceware.org/pipermail/gdb-patches/2020-May/168350.html

I couldn't actually verify this myself, though.

Regarding nto, I have submitted a separate patch, since it seemed to me
that passing a string was a bit "strange" there anyway from what what I
understood:
https://sourceware.org/pipermail/gdb-patches/2020-May/168349.html

>> 2020-04-29  Michael Weghorn  <m.weghorn@posteo.de>
>>
>>         PR gdbserver/25893
>>         * linux-low.cc (linux_process_target::create_inferior),
>>         lynx-low.cc (lynx_process_target::create_inferior): Use
>>         construct_inferior_arguments instead of stringify_argv
>>         to get string representation which properly escapes
>>         special characters.
>> ---
>>  gdbserver/linux-low.cc | 2 +-
>>  gdbserver/lynx-low.cc  | 2 +-
>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
>> index 3cd8d5594d..3f3ffc6c43 100644
>> --- a/gdbserver/linux-low.cc
>> +++ b/gdbserver/linux-low.cc
>> @@ -984,7 +984,7 @@ linux_process_target::create_inferior (const char *program,
>>    {
>>      maybe_disable_address_space_randomization restore_personality
>>        (cs.disable_randomization);
>> -    std::string str_program_args = stringify_argv (program_args);
>> +    std::string str_program_args = construct_inferior_arguments (program_args.size(), program_args.data());
> 
> This line is too long, see below.
> 

Fixed in the new version.

>>  
>>      pid = fork_inferior (program,
>>  			 str_program_args.c_str (),
>> diff --git a/gdbserver/lynx-low.cc b/gdbserver/lynx-low.cc
>> index 9aa140c129..61f692f0c0 100644
>> --- a/gdbserver/lynx-low.cc
>> +++ b/gdbserver/lynx-low.cc
>> @@ -253,7 +253,7 @@ lynx_process_target::create_inferior (const char *program,
>>  				      const std::vector<char *> &program_args)
>>  {
>>    int pid;
>> -  std::string str_program_args = stringify_argv (program_args);
>> +  std::string str_program_args = construct_inferior_arguments (program_args.size(), program_args.data());;
> 
> Extra semi-colon, and spaces before parenthesis.  The line should fit in 80 columns, here's how
> we'd typically break it:
> 
>   std::string str_program_args
>     = construct_inferior_arguments (program_args.size (), program_args.data ());
> 
> (with the equal sign on the second line, I know that may seem odd)

Done in the new version.

> 
> If you are game, you could make another preparatory patch (anywhere in the series
> before this one) that would make construct_inferior_arguments take a
> gdb::array_view<char *> parameter.  That would allow you to call it more simply
> here:
> 
>   construct_inferior_arguments (program_args);


I tried this, but as far as I understand (but I might have missed
something), this does not work here, since the gdb::array_view
constructor does not accept a const container (or reference to one) as a
parameter, so e.g.

    const std::vector<char *> vec;
    gdb::array_view<char *> view(vec);

does not work/compile, and program_args is a const reference here.
(Using a non-const vector does, but I'd personally rather not remove the
const qualifier for the 'program_args' parameter of the
'create_inferior' method.)

Michael

Re: [PATCH 3/4] [PR gdbserver/25893]: Use construct_inferior_arguments which handles special chars

[Michael Weghorn]

On 08/05/2020 00.09, Simon Marchi wrote:
> While trying out stuff to review patch 4, I noticed a crash introduced by this patch.
> 
> In one terminal, run gdbserver with:
> 
> $ ./gdbserver --once --multi :1234
> 
> In another, run GDB with:
> 
> $ ./gdb /bin/ls -ex 'set pagination off' -ex 'set remote exec-file /bin/ls' -ex 'target extended-remote :1234' -ex 'run hello'
> 
> Doing so segfaults gdbserver for me.  Could you look into it?

This is fixed in the next version of the parent commit,
https://sourceware.org/pipermail/gdb-patches/2020-May/168348.html

I had previously missed another instance where a NULL element was passed
into the argument vector.

> The commands above try to run a new process through gdbserver.  The args are passed
> to GDB using the remote protocol.  You can add -ex 'set debug remote 1' before the run
> part to see the exchanged packets, the args are hex-encoded in the vRun packet.

Thanks, those commands also helped a lot in analyzing the test failures
I experienced with my patch series for the new test you have created.

Michael

Re: [PATCH 1/4] gdbsupport: Extend construct_inferior_arguments to allow handling all stringify_args cases

[Simon Marchi]

On 2020-05-12 11:48 a.m., Michael Weghorn wrote:
> On 07/05/2020 21.29, Simon Marchi wrote:
>> On 2020-04-29 11:45 a.m., Christian Biesinger via Gdb-patches wrote:
>>> On Wed, Apr 29, 2020 at 10:25 AM Christian Biesinger
>>> <cbiesinger@google.com> wrote:
>>>>
>>>> Can you clarify which revision your patch is against? My files look
>>>> nothing like the ones you are patching.
>>>
>>> Oh, I see now that this patch applies on top of the other patch series you sent.
>>
>> I was also confused, normally the patch 0/N is the cover letter, not a patch
>> to apply.
>>
>> Simon
>>
> 
> Sorry for the confusion! It's the first time I'm contributing to a
> project using this workflow and I misunderstood how to properly do it.

It's not a big deal, it's just that I wonder how you did it.  The headers of you
messages say you've use git-send-email, and git-send-email numbers patches starting
as one, AFAIK.

> For the new version of the patch series, the patches now actually start
> with "PATCH v3 1/6".
> This also means that the patch numbers are no longer the same as in the
> previous version, which was no longer the case due to added/removed patches.
> 
> I've declared all patches as "v3", so all patches in the series have the
> same version number again. (previous "[PATCH v2 1/4]" had v2 already,
> while the others didn't.)

That's fine, good choice.

> 
> I hope this is OK, but am happy to hear about a better way to do this in
> the future.

No worries, the patch numbering was just a bit unexpected, that's all :).

Simon

Re: [PATCH 4/4] [PR gdbserver/25893]: Add gdbserver test for argument with space in it

[Michael Weghorn]

On 11/05/2020 17.04, Simon Marchi wrote:
> On 2020-05-11 10:33 a.m., Michael Weghorn wrote:
>> These don't work yet with the last patch series. I'll recheck once I
>> have finished updating the patch series and in particular fixed the
>> segfault you mentioned earlier.
>>
>> I hope to be able to send an updated patch series tomorrow.
>>
>> Do I understand correctly that I can just drop my patch adding the test,
>> since this is now already covered in a better way by the test case added
>> in your second patch?
> 
> I think so.  You can insert my two patches in your series, as the test depends
> on the fix.
> 
> If you haven't found already: after running the `make check` commands, the
> transcript is found in testsuite/gdb.log.

Thanks a lot, in particular also for your previous explanations of how
those things work together and how to run the various configurations.
Those really helped in finding out what was going wrong.

The problem was that 'handle_v_run' in 'gdbserver/server.cc' already had
some special handling for empty args, passing them as "''" into the arg
vector, and 'construct_inferior_arguments' then took this as a literal
string, so the output of the empty argv[2] was "''" instead of "".

I adapted this in the new version of that patch:
https://sourceware.org/pipermail/gdb-patches/2020-May/168350.html

All of

  make check TESTS="gdb.base/inferior-args.exp"
RUNTESTFLAGS="--target_board=native-gdbserver"

  make check TESTS="gdb.base/inferior-args.exp"
RUNTESTFLAGS="--target_board=unix"

  make check TESTS="gdb.base/inferior-args.exp"
RUNTESTFLAGS="--target_board=native-extended-gdbserver"

now pass for me on linux-amd64 (Debian testing) with the new patch set
and your two commits on top.

I did not explicitly add your commits in the patch series I sent, just
applied them locally (since I was uncertain of how to properly handle
the mismatch between you being the patch author and myself being the
submitter/email sender).

Michael

Re: [PATCH 1/4] gdbsupport: Extend construct_inferior_arguments to allow handling all stringify_args cases

[Michael Weghorn]

On 12/05/2020 18.11, Simon Marchi wrote:
> On 2020-05-12 11:48 a.m., Michael Weghorn wrote:
>> On 07/05/2020 21.29, Simon Marchi wrote:
>>> On 2020-04-29 11:45 a.m., Christian Biesinger via Gdb-patches wrote:
>>>> On Wed, Apr 29, 2020 at 10:25 AM Christian Biesinger
>>>> <cbiesinger@google.com> wrote:
>>>>>
>>>>> Can you clarify which revision your patch is against? My files look
>>>>> nothing like the ones you are patching.
>>>>
>>>> Oh, I see now that this patch applies on top of the other patch series you sent.
>>>
>>> I was also confused, normally the patch 0/N is the cover letter, not a patch
>>> to apply.
>>>
>>> Simon
>>>
>>
>> Sorry for the confusion! It's the first time I'm contributing to a
>> project using this workflow and I misunderstood how to properly do it.
> 
> It's not a big deal, it's just that I wonder how you did it.  The headers of you
> messages say you've use git-send-email, and git-send-email numbers patches starting
> as one, AFAIK.

It actually took me a while to find out as well. 'git-format-patch' (and
thus 'git-send-email') have a '--start-number' parameter, and I used
'--start-number=0', after seeing that other patch submissions started
with "[PATCH 0/N]", but missing that those were not actual patches.

Michael

Re: [PATCH 4/4] [PR gdbserver/25893]: Add gdbserver test for argument with space in it

[Simon Marchi]

On 2020-05-12 12:20 p.m., Michael Weghorn via Gdb-patches wrote:
> On 11/05/2020 17.04, Simon Marchi wrote:
>> On 2020-05-11 10:33 a.m., Michael Weghorn wrote:
>>> These don't work yet with the last patch series. I'll recheck once I
>>> have finished updating the patch series and in particular fixed the
>>> segfault you mentioned earlier.
>>>
>>> I hope to be able to send an updated patch series tomorrow.
>>>
>>> Do I understand correctly that I can just drop my patch adding the test,
>>> since this is now already covered in a better way by the test case added
>>> in your second patch?
>>
>> I think so.  You can insert my two patches in your series, as the test depends
>> on the fix.
>>
>> If you haven't found already: after running the `make check` commands, the
>> transcript is found in testsuite/gdb.log.
> 
> Thanks a lot, in particular also for your previous explanations of how
> those things work together and how to run the various configurations.
> Those really helped in finding out what was going wrong.
> 
> The problem was that 'handle_v_run' in 'gdbserver/server.cc' already had
> some special handling for empty args, passing them as "''" into the arg
> vector, and 'construct_inferior_arguments' then took this as a literal
> string, so the output of the empty argv[2] was "''" instead of "".
> 
> I adapted this in the new version of that patch:
> https://sourceware.org/pipermail/gdb-patches/2020-May/168350.html
> 
> All of
> 
>   make check TESTS="gdb.base/inferior-args.exp"
> RUNTESTFLAGS="--target_board=native-gdbserver"
> 
>   make check TESTS="gdb.base/inferior-args.exp"
> RUNTESTFLAGS="--target_board=unix"
> 
>   make check TESTS="gdb.base/inferior-args.exp"
> RUNTESTFLAGS="--target_board=native-extended-gdbserver"
> 
> now pass for me on linux-amd64 (Debian testing) with the new patch set
> and your two commits on top.
> 
> I did not explicitly add your commits in the patch series I sent, just
> applied them locally (since I was uncertain of how to properly handle
> the mismatch between you being the patch author and myself being the
> submitter/email sender).

If you try to git-send-email a commit for which you are not the "Author",
it will still send the email with you as the "From:", but there will be
an extra "From:" line in the body, such that when applied, the correct
author will be used.  In other words, it works to git-send-email patches
made by somebody else.

Thanks for sending a new version, I'll take a look.

Simon

Re: [PATCH 3/4] [PR gdbserver/25893]: Use construct_inferior_arguments which handles special chars

[Simon Marchi]

On 2020-05-12 12:07 p.m., Michael Weghorn via Gdb-patches wrote:
>> If you are game, you could make another preparatory patch (anywhere in the series
>> before this one) that would make construct_inferior_arguments take a
>> gdb::array_view<char *> parameter.  That would allow you to call it more simply
>> here:
>>
>>   construct_inferior_arguments (program_args);
> 
> 
> I tried this, but as far as I understand (but I might have missed
> something), this does not work here, since the gdb::array_view
> constructor does not accept a const container (or reference to one) as a
> parameter, so e.g.
> 
>     const std::vector<char *> vec;
>     gdb::array_view<char *> view(vec);
> 
> does not work/compile, and program_args is a const reference here.
> (Using a non-const vector does, but I'd personally rather not remove the
> const qualifier for the 'program_args' parameter of the
> 'create_inferior' method.)
> 
> Michael

Hmm yeah not sure about that.  We can deal with this after.

Simon

Re: [PATCH v3 2/6] gdbsupport: Adapt construct_inferior_arguments

[Simon Marchi]

Just some nits.

On 2020-05-12 11:42 a.m., Michael Weghorn via Gdb-patches wrote:
> diff --git a/gdbsupport/common-inferior.cc b/gdbsupport/common-inferior.cc
> index a7d631f357..cadbd50e9c 100644
> --- a/gdbsupport/common-inferior.cc
> +++ b/gdbsupport/common-inferior.cc
> @@ -27,15 +27,12 @@ bool startup_with_shell = true;
>  
>  /* See common-inferior.h.  */
>  
> -char *
> -construct_inferior_arguments (int argc, char **argv)
> +std::string
> +construct_inferior_arguments (int argc, char * const *argv)
>  {
> -  char *result;
> +  gdb_assert (argc >= 0);
>  
> -  /* ARGC should always be at least 1, but we double check this
> -     here.  This is also needed to silence -Werror-stringop
> -     warnings.  */
> -  gdb_assert (argc > 0);
> +  std::string result;
>  
>    if (startup_with_shell)
>      {
> @@ -52,48 +49,39 @@ construct_inferior_arguments (int argc, char **argv)
>        static const char quote = '\'';
>  #endif
>        int i;

Can you inline this in the for loop while at it?

> -      int length = 0;
> -      char *out, *cp;
> -
> -      /* We over-compute the size.  It shouldn't matter.  */
> -      for (i = 0; i < argc; ++i)
> -	length += 3 * strlen (argv[i]) + 1 + 2 * (argv[i][0] == '\0');
> -
> -      result = (char *) xmalloc (length);
> -      out = result;
>  
>        for (i = 0; i < argc; ++i)
>  	{
>  	  if (i > 0)
> -	    *out++ = ' ';
> +	    result += ' ';
>  
>  	  /* Need to handle empty arguments specially.  */
>  	  if (argv[i][0] == '\0')
>  	    {
> -	      *out++ = quote;
> -	      *out++ = quote;
> +	      result += quote;
> +	      result += quote;
>  	    }
>  	  else
>  	    {
>  #ifdef __MINGW32__
> -	      int quoted = 0;
> +	      bool quoted = 0;

Replace 0 with false.

>  
>  	      if (strpbrk (argv[i], special))
>  		{
> -		  quoted = 1;
> -		  *out++ = quote;
> +		  quoted = true;
> +		  result += quote;
>  		}
>  #endif
> -	      for (cp = argv[i]; *cp; ++cp)
> +	      for (char *cp = argv[i]; *cp; ++cp)
>  		{
>  		  if (*cp == '\n')
>  		    {
>  		      /* A newline cannot be quoted with a backslash (it
>  			 just disappears), only by putting it inside
>  			 quotes.  */
> -		      *out++ = quote;
> -		      *out++ = '\n';
> -		      *out++ = quote;
> +		      result += quote;
> +		      result += '\n';
> +		      result += quote;
>  		    }
>  		  else
>  		    {
> @@ -102,24 +90,22 @@ construct_inferior_arguments (int argc, char **argv)
>  #else
>  		      if (strchr (special, *cp) != NULL)
>  #endif
> -			*out++ = '\\';
> -		      *out++ = *cp;
> +			result += '\\';
> +		      result += *cp;
>  		    }
>  		}
>  #ifdef __MINGW32__
>  	      if (quoted)
> -		*out++ = quote;
> +		result += quote;
>  #endif
>  	    }
>  	}
> -      *out = '\0';
>      }
>    else
>      {
>        /* In this case we can't handle arguments that contain spaces,
>  	 tabs, or newlines -- see breakup_args().  */
>        int i;

Can you also inline this one in the for loops?

Simon

Re: [PATCH v3 4/6] nto_process_target::create_inferior: Pass args as char **

[Simon Marchi]

On 2020-05-12 11:42 a.m., Michael Weghorn via Gdb-patches wrote:
> Both, [1] and [2] suggest that the fifth parameter
> to the 'spawnp' function is of type 'char * const argv[]',
> so just pass the args contained in the vector as
> an array right away, rather than converting that
> to a C string first and passing that one.
> 
> With commit 2090129c36c7e582943b7d300968d19b46160d84
> ("Share fork_inferior et al with gdbserver",
> 2016-12-22) the type had changed from 'char **'
> to 'char *', but I can't see an apparent reason for
> that, and 'nto_procfs_target::create_inferior'
> (in gdb/nto-procfs.c) also passes a 'char **' to
> 'spawnp' instead.
> 
> I do not know much about that target and cannot actually
> test this, however.
> The main motivation to look at this was identifying
> and replacing the remaining uses of the 'stringify_argv'
> function which does not properly do escaping.
> 
> [1] https://support.sas.com/documentation/onlinedoc/sasc/doc750/html/lr2/zid-9360.htm
> [2] https://www.ibm.com/support/knowledgecenter/ssw_ibm_i_74/apis/spawnp.htm

I would suggest using this reference instead, which is from QNX Neutrino (the
platform this port is for).

http://www.qnx.com/developers/docs/7.0.0/#com.qnx.doc.neutrino.lib_ref/topic/s/spawnp.html

Otherwise, this patch is OK.

Simon

Re: [PATCH v3 5/6] [PR gdbserver/25893]: Use construct_inferior_arguments which handles special chars

[Simon Marchi]

On 2020-05-12 11:42 a.m., Michael Weghorn via Gdb-patches wrote:
> Use the construct_inferior_arguments function instead of
> stringify_argv to construct a string from the program
> arguments in those places where that one is then passed
> to fork_inferior (linux-low, lyn-low), since
> construct_inferior_arguments properly takes care of
> special characters, while stringify_argv does not.
> Using construct_inferior_arguments seems "natural", since its
> documentation also mentions that it "does the
> same shell processing as fork_inferior".
> 
> Since construct_inferior_args has been extended to do
> proper quoting for Windows shells in commit
> 5d60742e2dd3c9b475dce54b56043a358751bbb8
> ("Fix quoting of special characters for the MinGW build.",
> 2012-06-12), use it for the Windows case as well.
> (I could not test that case myself, though.)
> 
> Adapt handling of empty args in function 'handle_v_run'
> in gdbserver/server.cc to just insert an empty string
> for an empty arg, since that one is now properly handled
> in 'construct_inferior_arguments' already (and inserting
> a "''" string in 'handle_v_run' would otherwise
> cause that one to be treated as a string literally
> containing two quote characters, which
> 'construct_inferior_args' would preserve by adding
> extra escaping).
> 
> This makes gdbserver properly handle program args containing special
> characters (like spaces), e.g. (example from PR25893)
> 
>   $ gdbserver localhost:50505 myprogram "hello world"
> 
> now properly handles "hello world" as a single arg, not two separate
> ones ("hello", "world").
> 
> 2020-05-12  Michael Weghorn  <m.weghorn@posteo.de>
> 
>         PR gdbserver/25893
>         * linux-low.cc (linux_process_target::create_inferior),
>         lynx-low.cc (lynx_process_target::create_inferior),
>         win32-low.cc (win32_process_target::create_inferior): Use
>         construct_inferior_arguments instead of stringify_argv
>         to get string representation which properly escapes
>         special characters.
>         * server.cc (handle_v_run): Just pass empty program arg
>         as such, since any further processing is now handled via
>         construct_inferior_arguments

This patch LGTM.

Simon

Re: [PATCH v3 6/6] gdbsupport: Drop now unused function 'stringify_argv'

[Simon Marchi]

On 2020-05-12 11:42 a.m., Michael Weghorn via Gdb-patches wrote:
> The function did not properly escape special characters
> and all uses have been replaced in previous commits, so
> drop the now unused function.
> 
> gdbsupport/ChangeLog:
> 
> 2020-05-12  Michael Weghorn  <m.weghorn@posteo.de>
> 
>         * common-utils.cc, common-utils.h (stringify_argv): Drop
>         now unused function stringify_argv
> ---
>  gdbsupport/common-utils.cc | 23 -----------------------
>  gdbsupport/common-utils.h  |  4 ----
>  2 files changed, 27 deletions(-)
> 
> diff --git a/gdbsupport/common-utils.cc b/gdbsupport/common-utils.cc
> index ed05d619c7..b5e4d2928e 100644
> --- a/gdbsupport/common-utils.cc
> +++ b/gdbsupport/common-utils.cc
> @@ -375,29 +375,6 @@ free_vector_argv (std::vector<char *> &v)
>  
>  /* See gdbsupport/common-utils.h.  */
>  
> -std::string
> -stringify_argv (const std::vector<char *> &args)
> -{
> -  std::string ret;
> -
> -  if (!args.empty () && args[0] != NULL)
> -    {
> -      for (auto s : args)
> -	if (s != NULL)
> -	  {
> -	    ret += s;
> -	    ret += ' ';
> -	  }
> -
> -      /* Erase the last whitespace.  */
> -      ret.erase (ret.end () - 1);
> -    }
> -
> -  return ret;
> -}
> -
> -/* See gdbsupport/common-utils.h.  */
> -
>  ULONGEST
>  align_up (ULONGEST v, int n)
>  {
> diff --git a/gdbsupport/common-utils.h b/gdbsupport/common-utils.h
> index ba03427c6f..30ee412365 100644
> --- a/gdbsupport/common-utils.h
> +++ b/gdbsupport/common-utils.h
> @@ -154,10 +154,6 @@ extern const char *skip_to_space (const char *inp);
>     freeing all the elements.  */
>  extern void free_vector_argv (std::vector<char *> &v);
>  
> -/* Given a vector of arguments ARGV, return a string equivalent to
> -   joining all the arguments with a whitespace separating them.  */
> -extern std::string stringify_argv (const std::vector<char *> &argv);
> -
>  /* Return true if VALUE is in [LOW, HIGH].  */
>  
>  template <typename T>
> -- 
> 2.26.2
> 

This LGTM too.

Simon

Re: [PATCH v3 5/6] [PR gdbserver/25893]: Use construct_inferior_arguments which handles special chars

[Simon Marchi]

On 2020-05-12 8:52 p.m., Simon Marchi wrote:
> On 2020-05-12 11:42 a.m., Michael Weghorn via Gdb-patches wrote:
>> Use the construct_inferior_arguments function instead of
>> stringify_argv to construct a string from the program
>> arguments in those places where that one is then passed
>> to fork_inferior (linux-low, lyn-low), since
>> construct_inferior_arguments properly takes care of
>> special characters, while stringify_argv does not.
>> Using construct_inferior_arguments seems "natural", since its
>> documentation also mentions that it "does the
>> same shell processing as fork_inferior".
>>
>> Since construct_inferior_args has been extended to do
>> proper quoting for Windows shells in commit
>> 5d60742e2dd3c9b475dce54b56043a358751bbb8
>> ("Fix quoting of special characters for the MinGW build.",
>> 2012-06-12), use it for the Windows case as well.
>> (I could not test that case myself, though.)
>>
>> Adapt handling of empty args in function 'handle_v_run'
>> in gdbserver/server.cc to just insert an empty string
>> for an empty arg, since that one is now properly handled
>> in 'construct_inferior_arguments' already (and inserting
>> a "''" string in 'handle_v_run' would otherwise
>> cause that one to be treated as a string literally
>> containing two quote characters, which
>> 'construct_inferior_args' would preserve by adding
>> extra escaping).
>>
>> This makes gdbserver properly handle program args containing special
>> characters (like spaces), e.g. (example from PR25893)
>>
>>   $ gdbserver localhost:50505 myprogram "hello world"
>>
>> now properly handles "hello world" as a single arg, not two separate
>> ones ("hello", "world").
>>
>> 2020-05-12  Michael Weghorn  <m.weghorn@posteo.de>
>>
>>         PR gdbserver/25893
>>         * linux-low.cc (linux_process_target::create_inferior),
>>         lynx-low.cc (lynx_process_target::create_inferior),
>>         win32-low.cc (win32_process_target::create_inferior): Use
>>         construct_inferior_arguments instead of stringify_argv
>>         to get string representation which properly escapes
>>         special characters.
>>         * server.cc (handle_v_run): Just pass empty program arg
>>         as such, since any further processing is now handled via
>>         construct_inferior_arguments
> 
> This patch LGTM.
> 
> Simon
> 

I forgot to mention, I built this on mingw-w64 and confirmed that it
fixed the same problem.  It also handles empty arguments correctly, both
on gdbserver's command line and "run".

Unfortunately, I wasn't able to run my test case, but that's because of
separate issues.

Simon

Re: [PATCH 3/4] [PR gdbserver/25893]: Use construct_inferior_arguments which handles special chars

[Simon Marchi]

On 2020-05-12 1:37 p.m., Simon Marchi wrote:
> On 2020-05-12 12:07 p.m., Michael Weghorn via Gdb-patches wrote:
>>> If you are game, you could make another preparatory patch (anywhere in the series
>>> before this one) that would make construct_inferior_arguments take a
>>> gdb::array_view<char *> parameter.  That would allow you to call it more simply
>>> here:
>>>
>>>   construct_inferior_arguments (program_args);
>>
>>
>> I tried this, but as far as I understand (but I might have missed
>> something), this does not work here, since the gdb::array_view
>> constructor does not accept a const container (or reference to one) as a
>> parameter, so e.g.
>>
>>     const std::vector<char *> vec;
>>     gdb::array_view<char *> view(vec);
>>
>> does not work/compile, and program_args is a const reference here.
>> (Using a non-const vector does, but I'd personally rather not remove the
>> const qualifier for the 'program_args' parameter of the
>> 'create_inferior' method.)
>>
>> Michael
> 
> Hmm yeah not sure about that.  We can deal with this after.
> 
> Simon
> 

I gave this a try, it works to declare the parameter as

  gdb::array_view<char * const> argv

Simon

[PATCH v4 1/9] gdb: Move construct_inferior_arguments to gdbsupport

[Michael Weghorn]

This moves the function construct_inferior_arguments from
gdb/inferior.h and gdb/infcmd.c to gdbsupport/common-inferior.{h,cc}.
While at it, also move the function's comment to the header file
to align with current standards.

The intention is to use it from gdbserver in a follow-up commit.

gdb/ChangeLog:

2020-05-13  Michael Weghorn  <m.weghorn@posteo.de>

        * infcmd.c, inferior.h: (construct_inferior_arguments):
          Moved function from here to gdbsupport/common-inferior.{h,cc}

gdbsupport/ChangeLog:

w020-05-13  Michael Weghorn  <m.weghorn@posteo.de>

        * common-inferior.h, common-inferior.cc: (construct_inferior_arguments):
        Move function here from gdb/infcmd.c, gdb/inferior.h
---
 gdb/infcmd.c                  | 124 ----------------------------------
 gdb/inferior.h                |   2 -
 gdbsupport/common-inferior.cc | 122 +++++++++++++++++++++++++++++++++
 gdbsupport/common-inferior.h  |   4 ++
 4 files changed, 126 insertions(+), 126 deletions(-)

diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 9bbb413d4e..8f7482347c 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -259,130 +259,6 @@ server's cwd if remote debugging.\n"));
 			"when starting the inferior is \"%s\".\n"), cwd);
 }
 
-\f
-/* Compute command-line string given argument vector.  This does the
-   same shell processing as fork_inferior.  */
-
-char *
-construct_inferior_arguments (int argc, char **argv)
-{
-  char *result;
-
-  /* ARGC should always be at least 1, but we double check this
-     here.  This is also needed to silence -Werror-stringop
-     warnings.  */
-  gdb_assert (argc > 0);
-
-  if (startup_with_shell)
-    {
-#ifdef __MINGW32__
-      /* This holds all the characters considered special to the
-	 Windows shells.  */
-      static const char special[] = "\"!&*|[]{}<>?`~^=;, \t\n";
-      static const char quote = '"';
-#else
-      /* This holds all the characters considered special to the
-	 typical Unix shells.  We include `^' because the SunOS
-	 /bin/sh treats it as a synonym for `|'.  */
-      static const char special[] = "\"!#$&*()\\|[]{}<>?'`~^; \t\n";
-      static const char quote = '\'';
-#endif
-      int i;
-      int length = 0;
-      char *out, *cp;
-
-      /* We over-compute the size.  It shouldn't matter.  */
-      for (i = 0; i < argc; ++i)
-	length += 3 * strlen (argv[i]) + 1 + 2 * (argv[i][0] == '\0');
-
-      result = (char *) xmalloc (length);
-      out = result;
-
-      for (i = 0; i < argc; ++i)
-	{
-	  if (i > 0)
-	    *out++ = ' ';
-
-	  /* Need to handle empty arguments specially.  */
-	  if (argv[i][0] == '\0')
-	    {
-	      *out++ = quote;
-	      *out++ = quote;
-	    }
-	  else
-	    {
-#ifdef __MINGW32__
-	      int quoted = 0;
-
-	      if (strpbrk (argv[i], special))
-		{
-		  quoted = 1;
-		  *out++ = quote;
-		}
-#endif
-	      for (cp = argv[i]; *cp; ++cp)
-		{
-		  if (*cp == '\n')
-		    {
-		      /* A newline cannot be quoted with a backslash (it
-			 just disappears), only by putting it inside
-			 quotes.  */
-		      *out++ = quote;
-		      *out++ = '\n';
-		      *out++ = quote;
-		    }
-		  else
-		    {
-#ifdef __MINGW32__
-		      if (*cp == quote)
-#else
-		      if (strchr (special, *cp) != NULL)
-#endif
-			*out++ = '\\';
-		      *out++ = *cp;
-		    }
-		}
-#ifdef __MINGW32__
-	      if (quoted)
-		*out++ = quote;
-#endif
-	    }
-	}
-      *out = '\0';
-    }
-  else
-    {
-      /* In this case we can't handle arguments that contain spaces,
-	 tabs, or newlines -- see breakup_args().  */
-      int i;
-      int length = 0;
-
-      for (i = 0; i < argc; ++i)
-	{
-	  char *cp = strchr (argv[i], ' ');
-	  if (cp == NULL)
-	    cp = strchr (argv[i], '\t');
-	  if (cp == NULL)
-	    cp = strchr (argv[i], '\n');
-	  if (cp != NULL)
-	    error (_("can't handle command-line "
-		     "argument containing whitespace"));
-	  length += strlen (argv[i]) + 1;
-	}
-
-      result = (char *) xmalloc (length);
-      result[0] = '\0';
-      for (i = 0; i < argc; ++i)
-	{
-	  if (i > 0)
-	    strcat (result, " ");
-	  strcat (result, argv[i]);
-	}
-    }
-
-  return result;
-}
-\f
 
 /* This function strips the '&' character (indicating background
    execution) that is added as *the last* of the arguments ARGS of a
diff --git a/gdb/inferior.h b/gdb/inferior.h
index 1ac51369df..95af474eed 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -184,8 +184,6 @@ extern void child_interrupt (struct target_ops *self);
    STARTUP_INFERIOR.  */
 extern ptid_t gdb_startup_inferior (pid_t pid, int num_traps);
 
-extern char *construct_inferior_arguments (int, char **);
-
 /* From infcmd.c */
 
 /* Initial inferior setup.  Determines the exec file is not yet known,
diff --git a/gdbsupport/common-inferior.cc b/gdbsupport/common-inferior.cc
index ed16e89a52..a7d631f357 100644
--- a/gdbsupport/common-inferior.cc
+++ b/gdbsupport/common-inferior.cc
@@ -24,3 +24,125 @@
 /* See common-inferior.h.  */
 
 bool startup_with_shell = true;
+
+/* See common-inferior.h.  */
+
+char *
+construct_inferior_arguments (int argc, char **argv)
+{
+  char *result;
+
+  /* ARGC should always be at least 1, but we double check this
+     here.  This is also needed to silence -Werror-stringop
+     warnings.  */
+  gdb_assert (argc > 0);
+
+  if (startup_with_shell)
+    {
+#ifdef __MINGW32__
+      /* This holds all the characters considered special to the
+	 Windows shells.  */
+      static const char special[] = "\"!&*|[]{}<>?`~^=;, \t\n";
+      static const char quote = '"';
+#else
+      /* This holds all the characters considered special to the
+	 typical Unix shells.  We include `^' because the SunOS
+	 /bin/sh treats it as a synonym for `|'.  */
+      static const char special[] = "\"!#$&*()\\|[]{}<>?'`~^; \t\n";
+      static const char quote = '\'';
+#endif
+      int i;
+      int length = 0;
+      char *out, *cp;
+
+      /* We over-compute the size.  It shouldn't matter.  */
+      for (i = 0; i < argc; ++i)
+	length += 3 * strlen (argv[i]) + 1 + 2 * (argv[i][0] == '\0');
+
+      result = (char *) xmalloc (length);
+      out = result;
+
+      for (i = 0; i < argc; ++i)
+	{
+	  if (i > 0)
+	    *out++ = ' ';
+
+	  /* Need to handle empty arguments specially.  */
+	  if (argv[i][0] == '\0')
+	    {
+	      *out++ = quote;
+	      *out++ = quote;
+	    }
+	  else
+	    {
+#ifdef __MINGW32__
+	      int quoted = 0;
+
+	      if (strpbrk (argv[i], special))
+		{
+		  quoted = 1;
+		  *out++ = quote;
+		}
+#endif
+	      for (cp = argv[i]; *cp; ++cp)
+		{
+		  if (*cp == '\n')
+		    {
+		      /* A newline cannot be quoted with a backslash (it
+			 just disappears), only by putting it inside
+			 quotes.  */
+		      *out++ = quote;
+		      *out++ = '\n';
+		      *out++ = quote;
+		    }
+		  else
+		    {
+#ifdef __MINGW32__
+		      if (*cp == quote)
+#else
+		      if (strchr (special, *cp) != NULL)
+#endif
+			*out++ = '\\';
+		      *out++ = *cp;
+		    }
+		}
+#ifdef __MINGW32__
+	      if (quoted)
+		*out++ = quote;
+#endif
+	    }
+	}
+      *out = '\0';
+    }
+  else
+    {
+      /* In this case we can't handle arguments that contain spaces,
+	 tabs, or newlines -- see breakup_args().  */
+      int i;
+      int length = 0;
+
+      for (i = 0; i < argc; ++i)
+	{
+	  char *cp = strchr (argv[i], ' ');
+	  if (cp == NULL)
+	    cp = strchr (argv[i], '\t');
+	  if (cp == NULL)
+	    cp = strchr (argv[i], '\n');
+	  if (cp != NULL)
+	    error (_("can't handle command-line "
+		     "argument containing whitespace"));
+	  length += strlen (argv[i]) + 1;
+	}
+
+      result = (char *) xmalloc (length);
+      result[0] = '\0';
+      for (i = 0; i < argc; ++i)
+	{
+	  if (i > 0)
+	    strcat (result, " ");
+	  strcat (result, argv[i]);
+	}
+    }
+
+  return result;
+}
diff --git a/gdbsupport/common-inferior.h b/gdbsupport/common-inferior.h
index 3c8e87aee6..ee87bc75a3 100644
--- a/gdbsupport/common-inferior.h
+++ b/gdbsupport/common-inferior.h
@@ -58,4 +58,8 @@ extern void set_inferior_cwd (const char *cwd);
    the target is started up with a shell.  */
 extern bool startup_with_shell;
 
+/* Compute command-line string given argument vector. This does the
+   same shell processing as fork_inferior.  */
+extern char *construct_inferior_arguments (int, char **);
+
 #endif /* COMMON_COMMON_INFERIOR_H */
-- 
2.26.2

[PATCH v4 2/9] gdbsupport: Adapt construct_inferior_arguments

[Michael Weghorn]

Allow construct_inferior_arguments to handle zero args
and have it return a std::string, similar to how
stringify_argv in gdbsupport/common-utils does.

Also, add a const qualifier for the second parameter,
since it is only read, not written to.

The intention is to replace existing uses of
stringify_argv by construct_inferior_arguments
in a subsequent step, since construct_inferior_arguments
properly handles special characters, while stringify_argv
doesn't.

2020-05-13  Michael Weghorn  <m.weghorn@posteo.de>

        * common-inferior.cc, common-inferior.h (construct_inferior_arguments):
        Adapt to handle zero args and return a std::string.
        Adapt call site.
---
 gdb/infcmd.c                  |  9 ++---
 gdbsupport/common-inferior.cc | 65 ++++++++++++-----------------------
 gdbsupport/common-inferior.h  |  2 +-
 3 files changed, 26 insertions(+), 50 deletions(-)

diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 8f7482347c..a7c037837c 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -151,12 +151,9 @@ get_inferior_args (void)
 {
   if (current_inferior ()->argc != 0)
     {
-      char *n;
-
-      n = construct_inferior_arguments (current_inferior ()->argc,
-					current_inferior ()->argv);
-      set_inferior_args (n);
-      xfree (n);
+      std::string n = construct_inferior_arguments (current_inferior ()->argc,
+					            current_inferior ()->argv);
+      set_inferior_args (n.c_str ());
     }
 
   if (current_inferior ()->args == NULL)
diff --git a/gdbsupport/common-inferior.cc b/gdbsupport/common-inferior.cc
index a7d631f357..aa8be14c74 100644
--- a/gdbsupport/common-inferior.cc
+++ b/gdbsupport/common-inferior.cc
@@ -27,15 +27,12 @@ bool startup_with_shell = true;
 
 /* See common-inferior.h.  */
 
-char *
-construct_inferior_arguments (int argc, char **argv)
+std::string
+construct_inferior_arguments (int argc, char * const *argv)
 {
-  char *result;
+  gdb_assert (argc >= 0);
 
-  /* ARGC should always be at least 1, but we double check this
-     here.  This is also needed to silence -Werror-stringop
-     warnings.  */
-  gdb_assert (argc > 0);
+  std::string result;
 
   if (startup_with_shell)
     {
@@ -51,49 +48,38 @@ construct_inferior_arguments (int argc, char **argv)
       static const char special[] = "\"!#$&*()\\|[]{}<>?'`~^; \t\n";
       static const char quote = '\'';
 #endif
-      int i;
-      int length = 0;
-      char *out, *cp;
-
-      /* We over-compute the size.  It shouldn't matter.  */
-      for (i = 0; i < argc; ++i)
-	length += 3 * strlen (argv[i]) + 1 + 2 * (argv[i][0] == '\0');
-
-      result = (char *) xmalloc (length);
-      out = result;
-
-      for (i = 0; i < argc; ++i)
+      for (int i = 0; i < argc; ++i)
 	{
 	  if (i > 0)
-	    *out++ = ' ';
+	    result += ' ';
 
 	  /* Need to handle empty arguments specially.  */
 	  if (argv[i][0] == '\0')
 	    {
-	      *out++ = quote;
-	      *out++ = quote;
+	      result += quote;
+	      result += quote;
 	    }
 	  else
 	    {
 #ifdef __MINGW32__
-	      int quoted = 0;
+	      bool quoted = false;
 
 	      if (strpbrk (argv[i], special))
 		{
-		  quoted = 1;
-		  *out++ = quote;
+		  quoted = true;
+		  result += quote;
 		}
 #endif
-	      for (cp = argv[i]; *cp; ++cp)
+	      for (char *cp = argv[i]; *cp; ++cp)
 		{
 		  if (*cp == '\n')
 		    {
 		      /* A newline cannot be quoted with a backslash (it
 			 just disappears), only by putting it inside
 			 quotes.  */
-		      *out++ = quote;
-		      *out++ = '\n';
-		      *out++ = quote;
+		      result += quote;
+		      result += '\n';
+		      result += quote;
 		    }
 		  else
 		    {
@@ -102,26 +88,22 @@ construct_inferior_arguments (int argc, char **argv)
 #else
 		      if (strchr (special, *cp) != NULL)
 #endif
-			*out++ = '\\';
-		      *out++ = *cp;
+			result += '\\';
+		      result += *cp;
 		    }
 		}
 #ifdef __MINGW32__
 	      if (quoted)
-		*out++ = quote;
+		result += quote;
 #endif
 	    }
 	}
-      *out = '\0';
     }
   else
     {
       /* In this case we can't handle arguments that contain spaces,
 	 tabs, or newlines -- see breakup_args().  */
-      int i;
-      int length = 0;
-
-      for (i = 0; i < argc; ++i)
+      for (int i = 0; i < argc; ++i)
 	{
 	  char *cp = strchr (argv[i], ' ');
 	  if (cp == NULL)
@@ -131,16 +113,13 @@ construct_inferior_arguments (int argc, char **argv)
 	  if (cp != NULL)
 	    error (_("can't handle command-line "
 		     "argument containing whitespace"));
-	  length += strlen (argv[i]) + 1;
 	}
 
-      result = (char *) xmalloc (length);
-      result[0] = '\0';
-      for (i = 0; i < argc; ++i)
+      for (int i = 0; i < argc; ++i)
 	{
 	  if (i > 0)
-	    strcat (result, " ");
-	  strcat (result, argv[i]);
+	    result += " ";
+	  result += argv[i];
 	}
     }
 
diff --git a/gdbsupport/common-inferior.h b/gdbsupport/common-inferior.h
index ee87bc75a3..5e9fc8b0b9 100644
--- a/gdbsupport/common-inferior.h
+++ b/gdbsupport/common-inferior.h
@@ -60,6 +60,6 @@ extern bool startup_with_shell;
 
 /* Compute command-line string given argument vector. This does the
    same shell processing as fork_inferior.  */
-extern char *construct_inferior_arguments (int, char **);
+extern std::string construct_inferior_arguments (int, char * const *);
 
 #endif /* COMMON_COMMON_INFERIOR_H */
-- 
2.26.2

[PATCH v4 3/9] gdbsupport: Let construct_inferior_arguments take gdb::array_view param

[Michael Weghorn]

Adapt the construct_inferior_arguments function to
take a gdb::array_view<char * const> parameter instead
of a char * array and an int indicating the length
and adapt the only call site.

This will allow calling it more simply in a follow-up
patch introducing more uses of the function.

gdbsupport/ChangeLog:

2020-05-13  Michael Weghorn  <m.weghorn@posteo.de>

        * common-inferior.cc, common-inferior.h (construct_inferior_arguments):
        Adapt to take a gdb::array_view<char * const> parameter.
        Adapt call site.
---
 gdb/infcmd.c                  |  5 +++--
 gdbsupport/common-inferior.cc | 16 +++++++---------
 gdbsupport/common-inferior.h  |  5 ++++-
 3 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index a7c037837c..14066eaae1 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -151,8 +151,9 @@ get_inferior_args (void)
 {
   if (current_inferior ()->argc != 0)
     {
-      std::string n = construct_inferior_arguments (current_inferior ()->argc,
-					            current_inferior ()->argv);
+      gdb::array_view<char * const> args (current_inferior ()->argv,
+                                          current_inferior ()->argc);
+      std::string n = construct_inferior_arguments (args);
       set_inferior_args (n.c_str ());
     }
 
diff --git a/gdbsupport/common-inferior.cc b/gdbsupport/common-inferior.cc
index aa8be14c74..a67d1740a2 100644
--- a/gdbsupport/common-inferior.cc
+++ b/gdbsupport/common-inferior.cc
@@ -28,10 +28,8 @@ bool startup_with_shell = true;
 /* See common-inferior.h.  */
 
 std::string
-construct_inferior_arguments (int argc, char * const *argv)
+construct_inferior_arguments (gdb::array_view<char * const> argv)
 {
-  gdb_assert (argc >= 0);
-
   std::string result;
 
   if (startup_with_shell)
@@ -48,7 +46,7 @@ construct_inferior_arguments (int argc, char * const *argv)
       static const char special[] = "\"!#$&*()\\|[]{}<>?'`~^; \t\n";
       static const char quote = '\'';
 #endif
-      for (int i = 0; i < argc; ++i)
+      for (int i = 0; i < argv.size (); ++i)
 	{
 	  if (i > 0)
 	    result += ' ';
@@ -103,19 +101,19 @@ construct_inferior_arguments (int argc, char * const *argv)
     {
       /* In this case we can't handle arguments that contain spaces,
 	 tabs, or newlines -- see breakup_args().  */
-      for (int i = 0; i < argc; ++i)
+      for (char *arg : argv)
 	{
-	  char *cp = strchr (argv[i], ' ');
+	  char *cp = strchr (arg, ' ');
 	  if (cp == NULL)
-	    cp = strchr (argv[i], '\t');
+	    cp = strchr (arg, '\t');
 	  if (cp == NULL)
-	    cp = strchr (argv[i], '\n');
+	    cp = strchr (arg, '\n');
 	  if (cp != NULL)
 	    error (_("can't handle command-line "
 		     "argument containing whitespace"));
 	}
 
-      for (int i = 0; i < argc; ++i)
+      for (int i = 0; i < argv.size (); ++i)
 	{
 	  if (i > 0)
 	    result += " ";
diff --git a/gdbsupport/common-inferior.h b/gdbsupport/common-inferior.h
index 5e9fc8b0b9..4362934cef 100644
--- a/gdbsupport/common-inferior.h
+++ b/gdbsupport/common-inferior.h
@@ -21,6 +21,8 @@
 #ifndef COMMON_COMMON_INFERIOR_H
 #define COMMON_COMMON_INFERIOR_H
 
+#include "gdbsupport/array-view.h"
+
 /* Return the exec wrapper to be used when starting the inferior, or NULL
    otherwise.  */
 extern const char *get_exec_wrapper ();
@@ -60,6 +62,7 @@ extern bool startup_with_shell;
 
 /* Compute command-line string given argument vector. This does the
    same shell processing as fork_inferior.  */
-extern std::string construct_inferior_arguments (int, char * const *);
+extern std::string
+construct_inferior_arguments (gdb::array_view<char * const>);
 
 #endif /* COMMON_COMMON_INFERIOR_H */
-- 
2.26.2

[PATCH v4 4/9] gdbserver: Don't add extra NULL to program args

[Michael Weghorn]

The vector holding the program args is passed as a parameter
to target_create_inferior, which then passes it to
stringify_argv for all platforms, where any NULL entry in
the vector is ignored, so there seems to be no reason
to actually add one after all.

(Since the intention is to replace uses of stringify_argv with
construct_inferior_arguments in a follow-up commit and that
function doesn't currently handle such NULL arguments, it
would otherwise have to be extended.)

gdbserver/ChangeLog:

2020-05-13  Michael Weghorn  <m.weghorn@posteo.de>

        * server.cc (captured_main), (handle_v_run): No longer
        insert extra NULL element to args vector.
---
 gdbserver/server.cc | 2 --
 1 file changed, 2 deletions(-)

diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index 0672f9bc4d..27d0931f79 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -3015,7 +3015,6 @@ handle_v_run (char *own_buf)
       if (*next_p)
 	next_p++;
     }
-  new_argv.push_back (NULL);
 
   if (new_program_name == NULL)
     {
@@ -3815,7 +3814,6 @@ captured_main (int argc, char *argv[])
       program_path.set (make_unique_xstrdup (next_arg[0]));
       for (i = 1; i < n; i++)
 	program_args.push_back (xstrdup (next_arg[i]));
-      program_args.push_back (NULL);
 
       /* Wait till we are at first instruction in program.  */
       target_create_inferior (program_path.get (), program_args);
-- 
2.26.2

[PATCH v4 5/9] nto_process_target::create_inferior: Pass args as char **

[Michael Weghorn]

According to [1], the fifth parameter
to the 'spawnp' function is 'char * const argv[]',
so just pass the args contained in the vector as
an array right away, rather than converting that
to a C string first and passing that one.

With commit 2090129c36c7e582943b7d300968d19b46160d84
("Share fork_inferior et al with gdbserver",
2016-12-22) the type had changed from 'char **'
to 'char *', but I can't see an apparent reason for
that, and 'nto_procfs_target::create_inferior'
(in gdb/nto-procfs.c) also passes a 'char **' to
'spawnp' instead.

I do not know much about that target and cannot actually
test this, however.
The main motivation to look at this was identifying
and replacing the remaining uses of the 'stringify_argv'
function which does not properly do escaping.

[1] http://www.qnx.com/developers/docs/7.0.0/#com.qnx.doc.neutrino.lib_ref/topic/s/spawnp.html

gdbserver/ChangeLog:

2020-05-13  Michael Weghorn  <m.weghorn@posteo.de>

        * nto-low.cc (nto_process_target::create_inferior): Pass
        argv to spawnp function as char **.
---
 gdbserver/nto-low.cc | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/gdbserver/nto-low.cc b/gdbserver/nto-low.cc
index 642fe9ffd2..a88ad02f64 100644
--- a/gdbserver/nto-low.cc
+++ b/gdbserver/nto-low.cc
@@ -357,7 +357,6 @@ nto_process_target::create_inferior (const char *program,
   struct inheritance inherit;
   pid_t pid;
   sigset_t set;
-  std::string str_program_args = stringify_argv (program_args);
 
   TRACE ("%s %s\n", __func__, program);
   /* Clear any pending SIGUSR1's but keep the behavior the same.  */
@@ -371,7 +370,7 @@ nto_process_target::create_inferior (const char *program,
   inherit.flags |= SPAWN_SETGROUP | SPAWN_HOLD;
   inherit.pgroup = SPAWN_NEWPGROUP;
   pid = spawnp (program, 0, NULL, &inherit,
-		(char *) str_program_args.c_str (), 0);
+		program_args.data (), 0);
   sigprocmask (SIG_BLOCK, &set, NULL);
 
   if (pid == -1)
-- 
2.26.2

[PATCH v4 6/9] [PR gdbserver/25893]: Use construct_inferior_arguments which handles special chars

[Michael Weghorn]

Use the construct_inferior_arguments function instead of
stringify_argv to construct a string from the program
arguments in those places where that one is then passed
to fork_inferior (linux-low, lyn-low), since
construct_inferior_arguments properly takes care of
special characters, while stringify_argv does not.
Using construct_inferior_arguments seems "natural", since its
documentation also mentions that it "does the
same shell processing as fork_inferior".

Since construct_inferior_args has been extended to do
proper quoting for Windows shells in commit
5d60742e2dd3c9b475dce54b56043a358751bbb8
("Fix quoting of special characters for the MinGW build.",
2012-06-12), use it for the Windows case as well.
(I could not test that case myself, though.)

Adapt handling of empty args in function 'handle_v_run'
in gdbserver/server.cc to just insert an empty string
for an empty arg, since that one is now properly handled
in 'construct_inferior_arguments' already (and inserting
a "''" string in 'handle_v_run' would otherwise
cause that one to be treated as a string literally
containing two quote characters, which
'construct_inferior_args' would preserve by adding
extra escaping).

This makes gdbserver properly handle program args containing special
characters (like spaces), e.g. (example from PR25893)

  $ gdbserver localhost:50505 myprogram "hello world"

now properly handles "hello world" as a single arg, not two separate
ones ("hello", "world").

2020-05-13  Michael Weghorn  <m.weghorn@posteo.de>

        PR gdbserver/25893
        * linux-low.cc (linux_process_target::create_inferior),
        lynx-low.cc (lynx_process_target::create_inferior),
        win32-low.cc (win32_process_target::create_inferior): Use
        construct_inferior_arguments instead of stringify_argv
        to get string representation which properly escapes
        special characters.
        * server.cc (handle_v_run): Just pass empty program arg
        as such, since any further processing is now handled via
        construct_inferior_arguments.
---
 gdbserver/linux-low.cc | 2 +-
 gdbserver/lynx-low.cc  | 2 +-
 gdbserver/server.cc    | 2 +-
 gdbserver/win32-low.cc | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
index 3cd8d5594d..8ba39252c0 100644
--- a/gdbserver/linux-low.cc
+++ b/gdbserver/linux-low.cc
@@ -984,7 +984,7 @@ linux_process_target::create_inferior (const char *program,
   {
     maybe_disable_address_space_randomization restore_personality
       (cs.disable_randomization);
-    std::string str_program_args = stringify_argv (program_args);
+    std::string str_program_args = construct_inferior_arguments (program_args);
 
     pid = fork_inferior (program,
 			 str_program_args.c_str (),
diff --git a/gdbserver/lynx-low.cc b/gdbserver/lynx-low.cc
index 9aa140c129..a8e4e6079b 100644
--- a/gdbserver/lynx-low.cc
+++ b/gdbserver/lynx-low.cc
@@ -253,7 +253,7 @@ lynx_process_target::create_inferior (const char *program,
 				      const std::vector<char *> &program_args)
 {
   int pid;
-  std::string str_program_args = stringify_argv (program_args);
+  std::string str_program_args = construct_inferior_arguments (program_args);
 
   lynx_debug ("create_inferior ()");
 
diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index 27d0931f79..0313d18bb2 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -2957,7 +2957,7 @@ handle_v_run (char *own_buf)
       else if (p == next_p)
 	{
 	  /* Empty argument.  */
-	  new_argv.push_back (xstrdup ("''"));
+	  new_argv.push_back (xstrdup (""));
 	}
       else
 	{
diff --git a/gdbserver/win32-low.cc b/gdbserver/win32-low.cc
index 4eb63b7ca2..d5555a78fd 100644
--- a/gdbserver/win32-low.cc
+++ b/gdbserver/win32-low.cc
@@ -693,7 +693,7 @@ win32_process_target::create_inferior (const char *program,
   DWORD flags;
   PROCESS_INFORMATION pi;
   DWORD err;
-  std::string str_program_args = stringify_argv (program_args);
+  std::string str_program_args = construct_inferior_arguments (program_args);
   char *args = (char *) str_program_args.c_str ();
 
   /* win32_wait needs to know we're not attaching.  */
-- 
2.26.2

[PATCH v4 7/9] gdbsupport: Drop now unused function 'stringify_argv'

[Michael Weghorn]

The function did not properly escape special characters
and all uses have been replaced in previous commits, so
drop the now unused function.

gdbsupport/ChangeLog:

2020-05-13  Michael Weghorn  <m.weghorn@posteo.de>

        * common-utils.cc, common-utils.h (stringify_argv): Drop
        now unused function stringify_argv
---
 gdbsupport/common-utils.cc | 23 -----------------------
 gdbsupport/common-utils.h  |  4 ----
 2 files changed, 27 deletions(-)

diff --git a/gdbsupport/common-utils.cc b/gdbsupport/common-utils.cc
index ed05d619c7..b5e4d2928e 100644
--- a/gdbsupport/common-utils.cc
+++ b/gdbsupport/common-utils.cc
@@ -375,29 +375,6 @@ free_vector_argv (std::vector<char *> &v)
 
 /* See gdbsupport/common-utils.h.  */
 
-std::string
-stringify_argv (const std::vector<char *> &args)
-{
-  std::string ret;
-
-  if (!args.empty () && args[0] != NULL)
-    {
-      for (auto s : args)
-	if (s != NULL)
-	  {
-	    ret += s;
-	    ret += ' ';
-	  }
-
-      /* Erase the last whitespace.  */
-      ret.erase (ret.end () - 1);
-    }
-
-  return ret;
-}
-
-/* See gdbsupport/common-utils.h.  */
-
 ULONGEST
 align_up (ULONGEST v, int n)
 {
diff --git a/gdbsupport/common-utils.h b/gdbsupport/common-utils.h
index ba03427c6f..30ee412365 100644
--- a/gdbsupport/common-utils.h
+++ b/gdbsupport/common-utils.h
@@ -154,10 +154,6 @@ extern const char *skip_to_space (const char *inp);
    freeing all the elements.  */
 extern void free_vector_argv (std::vector<char *> &v);
 
-/* Given a vector of arguments ARGV, return a string equivalent to
-   joining all the arguments with a whitespace separating them.  */
-extern std::string stringify_argv (const std::vector<char *> &argv);
-
 /* Return true if VALUE is in [LOW, HIGH].  */
 
 template <typename T>
-- 
2.26.2

[PATCH v4 8/9] gdb/testsuite: support passing inferior arguments with native-gdbserver board

[Michael Weghorn]

From: Simon Marchi <simon.marchi@efficios.com>

This patch makes it possible to run tests requiring passing arguments to
the inferior with the native-gdbserver board.  The end goal is to write
a test that verifies passing arguments to the inferior works, and to
have that test exercise inferior arguments passed on the gdbserver
command line, when using the native-gdbserver target board (in addition
to the other boards).  This is done in the next patch.

With the native-gdbserver target board, gdbserver is started in
gdb_reload (implemented in config/gdbserver.exp), called in gdb_run_cmd.
gdb_run_cmd already supposedly accepts inferior arguments (although that
feature does not seem to be used anywhere), which it passes to the `run`
command, for non-stub target boards.  I've changed gdb_run_cmd so that
it forwards these arguments to gdb_reload as well.  gdb_reload passes
them to gdbserver_run, and they eventually make their way to the
gdbserver command line.

gdb_run_cmd currently accepts `args` (the varargs of tcl), which means
it receives inferior arguments as a list.  This won't work with
arguments with spaces, because they will end up being formatted with
curly braces like this:

    % set args [list hello "with spaces" world]
    hello {with spaces} world
    % puts "run $args"
    run hello {with spaces} world

I've changed it to accept a single string that is passed to `run` and
gdb_reload.  I've done the same change in gdb_start_cmd and
gdb_starti_cmd, although these two are not used with native-gdbserver.

I've changed all gdb_reload implementations in the tree to accept a new
inferior_args argument, although most of them don't do anything with it
(and don't need to).  People maintaining target boards out of tree will
need to do the same.

I found two tests to adjust to avoid adding new failures or errors.
These tests needed new [use_gdb_stub] checks, because they rely on
having GDB run new processes.  These are guarded by a [target_info
exists noargs], which made them get skipped on native-gdbserver.  But
now that the native-gdbserver board supports args, this is no longer
enough.

Note that with this change, noargs and use_gdb_stub are orthogonal.  It
took me a moment to grasp this, so I thought I would spell out the
different possible situations:

- !noargs and !use_gdb_stub: inferior process started by gdb, can pass
  args
- noargs and !use_gdb_stub: inferior process started by gdb (perhaps
  through extended-remote protocol, the simulator, some other target),
  but that target doesn't support inferior arguments
- noargs and use_gdb_stub: inferior process started by some other
  program to which GDB connects using the remote protocol, that program
  does not support passing args to the inferior process
- !noargs and use_gdb_stub: inferior process started by some other
  program to which GDB connects u sing the remote protocol, that program
  supports passing args to the inferior process

gdb/testsuite/ChangeLog:

	* lib/gdb.exp (gdb_run_cmd): Change argument from args to
	inferior_args.  Pass it to gdb_reload.
	(gdb_start_cmd, gdb_starti_cmd): Change argument from args to
	inferior_args.
	(gdb_reload): Add inferior_args argument.
	* config/gdbserver.exp (gdb_reload): Add inferior_args argument,
	pass it to gdbserver_run.
	* boards/native-gdbserver.exp: Do not set noargs.
	* boards/native-extended-gdbserver.exp (gdb_reload): Add
	inferior_args argument.
	* boards/stdio-gdbserver-base.exp (gdb_reload): Likewise.
	* gdb.base/a2-run.exp: Check for use_gdb_stub.
	* gdb.base/args.exp: Likewise.
---
 .../boards/native-extended-gdbserver.exp      |  2 +-
 gdb/testsuite/boards/native-gdbserver.exp     |  3 --
 gdb/testsuite/boards/stdio-gdbserver-base.exp |  2 +-
 gdb/testsuite/config/gdbserver.exp            | 12 +++----
 gdb/testsuite/gdb.base/a2-run.exp             | 11 +++++-
 gdb/testsuite/gdb.base/args.exp               |  6 ++++
 gdb/testsuite/lib/gdb.exp                     | 34 +++++++++++++------
 7 files changed, 46 insertions(+), 24 deletions(-)

diff --git a/gdb/testsuite/boards/native-extended-gdbserver.exp b/gdb/testsuite/boards/native-extended-gdbserver.exp
index 465c1cc8ab..3d5e782114 100644
--- a/gdb/testsuite/boards/native-extended-gdbserver.exp
+++ b/gdb/testsuite/boards/native-extended-gdbserver.exp
@@ -102,7 +102,7 @@ proc gdb_file_cmd { arg } {
     return [extended_gdbserver_load_last_file]
 }
 
-proc gdb_reload { } {
+proc gdb_reload { {inferior_args {}} } {
     return [extended_gdbserver_load_last_file]
 }
 
diff --git a/gdb/testsuite/boards/native-gdbserver.exp b/gdb/testsuite/boards/native-gdbserver.exp
index f9a507261f..823f5cfa3a 100644
--- a/gdb/testsuite/boards/native-gdbserver.exp
+++ b/gdb/testsuite/boards/native-gdbserver.exp
@@ -27,9 +27,6 @@ load_board_description "local-board"
 # This gdbserver can only run a process once per session.
 set_board_info gdb,do_reload_on_run 1
 
-# There's no support for argument-passing (yet).
-set_board_info noargs 1
-
 set_board_info use_gdb_stub 1
 set_board_info exit_is_reliable 1
 
diff --git a/gdb/testsuite/boards/stdio-gdbserver-base.exp b/gdb/testsuite/boards/stdio-gdbserver-base.exp
index aafcf6991b..720e2e823f 100644
--- a/gdb/testsuite/boards/stdio-gdbserver-base.exp
+++ b/gdb/testsuite/boards/stdio-gdbserver-base.exp
@@ -45,7 +45,7 @@ proc make_gdbserver_stdio_port {} {
     return "| [get_target_remote_pipe_cmd]"
 }
 
-proc gdb_reload { } {
+proc gdb_reload { {inferior_args {}} } {
     return [gdb_target_cmd "remote" [make_gdbserver_stdio_port]]
 }
 
diff --git a/gdb/testsuite/config/gdbserver.exp b/gdb/testsuite/config/gdbserver.exp
index cb053b63e4..a4e935f214 100644
--- a/gdb/testsuite/config/gdbserver.exp
+++ b/gdb/testsuite/config/gdbserver.exp
@@ -36,12 +36,8 @@
 #	Unles you have a gdbserver that can handle multiple sessions.
 #
 #   set_board_info noargs 1
-#	At present there is no provision in the remote protocol
-#	for passing arguments.  This test framework does not
-#	address the issue, so it's best to set this variable
-#	in your baseboard configuration file.  
-#	FIXME: there's no reason why the test harness couldn't
-#	pass commandline args when it spawns gdbserver.
+#	Set this if the board does not support passing arguments to the
+#	inferior process.
 #
 #   set_board_info gdb,noinferiorio 1
 #	Neither the traditional gdbserver nor the one in libremote
@@ -77,8 +73,8 @@ proc gdbserver_gdb_load { } {
     return [gdbserver_spawn ""]
 }
 
-proc gdb_reload { } {
-    return [gdbserver_run ""]
+proc gdb_reload { {inferior_args {}} } {
+    return [gdbserver_run $inferior_args]
 }
 
 proc gdb_reconnect { } {
diff --git a/gdb/testsuite/gdb.base/a2-run.exp b/gdb/testsuite/gdb.base/a2-run.exp
index 2e8ed60ec6..ea8f7ec95f 100644
--- a/gdb/testsuite/gdb.base/a2-run.exp
+++ b/gdb/testsuite/gdb.base/a2-run.exp
@@ -135,7 +135,7 @@ gdb_run_cmd 5
 gdb_test_stdio "" "120" "" "run \"$testfile\" with arg"
 
 # Run again with same arguments.
-gdb_run_cmd
+gdb_run_cmd 5
 
 setup_xfail "arm-*-coff"
 gdb_test_stdio "" "120" "" "run \"$testfile\" again with same args"
@@ -147,6 +147,15 @@ gdb_run_cmd
 
 gdb_test_stdio "" "usage:  factorial <number>" "" "run after setting args to nil"
 
+# The remaining tests pass inferior arguments through GDB, so doesn't
+# work with stub targets, where GDB connects to debug an already started
+# process.
+
+if [use_gdb_stub] {
+    verbose "Skipping rest of a2-run.exp because target is a stub."
+    return
+}
+
 # Use "set args" command to specify an argument and run again.
 gdb_test_no_output "set args 6"
 
diff --git a/gdb/testsuite/gdb.base/args.exp b/gdb/testsuite/gdb.base/args.exp
index 6c416fcfc8..ee75445c0b 100644
--- a/gdb/testsuite/gdb.base/args.exp
+++ b/gdb/testsuite/gdb.base/args.exp
@@ -23,6 +23,12 @@ if [target_info exists noargs] {
     return
 }
 
+# This test requires starting new inferior processes, skip it if the target
+# board is a stub.
+if [use_gdb_stub] {
+    return
+}
+
 standard_testfile
 
 if {[build_executable $testfile.exp $testfile \
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 3c6f0d76d6..ec77cfb0d3 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -245,10 +245,13 @@ proc target_can_use_run_cmd {} {
 # Using ``.*$'' could swallow up output that we attempt to match
 # elsewhere.
 #
+# INFERIOR_ARGS is passed as arguments to the start command, so may contain
+# inferior arguments.
+#
 # N.B. This function does not wait for gdb to return to the prompt,
 # that is the caller's responsibility.
 
-proc gdb_run_cmd {args} {
+proc gdb_run_cmd { {inferior_args {}} } {
     global gdb_prompt use_gdb_stub
 
     foreach command [gdb_init_commands] {
@@ -264,7 +267,7 @@ proc gdb_run_cmd {args} {
 
     if $use_gdb_stub {
 	if [target_info exists gdb,do_reload_on_run] {
-	    if { [gdb_reload] != 0 } {
+	    if { [gdb_reload $inferior_args] != 0 } {
 		return
 	    }
 	    send_gdb "continue\n"
@@ -309,7 +312,7 @@ proc gdb_run_cmd {args} {
 		    send_gdb "y\n" answer
 		}
 		-re "The program is not being run.*$gdb_prompt $" {
-		    if { [gdb_reload] != 0 } {
+		    if { [gdb_reload $inferior_args] != 0 } {
 			return
 		    }
 		    send_gdb "jump *$start\n"
@@ -324,11 +327,11 @@ proc gdb_run_cmd {args} {
     }
 
     if [target_info exists gdb,do_reload_on_run] {
-	if { [gdb_reload] != 0 } {
+	if { [gdb_reload $inferior_args] != 0 } {
 	    return
 	}
     }
-    send_gdb "run $args\n"
+    send_gdb "run $inferior_args\n"
 # This doesn't work quite right yet.
 # Use -notransfer here so that test cases (like chng-sym.exp)
 # may test for additional start-up messages.
@@ -347,10 +350,13 @@ proc gdb_run_cmd {args} {
 # Generic start command.  Return 0 if we could start the program, -1
 # if we could not.
 #
+# INFERIOR_ARGS is passed as arguments to the start command, so may contain
+# inferior arguments.
+#
 # N.B. This function does not wait for gdb to return to the prompt,
 # that is the caller's responsibility.
 
-proc gdb_start_cmd {args} {
+proc gdb_start_cmd { {inferior_args {}} } {
     global gdb_prompt use_gdb_stub
 
     foreach command [gdb_init_commands] {
@@ -368,7 +374,7 @@ proc gdb_start_cmd {args} {
 	return -1
     }
 
-    send_gdb "start $args\n"
+    send_gdb "start $inferior_args\n"
     # Use -notransfer here so that test cases (like chng-sym.exp)
     # may test for additional start-up messages.
     gdb_expect 60 {
@@ -386,10 +392,13 @@ proc gdb_start_cmd {args} {
 # Generic starti command.  Return 0 if we could start the program, -1
 # if we could not.
 #
+# INFERIOR_ARGS is passed as arguments to the starti command, so may contain
+# inferior arguments.
+#
 # N.B. This function does not wait for gdb to return to the prompt,
 # that is the caller's responsibility.
 
-proc gdb_starti_cmd {args} {
+proc gdb_starti_cmd { {inferior_args {}} } {
     global gdb_prompt use_gdb_stub
 
     foreach command [gdb_init_commands] {
@@ -407,7 +416,7 @@ proc gdb_starti_cmd {args} {
 	return -1
     }
 
-    send_gdb "starti $args\n"
+    send_gdb "starti $inferior_args\n"
     gdb_expect 60 {
 	-re "The program .* has been started already.*y or n. $" {
 	    send_gdb "y\n" answer
@@ -4817,8 +4826,13 @@ proc gdb_load { arg } {
 # either the first time or after already starting the program once,
 # for remote targets.  Most files that override gdb_load should now
 # override this instead.
+#
+# INFERIOR_ARGS contains the arguments to pass to the inferiors, as a
+# single string to get interpreted by a shell.  If the target board
+# overriding gdb_reload is a "stub", then it should arrange things such
+# these arguments make their way to the inferior process.
 
-proc gdb_reload { } {
+proc gdb_reload { {inferior_args {}} } {
     # For the benefit of existing configurations, default to gdb_load.
     # Specifying no file defaults to the executable currently being
     # debugged.
-- 
2.26.2

[PATCH v4 9/9] gdb/testsuite: add inferior arguments test

[Michael Weghorn]

From: Simon Marchi <simon.marchi@efficios.com>

Add a test for verifying different methods of passing arguments to the
inferior: the start, starti and run commands, as well as `set args`.

All these methods work naturally when using the unix or
native-extended-gdbserver target boards.  Since those are non-stub
boards, GDB runs new inferiors and therefore pass arguments to them.
With target boards where GDB connects to a stub, for example with
native-gdbserver, they don't really make sense.  The inferior process is
already started when GDB connects.

However, the "run" method is still tested with stub targets, because the
gdb_run_cmd procedure is adapted for stub targets.  Instead of issuing
the `run` command, it spawns whatever program is supposed to bring up
the stub (gdbserver, for example) using gdb_reload and makes GDB connect
to it.  So this allows us to exercise argument passing through the
gdbserver command line, when testing with the native-gdbserver board.

Note that there is already a gdb.base/args.exp, but this tests
specifically the --args switch of GDB.  Perhaps it could be integrated
in this new test, as a new "method".

gdb/testsuite/ChangeLog:

	* lib/gdb.exp (gdb_run_cmd): Return success or failure.
	* gdb.base/inferior-args.exp: New file.
	* gdb.base/inferior-args.c: New file.
---
 gdb/testsuite/gdb.base/inferior-args.c   |   8 ++
 gdb/testsuite/gdb.base/inferior-args.exp | 126 +++++++++++++++++++++++
 gdb/testsuite/lib/gdb.exp                |  21 ++--
 3 files changed, 147 insertions(+), 8 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/inferior-args.c
 create mode 100644 gdb/testsuite/gdb.base/inferior-args.exp

diff --git a/gdb/testsuite/gdb.base/inferior-args.c b/gdb/testsuite/gdb.base/inferior-args.c
new file mode 100644
index 0000000000..74ca58b508
--- /dev/null
+++ b/gdb/testsuite/gdb.base/inferior-args.c
@@ -0,0 +1,8 @@
+#include <stdio.h>
+
+int main(int argc, char **argv) {
+	for (int i = 0; i < argc; i++)
+		printf("[%d] %s\n", i, argv[i]);
+
+	return 0;
+}
diff --git a/gdb/testsuite/gdb.base/inferior-args.exp b/gdb/testsuite/gdb.base/inferior-args.exp
new file mode 100644
index 0000000000..dcda102fe5
--- /dev/null
+++ b/gdb/testsuite/gdb.base/inferior-args.exp
@@ -0,0 +1,126 @@
+# Copyright 2020 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Test running an inferior with arguments.
+
+# This does not work on boards that don't support inferior arguments.
+if [target_info exists noargs] then {
+    verbose "skipping gdb.base/inferior-args.exp because of noargs"
+    return
+}
+
+standard_testfile .c
+
+if {[build_executable "failed to prepare" $testfile $srcfile debug] == -1} {
+    return
+}
+
+clean_restart $binfile
+
+proc do_test { method } {
+    global binfile hex
+
+    # The second arg is an empty string on purpose.
+    set inferior_args { "first arg" "" "third-arg" }
+
+    clean_restart $binfile
+
+    if { $method == "start" } {
+	# The start command does not make sense for a stub.
+	if { [use_gdb_stub] } {
+	    return;
+	}
+
+	if { [gdb_start_cmd $inferior_args] < 0 } {
+	    fail "could not issue start command"
+	    return -1
+	}
+
+	# Consume up to the GDB prompt after the stop.
+	gdb_test "" ".*main.*" "stop at main"
+
+    } elseif { $method == "starti" } {
+	# The starti command does not make sense for a stub.
+	if { [use_gdb_stub] } {
+	    return;
+	}
+
+	if { [gdb_starti_cmd $inferior_args] < 0 } {
+	    fail "could not issue start command"
+	    return -1
+	}
+
+	# Consume up to the GDB prompt after the stop.
+	gdb_test "" "" "stop at first instruction"
+
+	# Put a breakpoint and continue until main.
+	if { ![gdb_breakpoint "main" message] } {
+	    fail "could not set breakpoint on main"
+	    return -1
+	}
+
+	if { [gdb_continue "main"] != 0 } {
+	    fail "could not continue to main"
+	    return -1
+	}
+
+    } elseif { $method == "run" } {
+	if { ![gdb_breakpoint "main" message] } {
+	    fail "could not set breakpoint on main"
+	    return -1
+	}
+
+	# The run command does not make sense for a stub, but GDB_RUN_CMD
+	# does the right thing when the target is a stub (start the stub,
+	# connect to it, and "continue").
+	#
+	# This allows us to test arguments passed on the gdbserver command
+	# line.
+	if { [gdb_run_cmd $inferior_args] < 0 } {
+	    fail "could not run"
+	    return -1
+	}
+
+	# Consume up to the GDB prompt after the stop.
+	gdb_test "" ".*main.*" "stop at main"
+
+    } elseif { $method == "set args" } {
+	# Using "set args" does not make sense with a stub.
+	if { [use_gdb_stub] } {
+	    return;
+	}
+
+	gdb_test_no_output "set args $inferior_args"
+
+	if { ![runto_main] } {
+	    fail "could not run to main"
+	    return -1
+	}
+
+    } else {
+	error "invalid method $method"
+    }
+
+    # Now that we are stopped at main, inspect argc/argv.
+    gdb_test "print argc" " = 4"
+    gdb_test "print argv\[0\]" " = $hex \".*\""
+    gdb_test "print argv\[1\]" " = $hex \"first arg\""
+    gdb_test "print argv\[2\]" " = $hex \"\""
+    gdb_test "print argv\[3\]" " = $hex \"third-arg\""
+}
+
+foreach_with_prefix method { "start" "starti" "run" "set args" } {
+    do_test $method
+}
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index ec77cfb0d3..a5eeba6b3c 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -241,6 +241,8 @@ proc target_can_use_run_cmd {} {
 
 # Generic run command.
 #
+# Return 0 if we could start the program, -1 if we could not.
+#
 # The second pattern below matches up to the first newline *only*.
 # Using ``.*$'' could swallow up output that we attempt to match
 # elsewhere.
@@ -268,14 +270,14 @@ proc gdb_run_cmd { {inferior_args {}} } {
     if $use_gdb_stub {
 	if [target_info exists gdb,do_reload_on_run] {
 	    if { [gdb_reload $inferior_args] != 0 } {
-		return
+		return -1
 	    }
 	    send_gdb "continue\n"
 	    gdb_expect 60 {
 		-re "Continu\[^\r\n\]*\[\r\n\]" {}
 		default {}
 	    }
-	    return
+	    return 0
 	}
 
 	if [target_info exists gdb,start_symbol] {
@@ -291,7 +293,7 @@ proc gdb_run_cmd { {inferior_args {}} } {
 	    # clever and not send a command when it has failed.
 	    if [expr $start_attempt > 3] {
 		perror "Jump to start() failed (retry count exceeded)"
-		return
+		return -1
 	    }
 	    set start_attempt [expr $start_attempt + 1]
 	    gdb_expect 30 {
@@ -300,7 +302,7 @@ proc gdb_run_cmd { {inferior_args {}} } {
 		}
 		-re "No symbol \"_start\" in current.*$gdb_prompt $" {
 		    perror "Can't find start symbol to run in gdb_run"
-		    return
+		    return -1
 		}
 		-re "No symbol \"start\" in current.*$gdb_prompt $" {
 		    send_gdb "jump *_start\n"
@@ -313,22 +315,23 @@ proc gdb_run_cmd { {inferior_args {}} } {
 		}
 		-re "The program is not being run.*$gdb_prompt $" {
 		    if { [gdb_reload $inferior_args] != 0 } {
-			return
+			return -1
 		    }
 		    send_gdb "jump *$start\n"
 		}
 		timeout {
 		    perror "Jump to start() failed (timeout)"
-		    return
+		    return -1
 		}
 	    }
 	}
-	return
+
+	return 0
     }
 
     if [target_info exists gdb,do_reload_on_run] {
 	if { [gdb_reload $inferior_args] != 0 } {
-	    return
+	    return -1
 	}
     }
     send_gdb "run $inferior_args\n"
@@ -345,6 +348,8 @@ proc gdb_run_cmd { {inferior_args {}} } {
 	    # There is no more input expected.
 	}
     }
+
+    return 0
 }
 
 # Generic start command.  Return 0 if we could start the program, -1
-- 
2.26.2

Re: [PATCH 3/4] [PR gdbserver/25893]: Use construct_inferior_arguments which handles special chars

[Michael Weghorn]

On 13/05/2020 03.59, Simon Marchi wrote:
> On 2020-05-12 1:37 p.m., Simon Marchi wrote:
>> On 2020-05-12 12:07 p.m., Michael Weghorn via Gdb-patches wrote:
>>>> If you are game, you could make another preparatory patch (anywhere in the series
>>>> before this one) that would make construct_inferior_arguments take a
>>>> gdb::array_view<char *> parameter.  That would allow you to call it more simply
>>>> here:
>>>>
>>>>   construct_inferior_arguments (program_args);
>>>
>>>
>>> I tried this, but as far as I understand (but I might have missed
>>> something), this does not work here, since the gdb::array_view
>>> constructor does not accept a const container (or reference to one) as a
>>> parameter, so e.g.
>>>
>>>     const std::vector<char *> vec;
>>>     gdb::array_view<char *> view(vec);
>>>
>>> does not work/compile, and program_args is a const reference here.
>>> (Using a non-const vector does, but I'd personally rather not remove the
>>> const qualifier for the 'program_args' parameter of the
>>> 'create_inferior' method.)
>>>
>>> Michael
>>
>> Hmm yeah not sure about that.  We can deal with this after.
>>
>> Simon
>>
> 
> I gave this a try, it works to declare the parameter as
> 
>   gdb::array_view<char * const> argv

Thanks! I've added a patch to have 'construct_inferior_arguments' take a
'gdb::array_view<char * const>' param in version 4 of the patch series:
https://sourceware.org/pipermail/gdb-patches/2020-May/168437.html

Michael

Re: [PATCH 4/4] [PR gdbserver/25893]: Add gdbserver test for argument with space in it

[Michael Weghorn]

On 12/05/2020 18.50, Simon Marchi wrote:
>>
>> I did not explicitly add your commits in the patch series I sent, just
>> applied them locally (since I was uncertain of how to properly handle
>> the mismatch between you being the patch author and myself being the
>> submitter/email sender).
> 
> If you try to git-send-email a commit for which you are not the "Author",
> it will still send the email with you as the "From:", but there will be
> an extra "From:" line in the body, such that when applied, the correct
> author will be used.  In other words, it works to git-send-email patches
> made by somebody else.
> 
> Thanks for sending a new version, I'll take a look.

Thanks for the tip. I added your patches on top of mine for the next
version (v4) of the series now.

Sorry for sending you two extra emails in my test run that was only
supposed to be sent to myself. I didn't have in mind that you would be
added in CC automatically for the patches whose author you are, unless I
tell git-send-email not to.

Michael

Re: [PATCH v3 2/6] gdbsupport: Adapt construct_inferior_arguments

[Michael Weghorn]

On 12/05/2020 19.53, Simon Marchi wrote:
> Just some nits.
> 
> On 2020-05-12 11:42 a.m., Michael Weghorn via Gdb-patches wrote:
>> diff --git a/gdbsupport/common-inferior.cc b/gdbsupport/common-inferior.cc
>> index a7d631f357..cadbd50e9c 100644
>> --- a/gdbsupport/common-inferior.cc
>> +++ b/gdbsupport/common-inferior.cc
>> @@ -27,15 +27,12 @@ bool startup_with_shell = true;
>>  
>>  /* See common-inferior.h.  */
>>  
>> -char *
>> -construct_inferior_arguments (int argc, char **argv)
>> +std::string
>> +construct_inferior_arguments (int argc, char * const *argv)
>>  {
>> -  char *result;
>> +  gdb_assert (argc >= 0);
>>  
>> -  /* ARGC should always be at least 1, but we double check this
>> -     here.  This is also needed to silence -Werror-stringop
>> -     warnings.  */
>> -  gdb_assert (argc > 0);
>> +  std::string result;
>>  
>>    if (startup_with_shell)
>>      {
>> @@ -52,48 +49,39 @@ construct_inferior_arguments (int argc, char **argv)
>>        static const char quote = '\'';
>>  #endif
>>        int i;
> 
> Can you inline this in the for loop while at it?

Yes, done in v4.

> 
>> -      int length = 0;
>> -      char *out, *cp;
>> -
>> -      /* We over-compute the size.  It shouldn't matter.  */
>> -      for (i = 0; i < argc; ++i)
>> -	length += 3 * strlen (argv[i]) + 1 + 2 * (argv[i][0] == '\0');
>> -
>> -      result = (char *) xmalloc (length);
>> -      out = result;
>>  
>>        for (i = 0; i < argc; ++i)
>>  	{
>>  	  if (i > 0)
>> -	    *out++ = ' ';
>> +	    result += ' ';
>>  
>>  	  /* Need to handle empty arguments specially.  */
>>  	  if (argv[i][0] == '\0')
>>  	    {
>> -	      *out++ = quote;
>> -	      *out++ = quote;
>> +	      result += quote;
>> +	      result += quote;
>>  	    }
>>  	  else
>>  	    {
>>  #ifdef __MINGW32__
>> -	      int quoted = 0;
>> +	      bool quoted = 0;
> 
> Replace 0 with false.

Done in v4.
> 
>>  
>>  	      if (strpbrk (argv[i], special))
>>  		{
>> -		  quoted = 1;
>> -		  *out++ = quote;
>> +		  quoted = true;
>> +		  result += quote;
>>  		}
>>  #endif
>> -	      for (cp = argv[i]; *cp; ++cp)
>> +	      for (char *cp = argv[i]; *cp; ++cp)
>>  		{
>>  		  if (*cp == '\n')
>>  		    {
>>  		      /* A newline cannot be quoted with a backslash (it
>>  			 just disappears), only by putting it inside
>>  			 quotes.  */
>> -		      *out++ = quote;
>> -		      *out++ = '\n';
>> -		      *out++ = quote;
>> +		      result += quote;
>> +		      result += '\n';
>> +		      result += quote;
>>  		    }
>>  		  else
>>  		    {
>> @@ -102,24 +90,22 @@ construct_inferior_arguments (int argc, char **argv)
>>  #else
>>  		      if (strchr (special, *cp) != NULL)
>>  #endif
>> -			*out++ = '\\';
>> -		      *out++ = *cp;
>> +			result += '\\';
>> +		      result += *cp;
>>  		    }
>>  		}
>>  #ifdef __MINGW32__
>>  	      if (quoted)
>> -		*out++ = quote;
>> +		result += quote;
>>  #endif
>>  	    }
>>  	}
>> -      *out = '\0';
>>      }
>>    else
>>      {
>>        /* In this case we can't handle arguments that contain spaces,
>>  	 tabs, or newlines -- see breakup_args().  */
>>        int i;
> 
> Can you also inline this one in the for loops?

Yes done in v4.

Michael

Re: [PATCH v3 4/6] nto_process_target::create_inferior: Pass args as char **

[Michael Weghorn]

On 12/05/2020 20.34, Simon Marchi wrote:
> On 2020-05-12 11:42 a.m., Michael Weghorn via Gdb-patches wrote:
>> Both, [1] and [2] suggest that the fifth parameter
>> to the 'spawnp' function is of type 'char * const argv[]',
>> so just pass the args contained in the vector as
>> an array right away, rather than converting that
>> to a C string first and passing that one.
>>
>> With commit 2090129c36c7e582943b7d300968d19b46160d84
>> ("Share fork_inferior et al with gdbserver",
>> 2016-12-22) the type had changed from 'char **'
>> to 'char *', but I can't see an apparent reason for
>> that, and 'nto_procfs_target::create_inferior'
>> (in gdb/nto-procfs.c) also passes a 'char **' to
>> 'spawnp' instead.
>>
>> I do not know much about that target and cannot actually
>> test this, however.
>> The main motivation to look at this was identifying
>> and replacing the remaining uses of the 'stringify_argv'
>> function which does not properly do escaping.
>>
>> [1] https://support.sas.com/documentation/onlinedoc/sasc/doc750/html/lr2/zid-9360.htm
>> [2] https://www.ibm.com/support/knowledgecenter/ssw_ibm_i_74/apis/spawnp.htm
> 
> I would suggest using this reference instead, which is from QNX Neutrino (the
> platform this port is for).
> 
> http://www.qnx.com/developers/docs/7.0.0/#com.qnx.doc.neutrino.lib_ref/topic/s/spawnp.html
> 
> Otherwise, this patch is OK.

Thanks for the hint, I have update the commit message for v4 of the patch.

Michael

Re: [PATCH v3 5/6] [PR gdbserver/25893]: Use construct_inferior_arguments which handles special chars

[Michael Weghorn]

On 13/05/2020 02.54, Simon Marchi wrote:
> On 2020-05-12 8:52 p.m., Simon Marchi wrote:
>> On 2020-05-12 11:42 a.m., Michael Weghorn via Gdb-patches wrote:
>>> Use the construct_inferior_arguments function instead of
>>> stringify_argv to construct a string from the program
>>> arguments in those places where that one is then passed
>>> to fork_inferior (linux-low, lyn-low), since
>>> construct_inferior_arguments properly takes care of
>>> special characters, while stringify_argv does not.
>>> Using construct_inferior_arguments seems "natural", since its
>>> documentation also mentions that it "does the
>>> same shell processing as fork_inferior".
>>>
>>> Since construct_inferior_args has been extended to do
>>> proper quoting for Windows shells in commit
>>> 5d60742e2dd3c9b475dce54b56043a358751bbb8
>>> ("Fix quoting of special characters for the MinGW build.",
>>> 2012-06-12), use it for the Windows case as well.
>>> (I could not test that case myself, though.)
>>>
>>> Adapt handling of empty args in function 'handle_v_run'
>>> in gdbserver/server.cc to just insert an empty string
>>> for an empty arg, since that one is now properly handled
>>> in 'construct_inferior_arguments' already (and inserting
>>> a "''" string in 'handle_v_run' would otherwise
>>> cause that one to be treated as a string literally
>>> containing two quote characters, which
>>> 'construct_inferior_args' would preserve by adding
>>> extra escaping).
>>>
>>> This makes gdbserver properly handle program args containing special
>>> characters (like spaces), e.g. (example from PR25893)
>>>
>>>   $ gdbserver localhost:50505 myprogram "hello world"
>>>
>>> now properly handles "hello world" as a single arg, not two separate
>>> ones ("hello", "world").
>>>
>>> 2020-05-12  Michael Weghorn  <m.weghorn@posteo.de>
>>>
>>>         PR gdbserver/25893
>>>         * linux-low.cc (linux_process_target::create_inferior),
>>>         lynx-low.cc (lynx_process_target::create_inferior),
>>>         win32-low.cc (win32_process_target::create_inferior): Use
>>>         construct_inferior_arguments instead of stringify_argv
>>>         to get string representation which properly escapes
>>>         special characters.
>>>         * server.cc (handle_v_run): Just pass empty program arg
>>>         as such, since any further processing is now handled via
>>>         construct_inferior_arguments
>>
>> This patch LGTM.
>>
>> Simon
>>
> 
> I forgot to mention, I built this on mingw-w64 and confirmed that it
> fixed the same problem.  It also handles empty arguments correctly, both
> on gdbserver's command line and "run".
> 
> Unfortunately, I wasn't able to run my test case, but that's because of
> separate issues.

Thanks a lot for testing it actually works and all reviewing!

Michael

Re: [PATCH v4 1/9] gdb: Move construct_inferior_arguments to gdbsupport

[Simon Marchi]

On 2020-05-13 5:47 a.m., Michael Weghorn via Gdb-patches wrote:
> This moves the function construct_inferior_arguments from
> gdb/inferior.h and gdb/infcmd.c to gdbsupport/common-inferior.{h,cc}.
> While at it, also move the function's comment to the header file
> to align with current standards.
> 
> The intention is to use it from gdbserver in a follow-up commit.

Hi Michael,

The series LGTM.  I think all we need now is to wait for your copyright assignment to
be completed?

Simon

Re: [PATCH v4 1/9] gdb: Move construct_inferior_arguments to gdbsupport

[Michael Weghorn]

On 13/05/2020 16.39, Simon Marchi wrote:
> On 2020-05-13 5:47 a.m., Michael Weghorn via Gdb-patches wrote:
>> This moves the function construct_inferior_arguments from
>> gdb/inferior.h and gdb/infcmd.c to gdbsupport/common-inferior.{h,cc}.
>> While at it, also move the function's comment to the header file
>> to align with current standards.
>>
>> The intention is to use it from gdbserver in a follow-up commit.
> 
> Hi Michael,
> 
> The series LGTM.  I think all we need now is to wait for your copyright assignment to
> be completed?
> 
> Simon
>
Hi Simon,

thank you again for all help.

I've signed the copyright assignment last week and sent it by email, but
haven't received an FSF-signed version back so far.

Michael

Re: [PATCH v4 1/9] gdb: Move construct_inferior_arguments to gdbsupport

[Simon Marchi]

On 2020-05-13 11:01 a.m., Michael Weghorn wrote:
> On 13/05/2020 16.39, Simon Marchi wrote:
>> On 2020-05-13 5:47 a.m., Michael Weghorn via Gdb-patches wrote:
>>> This moves the function construct_inferior_arguments from
>>> gdb/inferior.h and gdb/infcmd.c to gdbsupport/common-inferior.{h,cc}.
>>> While at it, also move the function's comment to the header file
>>> to align with current standards.
>>>
>>> The intention is to use it from gdbserver in a follow-up commit.
>>
>> Hi Michael,
>>
>> The series LGTM.  I think all we need now is to wait for your copyright assignment to
>> be completed?
>>
>> Simon
>>
> Hi Simon,
> 
> thank you again for all help.
> 
> I've signed the copyright assignment last week and sent it by email, but
> haven't received an FSF-signed version back so far.
> 
> Michael
> 

Ok, let me know when you hear back.

Simon

Re: [PATCH v4 8/9] gdb/testsuite: support passing inferior arguments with native-gdbserver board

[Tom de Vries]

On 13-05-2020 11:47, Michael Weghorn via Gdb-patches wrote:
> From: Simon Marchi <simon.marchi@efficios.com>
> 
> This patch makes it possible to run tests requiring passing arguments to
> the inferior with the native-gdbserver board.  The end goal is to write
> a test that verifies passing arguments to the inferior works, and to
> have that test exercise inferior arguments passed on the gdbserver
> command line, when using the native-gdbserver target board (in addition
> to the other boards).  This is done in the next patch.
> 
> With the native-gdbserver target board, gdbserver is started in
> gdb_reload (implemented in config/gdbserver.exp), called in gdb_run_cmd.
> gdb_run_cmd already supposedly accepts inferior arguments (although that
> feature does not seem to be used anywhere), which it passes to the `run`
> command, for non-stub target boards.  I've changed gdb_run_cmd so that
> it forwards these arguments to gdb_reload as well.  gdb_reload passes
> them to gdbserver_run, and they eventually make their way to the
> gdbserver command line.
> 
> gdb_run_cmd currently accepts `args` (the varargs of tcl), which means
> it receives inferior arguments as a list.  This won't work with
> arguments with spaces, because they will end up being formatted with
> curly braces like this:
> 
>     % set args [list hello "with spaces" world]
>     hello {with spaces} world
>     % puts "run $args"
>     run hello {with spaces} world
> 
> I've changed it to accept a single string that is passed to `run` and
> gdb_reload.  I've done the same change in gdb_start_cmd and
> gdb_starti_cmd, although these two are not used with native-gdbserver.
> 
> I've changed all gdb_reload implementations in the tree to accept a new
> inferior_args argument, although most of them don't do anything with it
> (and don't need to).  People maintaining target boards out of tree will
> need to do the same.
> 

The curly braces are what you get when you directly convert a list of
strings into a string.

If we do the conversion ourselves, we get the desired outcome:
...
$ cat test.tcl
#!/usr/bin/tclsh

set args [list hello "with spaces" world]

proc string_list_to_string_with_quotes { args } {
    set s ""
    foreach arg $args {
        set s "$s \"$arg\""
    }
    return $s
}

set args_str [string_list_to_string_with_quotes {*}$args]
puts "run $args_str"
$ ./test.tcl
run  "hello" "with spaces" "world"
...

And we can use the normal "args" rather than the inferior_args, as long
as we take care to pass the args from one varargs function to another
using '{*}$args'.

Thanks,
- Tom

Re: [PATCH 1/2] gdb/testsuite: support passing inferior arguments with native-gdbserver board

[Pedro Alves]

On 5/10/20 4:59 PM, Simon Marchi via Gdb-patches wrote:

> gdb/testsuite/ChangeLog:
> 
> 	* lib/gdb.exp (gdb_run_cmd): Change argument from args to
> 	inferior_args.  Pass it to gdb_reload.
> 	(gdb_start_cmd, gdb_starti_cmd): Change argument from args to
> 	inferior_args.
> 	(gdb_reload): Add inferior_args argument.
> 	* config/gdbserver.exp (gdb_reload): Add inferior_args argument,
> 	pass it to gdbserver_run.
> 	* boards/native-gdbserver.exp: Do not set noargs.
> 	* boards/native-extended-gdbserver.exp (gdb_reload): Add
> 	inferior_args argument.
> 	* boards/stdio-gdbserver-base.exp (gdb_reload): Likewise.
> 	* gdb.base/a2-run.exp: Check for use_gdb_stub.
> 	* gdb.base/args.exp: Likewise.

Woohoo, finally!  LGTM.

Thanks,
Pedro Alves

Re: [PATCH 2/2] gdb/testsuite: add inferior arguments test

[Pedro Alves]

On 5/10/20 4:59 PM, Simon Marchi via Gdb-patches wrote:

> +++ b/gdb/testsuite/gdb.base/inferior-args.c
> @@ -0,0 +1,8 @@
> +#include <stdio.h>
> +
> +int main(int argc, char **argv) {
> +	for (int i = 0; i < argc; i++)
> +		printf("[%d] %s\n", i, argv[i]);
> +
> +	return 0;
> +}

Formatting.  Also missing copyright header.

> +standard_testfile .c
> +
> +if {[build_executable "failed to prepare" $testfile $srcfile debug] == -1} {
> +    return
> +}
> +
> +clean_restart $binfile

This seems unnecessary, since do_test already calls it.

Otherwise, you can use prepare_for_testing here.

> +
> +proc do_test { method } {
> +    global binfile hex
> +
> +    # The second arg is an empty string on purpose.
> +    set inferior_args { "first arg" "" "third-arg" }
> +
> +    clean_restart $binfile
Otherwise LGTM.

Thanks,
Pedro Alves

Re: [PATCH v4 8/9] gdb/testsuite: support passing inferior arguments with native-gdbserver board

[Simon Marchi]

On 2020-05-15 1:29 p.m., Tom de Vries wrote:
> The curly braces are what you get when you directly convert a list of
> strings into a string.
> 
> If we do the conversion ourselves, we get the desired outcome:
> ...
> $ cat test.tcl
> #!/usr/bin/tclsh
> 
> set args [list hello "with spaces" world]
> 
> proc string_list_to_string_with_quotes { args } {
>     set s ""
>     foreach arg $args {
>         set s "$s \"$arg\""
>     }
>     return $s
> }
> 
> set args_str [string_list_to_string_with_quotes {*}$args]
> puts "run $args_str"
> $ ./test.tcl
> run  "hello" "with spaces" "world"
> ...
> 
> And we can use the normal "args" rather than the inferior_args, as long
> as we take care to pass the args from one varargs function to another
> using '{*}$args'.

I'm fine with both ways, do you have a preference?

Simon

Re: [PATCH 2/2] gdb/testsuite: add inferior arguments test

[Simon Marchi]

On 2020-05-15 4:07 p.m., Pedro Alves via Gdb-patches wrote:
> On 5/10/20 4:59 PM, Simon Marchi via Gdb-patches wrote:
> 
>> +++ b/gdb/testsuite/gdb.base/inferior-args.c
>> @@ -0,0 +1,8 @@
>> +#include <stdio.h>
>> +
>> +int main(int argc, char **argv) {
>> +	for (int i = 0; i < argc; i++)
>> +		printf("[%d] %s\n", i, argv[i]);
>> +
>> +	return 0;
>> +}
> 
> Formatting.  Also missing copyright header.
> 
>> +standard_testfile .c
>> +
>> +if {[build_executable "failed to prepare" $testfile $srcfile debug] == -1} {
>> +    return
>> +}
>> +
>> +clean_restart $binfile
> 
> This seems unnecessary, since do_test already calls it.
> 
> Otherwise, you can use prepare_for_testing here.
> 
>> +
>> +proc do_test { method } {
>> +    global binfile hex
>> +
>> +    # The second arg is an empty string on purpose.
>> +    set inferior_args { "first arg" "" "third-arg" }
>> +
>> +    clean_restart $binfile
> Otherwise LGTM.
> 
> Thanks,
> Pedro Alves
> 

Thanks for the comments.  Michael, would you mind addressing these changes
in my patches in your branch?

Simon

Re: [PATCH v4 8/9] gdb/testsuite: support passing inferior arguments with native-gdbserver board

[Simon Marchi]

On 2020-05-19 1:11 p.m., Simon Marchi wrote:
> On 2020-05-15 1:29 p.m., Tom de Vries wrote:
>> The curly braces are what you get when you directly convert a list of
>> strings into a string.
>>
>> If we do the conversion ourselves, we get the desired outcome:
>> ...
>> $ cat test.tcl
>> #!/usr/bin/tclsh
>>
>> set args [list hello "with spaces" world]
>>
>> proc string_list_to_string_with_quotes { args } {
>>     set s ""
>>     foreach arg $args {
>>         set s "$s \"$arg\""
>>     }
>>     return $s
>> }
>>
>> set args_str [string_list_to_string_with_quotes {*}$args]
>> puts "run $args_str"
>> $ ./test.tcl
>> run  "hello" "with spaces" "world"
>> ...
>>
>> And we can use the normal "args" rather than the inferior_args, as long
>> as we take care to pass the args from one varargs function to another
>> using '{*}$args'.
> 
> I'm fine with both ways, do you have a preference?
> 
> Simon
> 

In fact, I think I'd rather not use "args", because that would make it more
difficult to add other parameters in the future.  So I'd keep "inferior_args",
although we can make it accept a list instead of a string, and apply the
treatment you have shown above.

Or, I like the procedures that accept optional arguments using shell-like switches,
so we could do:

  set inferior_args [list a b c]
  gdb_run_cmd -inferior-args $inferior_args

This is untested, I always forget how tcl converts between strings and lists
exactly, but you should see the point.  That makes it easier to add other parameters
in the future.

Simon

Re: [PATCH v4 8/9] gdb/testsuite: support passing inferior arguments with native-gdbserver board

[Tom de Vries]

On 19-05-2020 19:22, Simon Marchi wrote:
> On 2020-05-19 1:11 p.m., Simon Marchi wrote:
>> On 2020-05-15 1:29 p.m., Tom de Vries wrote:
>>> The curly braces are what you get when you directly convert a list of
>>> strings into a string.
>>>
>>> If we do the conversion ourselves, we get the desired outcome:
>>> ...
>>> $ cat test.tcl
>>> #!/usr/bin/tclsh
>>>
>>> set args [list hello "with spaces" world]
>>>
>>> proc string_list_to_string_with_quotes { args } {
>>>     set s ""
>>>     foreach arg $args {
>>>         set s "$s \"$arg\""
>>>     }
>>>     return $s
>>> }
>>>
>>> set args_str [string_list_to_string_with_quotes {*}$args]
>>> puts "run $args_str"
>>> $ ./test.tcl
>>> run  "hello" "with spaces" "world"
>>> ...
>>>
>>> And we can use the normal "args" rather than the inferior_args, as long
>>> as we take care to pass the args from one varargs function to another
>>> using '{*}$args'.
>>
>> I'm fine with both ways, do you have a preference?
>>
>> Simon
>>
> 
> In fact, I think I'd rather not use "args", because that would make it more
> difficult to add other parameters in the future.

Ah, good point, agreed.

> So I'd keep "inferior_args",
> although we can make it accept a list instead of a string, and apply the
> treatment you have shown above.
> 
> Or, I like the procedures that accept optional arguments using shell-like switches,
> so we could do:
> 
>   set inferior_args [list a b c]
>   gdb_run_cmd -inferior-args $inferior_args
> 
> This is untested, I always forget how tcl converts between strings and lists
> exactly, but you should see the point.  That makes it easier to add other parameters
> in the future.

Yep, sounds good to me.

Thanks,
- Tom

[PATCH v5 1/9] gdb: Move construct_inferior_arguments to gdbsupport

[Michael Weghorn]

This moves the function construct_inferior_arguments from
gdb/inferior.h and gdb/infcmd.c to gdbsupport/common-inferior.{h,cc}.
While at it, also move the function's comment to the header file
to align with current standards.

The intention is to use it from gdbserver in a follow-up commit.

gdb/ChangeLog:

2020-05-13  Michael Weghorn  <m.weghorn@posteo.de>

        * infcmd.c, inferior.h: (construct_inferior_arguments):
          Moved function from here to gdbsupport/common-inferior.{h,cc}

gdbsupport/ChangeLog:

w020-05-13  Michael Weghorn  <m.weghorn@posteo.de>

        * common-inferior.h, common-inferior.cc: (construct_inferior_arguments):
        Move function here from gdb/infcmd.c, gdb/inferior.h
---
 gdb/infcmd.c                  | 124 ----------------------------------
 gdb/inferior.h                |   2 -
 gdbsupport/common-inferior.cc | 122 +++++++++++++++++++++++++++++++++
 gdbsupport/common-inferior.h  |   4 ++
 4 files changed, 126 insertions(+), 126 deletions(-)

diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 32905a7b59..cf6e540e79 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -259,130 +259,6 @@ server's cwd if remote debugging.\n"));
 			"when starting the inferior is \"%s\".\n"), cwd);
 }
 
-\f
-/* Compute command-line string given argument vector.  This does the
-   same shell processing as fork_inferior.  */
-
-char *
-construct_inferior_arguments (int argc, char **argv)
-{
-  char *result;
-
-  /* ARGC should always be at least 1, but we double check this
-     here.  This is also needed to silence -Werror-stringop
-     warnings.  */
-  gdb_assert (argc > 0);
-
-  if (startup_with_shell)
-    {
-#ifdef __MINGW32__
-      /* This holds all the characters considered special to the
-	 Windows shells.  */
-      static const char special[] = "\"!&*|[]{}<>?`~^=;, \t\n";
-      static const char quote = '"';
-#else
-      /* This holds all the characters considered special to the
-	 typical Unix shells.  We include `^' because the SunOS
-	 /bin/sh treats it as a synonym for `|'.  */
-      static const char special[] = "\"!#$&*()\\|[]{}<>?'`~^; \t\n";
-      static const char quote = '\'';
-#endif
-      int i;
-      int length = 0;
-      char *out, *cp;
-
-      /* We over-compute the size.  It shouldn't matter.  */
-      for (i = 0; i < argc; ++i)
-	length += 3 * strlen (argv[i]) + 1 + 2 * (argv[i][0] == '\0');
-
-      result = (char *) xmalloc (length);
-      out = result;
-
-      for (i = 0; i < argc; ++i)
-	{
-	  if (i > 0)
-	    *out++ = ' ';
-
-	  /* Need to handle empty arguments specially.  */
-	  if (argv[i][0] == '\0')
-	    {
-	      *out++ = quote;
-	      *out++ = quote;
-	    }
-	  else
-	    {
-#ifdef __MINGW32__
-	      int quoted = 0;
-
-	      if (strpbrk (argv[i], special))
-		{
-		  quoted = 1;
-		  *out++ = quote;
-		}
-#endif
-	      for (cp = argv[i]; *cp; ++cp)
-		{
-		  if (*cp == '\n')
-		    {
-		      /* A newline cannot be quoted with a backslash (it
-			 just disappears), only by putting it inside
-			 quotes.  */
-		      *out++ = quote;
-		      *out++ = '\n';
-		      *out++ = quote;
-		    }
-		  else
-		    {
-#ifdef __MINGW32__
-		      if (*cp == quote)
-#else
-		      if (strchr (special, *cp) != NULL)
-#endif
-			*out++ = '\\';
-		      *out++ = *cp;
-		    }
-		}
-#ifdef __MINGW32__
-	      if (quoted)
-		*out++ = quote;
-#endif
-	    }
-	}
-      *out = '\0';
-    }
-  else
-    {
-      /* In this case we can't handle arguments that contain spaces,
-	 tabs, or newlines -- see breakup_args().  */
-      int i;
-      int length = 0;
-
-      for (i = 0; i < argc; ++i)
-	{
-	  char *cp = strchr (argv[i], ' ');
-	  if (cp == NULL)
-	    cp = strchr (argv[i], '\t');
-	  if (cp == NULL)
-	    cp = strchr (argv[i], '\n');
-	  if (cp != NULL)
-	    error (_("can't handle command-line "
-		     "argument containing whitespace"));
-	  length += strlen (argv[i]) + 1;
-	}
-
-      result = (char *) xmalloc (length);
-      result[0] = '\0';
-      for (i = 0; i < argc; ++i)
-	{
-	  if (i > 0)
-	    strcat (result, " ");
-	  strcat (result, argv[i]);
-	}
-    }
-
-  return result;
-}
-\f
 
 /* This function strips the '&' character (indicating background
    execution) that is added as *the last* of the arguments ARGS of a
diff --git a/gdb/inferior.h b/gdb/inferior.h
index 1ac51369df..95af474eed 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -184,8 +184,6 @@ extern void child_interrupt (struct target_ops *self);
    STARTUP_INFERIOR.  */
 extern ptid_t gdb_startup_inferior (pid_t pid, int num_traps);
 
-extern char *construct_inferior_arguments (int, char **);
-
 /* From infcmd.c */
 
 /* Initial inferior setup.  Determines the exec file is not yet known,
diff --git a/gdbsupport/common-inferior.cc b/gdbsupport/common-inferior.cc
index ed16e89a52..a7d631f357 100644
--- a/gdbsupport/common-inferior.cc
+++ b/gdbsupport/common-inferior.cc
@@ -24,3 +24,125 @@
 /* See common-inferior.h.  */
 
 bool startup_with_shell = true;
+
+/* See common-inferior.h.  */
+
+char *
+construct_inferior_arguments (int argc, char **argv)
+{
+  char *result;
+
+  /* ARGC should always be at least 1, but we double check this
+     here.  This is also needed to silence -Werror-stringop
+     warnings.  */
+  gdb_assert (argc > 0);
+
+  if (startup_with_shell)
+    {
+#ifdef __MINGW32__
+      /* This holds all the characters considered special to the
+	 Windows shells.  */
+      static const char special[] = "\"!&*|[]{}<>?`~^=;, \t\n";
+      static const char quote = '"';
+#else
+      /* This holds all the characters considered special to the
+	 typical Unix shells.  We include `^' because the SunOS
+	 /bin/sh treats it as a synonym for `|'.  */
+      static const char special[] = "\"!#$&*()\\|[]{}<>?'`~^; \t\n";
+      static const char quote = '\'';
+#endif
+      int i;
+      int length = 0;
+      char *out, *cp;
+
+      /* We over-compute the size.  It shouldn't matter.  */
+      for (i = 0; i < argc; ++i)
+	length += 3 * strlen (argv[i]) + 1 + 2 * (argv[i][0] == '\0');
+
+      result = (char *) xmalloc (length);
+      out = result;
+
+      for (i = 0; i < argc; ++i)
+	{
+	  if (i > 0)
+	    *out++ = ' ';
+
+	  /* Need to handle empty arguments specially.  */
+	  if (argv[i][0] == '\0')
+	    {
+	      *out++ = quote;
+	      *out++ = quote;
+	    }
+	  else
+	    {
+#ifdef __MINGW32__
+	      int quoted = 0;
+
+	      if (strpbrk (argv[i], special))
+		{
+		  quoted = 1;
+		  *out++ = quote;
+		}
+#endif
+	      for (cp = argv[i]; *cp; ++cp)
+		{
+		  if (*cp == '\n')
+		    {
+		      /* A newline cannot be quoted with a backslash (it
+			 just disappears), only by putting it inside
+			 quotes.  */
+		      *out++ = quote;
+		      *out++ = '\n';
+		      *out++ = quote;
+		    }
+		  else
+		    {
+#ifdef __MINGW32__
+		      if (*cp == quote)
+#else
+		      if (strchr (special, *cp) != NULL)
+#endif
+			*out++ = '\\';
+		      *out++ = *cp;
+		    }
+		}
+#ifdef __MINGW32__
+	      if (quoted)
+		*out++ = quote;
+#endif
+	    }
+	}
+      *out = '\0';
+    }
+  else
+    {
+      /* In this case we can't handle arguments that contain spaces,
+	 tabs, or newlines -- see breakup_args().  */
+      int i;
+      int length = 0;
+
+      for (i = 0; i < argc; ++i)
+	{
+	  char *cp = strchr (argv[i], ' ');
+	  if (cp == NULL)
+	    cp = strchr (argv[i], '\t');
+	  if (cp == NULL)
+	    cp = strchr (argv[i], '\n');
+	  if (cp != NULL)
+	    error (_("can't handle command-line "
+		     "argument containing whitespace"));
+	  length += strlen (argv[i]) + 1;
+	}
+
+      result = (char *) xmalloc (length);
+      result[0] = '\0';
+      for (i = 0; i < argc; ++i)
+	{
+	  if (i > 0)
+	    strcat (result, " ");
+	  strcat (result, argv[i]);
+	}
+    }
+
+  return result;
+}
diff --git a/gdbsupport/common-inferior.h b/gdbsupport/common-inferior.h
index 3c8e87aee6..ee87bc75a3 100644
--- a/gdbsupport/common-inferior.h
+++ b/gdbsupport/common-inferior.h
@@ -58,4 +58,8 @@ extern void set_inferior_cwd (const char *cwd);
    the target is started up with a shell.  */
 extern bool startup_with_shell;
 
+/* Compute command-line string given argument vector. This does the
+   same shell processing as fork_inferior.  */
+extern char *construct_inferior_arguments (int, char **);
+
 #endif /* COMMON_COMMON_INFERIOR_H */
-- 
2.26.2

[PATCH v5 2/9] gdbsupport: Adapt construct_inferior_arguments

[Michael Weghorn]

Allow construct_inferior_arguments to handle zero args
and have it return a std::string, similar to how
stringify_argv in gdbsupport/common-utils does.

Also, add a const qualifier for the second parameter,
since it is only read, not written to.

The intention is to replace existing uses of
stringify_argv by construct_inferior_arguments
in a subsequent step, since construct_inferior_arguments
properly handles special characters, while stringify_argv
doesn't.

2020-05-13  Michael Weghorn  <m.weghorn@posteo.de>

        * common-inferior.cc, common-inferior.h (construct_inferior_arguments):
        Adapt to handle zero args and return a std::string.
        Adapt call site.
---
 gdb/infcmd.c                  |  9 ++---
 gdbsupport/common-inferior.cc | 65 ++++++++++++-----------------------
 gdbsupport/common-inferior.h  |  2 +-
 3 files changed, 26 insertions(+), 50 deletions(-)

diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index cf6e540e79..ffcc364f64 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -151,12 +151,9 @@ get_inferior_args (void)
 {
   if (current_inferior ()->argc != 0)
     {
-      char *n;
-
-      n = construct_inferior_arguments (current_inferior ()->argc,
-					current_inferior ()->argv);
-      set_inferior_args (n);
-      xfree (n);
+      std::string n = construct_inferior_arguments (current_inferior ()->argc,
+					            current_inferior ()->argv);
+      set_inferior_args (n.c_str ());
     }
 
   if (current_inferior ()->args == NULL)
diff --git a/gdbsupport/common-inferior.cc b/gdbsupport/common-inferior.cc
index a7d631f357..aa8be14c74 100644
--- a/gdbsupport/common-inferior.cc
+++ b/gdbsupport/common-inferior.cc
@@ -27,15 +27,12 @@ bool startup_with_shell = true;
 
 /* See common-inferior.h.  */
 
-char *
-construct_inferior_arguments (int argc, char **argv)
+std::string
+construct_inferior_arguments (int argc, char * const *argv)
 {
-  char *result;
+  gdb_assert (argc >= 0);
 
-  /* ARGC should always be at least 1, but we double check this
-     here.  This is also needed to silence -Werror-stringop
-     warnings.  */
-  gdb_assert (argc > 0);
+  std::string result;
 
   if (startup_with_shell)
     {
@@ -51,49 +48,38 @@ construct_inferior_arguments (int argc, char **argv)
       static const char special[] = "\"!#$&*()\\|[]{}<>?'`~^; \t\n";
       static const char quote = '\'';
 #endif
-      int i;
-      int length = 0;
-      char *out, *cp;
-
-      /* We over-compute the size.  It shouldn't matter.  */
-      for (i = 0; i < argc; ++i)
-	length += 3 * strlen (argv[i]) + 1 + 2 * (argv[i][0] == '\0');
-
-      result = (char *) xmalloc (length);
-      out = result;
-
-      for (i = 0; i < argc; ++i)
+      for (int i = 0; i < argc; ++i)
 	{
 	  if (i > 0)
-	    *out++ = ' ';
+	    result += ' ';
 
 	  /* Need to handle empty arguments specially.  */
 	  if (argv[i][0] == '\0')
 	    {
-	      *out++ = quote;
-	      *out++ = quote;
+	      result += quote;
+	      result += quote;
 	    }
 	  else
 	    {
 #ifdef __MINGW32__
-	      int quoted = 0;
+	      bool quoted = false;
 
 	      if (strpbrk (argv[i], special))
 		{
-		  quoted = 1;
-		  *out++ = quote;
+		  quoted = true;
+		  result += quote;
 		}
 #endif
-	      for (cp = argv[i]; *cp; ++cp)
+	      for (char *cp = argv[i]; *cp; ++cp)
 		{
 		  if (*cp == '\n')
 		    {
 		      /* A newline cannot be quoted with a backslash (it
 			 just disappears), only by putting it inside
 			 quotes.  */
-		      *out++ = quote;
-		      *out++ = '\n';
-		      *out++ = quote;
+		      result += quote;
+		      result += '\n';
+		      result += quote;
 		    }
 		  else
 		    {
@@ -102,26 +88,22 @@ construct_inferior_arguments (int argc, char **argv)
 #else
 		      if (strchr (special, *cp) != NULL)
 #endif
-			*out++ = '\\';
-		      *out++ = *cp;
+			result += '\\';
+		      result += *cp;
 		    }
 		}
 #ifdef __MINGW32__
 	      if (quoted)
-		*out++ = quote;
+		result += quote;
 #endif
 	    }
 	}
-      *out = '\0';
     }
   else
     {
       /* In this case we can't handle arguments that contain spaces,
 	 tabs, or newlines -- see breakup_args().  */
-      int i;
-      int length = 0;
-
-      for (i = 0; i < argc; ++i)
+      for (int i = 0; i < argc; ++i)
 	{
 	  char *cp = strchr (argv[i], ' ');
 	  if (cp == NULL)
@@ -131,16 +113,13 @@ construct_inferior_arguments (int argc, char **argv)
 	  if (cp != NULL)
 	    error (_("can't handle command-line "
 		     "argument containing whitespace"));
-	  length += strlen (argv[i]) + 1;
 	}
 
-      result = (char *) xmalloc (length);
-      result[0] = '\0';
-      for (i = 0; i < argc; ++i)
+      for (int i = 0; i < argc; ++i)
 	{
 	  if (i > 0)
-	    strcat (result, " ");
-	  strcat (result, argv[i]);
+	    result += " ";
+	  result += argv[i];
 	}
     }
 
diff --git a/gdbsupport/common-inferior.h b/gdbsupport/common-inferior.h
index ee87bc75a3..5e9fc8b0b9 100644
--- a/gdbsupport/common-inferior.h
+++ b/gdbsupport/common-inferior.h
@@ -60,6 +60,6 @@ extern bool startup_with_shell;
 
 /* Compute command-line string given argument vector. This does the
    same shell processing as fork_inferior.  */
-extern char *construct_inferior_arguments (int, char **);
+extern std::string construct_inferior_arguments (int, char * const *);
 
 #endif /* COMMON_COMMON_INFERIOR_H */
-- 
2.26.2

[PATCH v5 3/9] gdbsupport: Let construct_inferior_arguments take gdb::array_view param

[Michael Weghorn]

Adapt the construct_inferior_arguments function to
take a gdb::array_view<char * const> parameter instead
of a char * array and an int indicating the length
and adapt the only call site.

This will allow calling it more simply in a follow-up
patch introducing more uses of the function.

gdbsupport/ChangeLog:

2020-05-13  Michael Weghorn  <m.weghorn@posteo.de>

        * common-inferior.cc, common-inferior.h (construct_inferior_arguments):
        Adapt to take a gdb::array_view<char * const> parameter.
        Adapt call site.
---
 gdb/infcmd.c                  |  5 +++--
 gdbsupport/common-inferior.cc | 16 +++++++---------
 gdbsupport/common-inferior.h  |  5 ++++-
 3 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index ffcc364f64..891da91c80 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -151,8 +151,9 @@ get_inferior_args (void)
 {
   if (current_inferior ()->argc != 0)
     {
-      std::string n = construct_inferior_arguments (current_inferior ()->argc,
-					            current_inferior ()->argv);
+      gdb::array_view<char * const> args (current_inferior ()->argv,
+                                          current_inferior ()->argc);
+      std::string n = construct_inferior_arguments (args);
       set_inferior_args (n.c_str ());
     }
 
diff --git a/gdbsupport/common-inferior.cc b/gdbsupport/common-inferior.cc
index aa8be14c74..a67d1740a2 100644
--- a/gdbsupport/common-inferior.cc
+++ b/gdbsupport/common-inferior.cc
@@ -28,10 +28,8 @@ bool startup_with_shell = true;
 /* See common-inferior.h.  */
 
 std::string
-construct_inferior_arguments (int argc, char * const *argv)
+construct_inferior_arguments (gdb::array_view<char * const> argv)
 {
-  gdb_assert (argc >= 0);
-
   std::string result;
 
   if (startup_with_shell)
@@ -48,7 +46,7 @@ construct_inferior_arguments (int argc, char * const *argv)
       static const char special[] = "\"!#$&*()\\|[]{}<>?'`~^; \t\n";
       static const char quote = '\'';
 #endif
-      for (int i = 0; i < argc; ++i)
+      for (int i = 0; i < argv.size (); ++i)
 	{
 	  if (i > 0)
 	    result += ' ';
@@ -103,19 +101,19 @@ construct_inferior_arguments (int argc, char * const *argv)
     {
       /* In this case we can't handle arguments that contain spaces,
 	 tabs, or newlines -- see breakup_args().  */
-      for (int i = 0; i < argc; ++i)
+      for (char *arg : argv)
 	{
-	  char *cp = strchr (argv[i], ' ');
+	  char *cp = strchr (arg, ' ');
 	  if (cp == NULL)
-	    cp = strchr (argv[i], '\t');
+	    cp = strchr (arg, '\t');
 	  if (cp == NULL)
-	    cp = strchr (argv[i], '\n');
+	    cp = strchr (arg, '\n');
 	  if (cp != NULL)
 	    error (_("can't handle command-line "
 		     "argument containing whitespace"));
 	}
 
-      for (int i = 0; i < argc; ++i)
+      for (int i = 0; i < argv.size (); ++i)
 	{
 	  if (i > 0)
 	    result += " ";
diff --git a/gdbsupport/common-inferior.h b/gdbsupport/common-inferior.h
index 5e9fc8b0b9..4362934cef 100644
--- a/gdbsupport/common-inferior.h
+++ b/gdbsupport/common-inferior.h
@@ -21,6 +21,8 @@
 #ifndef COMMON_COMMON_INFERIOR_H
 #define COMMON_COMMON_INFERIOR_H
 
+#include "gdbsupport/array-view.h"
+
 /* Return the exec wrapper to be used when starting the inferior, or NULL
    otherwise.  */
 extern const char *get_exec_wrapper ();
@@ -60,6 +62,7 @@ extern bool startup_with_shell;
 
 /* Compute command-line string given argument vector. This does the
    same shell processing as fork_inferior.  */
-extern std::string construct_inferior_arguments (int, char * const *);
+extern std::string
+construct_inferior_arguments (gdb::array_view<char * const>);
 
 #endif /* COMMON_COMMON_INFERIOR_H */
-- 
2.26.2

[PATCH v5 4/9] gdbserver: Don't add extra NULL to program args

[Michael Weghorn]

The vector holding the program args is passed as a parameter
to target_create_inferior, which then passes it to
stringify_argv for all platforms, where any NULL entry in
the vector is ignored, so there seems to be no reason
to actually add one after all.

(Since the intention is to replace uses of stringify_argv with
construct_inferior_arguments in a follow-up commit and that
function doesn't currently handle such NULL arguments, it
would otherwise have to be extended.)

gdbserver/ChangeLog:

2020-05-13  Michael Weghorn  <m.weghorn@posteo.de>

        * server.cc (captured_main), (handle_v_run): No longer
        insert extra NULL element to args vector.
---
 gdbserver/server.cc | 2 --
 1 file changed, 2 deletions(-)

diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index 0672f9bc4d..27d0931f79 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -3015,7 +3015,6 @@ handle_v_run (char *own_buf)
       if (*next_p)
 	next_p++;
     }
-  new_argv.push_back (NULL);
 
   if (new_program_name == NULL)
     {
@@ -3815,7 +3814,6 @@ captured_main (int argc, char *argv[])
       program_path.set (make_unique_xstrdup (next_arg[0]));
       for (i = 1; i < n; i++)
 	program_args.push_back (xstrdup (next_arg[i]));
-      program_args.push_back (NULL);
 
       /* Wait till we are at first instruction in program.  */
       target_create_inferior (program_path.get (), program_args);
-- 
2.26.2

[PATCH v5 5/9] nto_process_target::create_inferior: Pass args as char **

[Michael Weghorn]

According to [1], the fifth parameter
to the 'spawnp' function is 'char * const argv[]',
so just pass the args contained in the vector as
an array right away, rather than converting that
to a C string first and passing that one.

With commit 2090129c36c7e582943b7d300968d19b46160d84
("Share fork_inferior et al with gdbserver",
2016-12-22) the type had changed from 'char **'
to 'char *', but I can't see an apparent reason for
that, and 'nto_procfs_target::create_inferior'
(in gdb/nto-procfs.c) also passes a 'char **' to
'spawnp' instead.

I do not know much about that target and cannot actually
test this, however.
The main motivation to look at this was identifying
and replacing the remaining uses of the 'stringify_argv'
function which does not properly do escaping.

[1] http://www.qnx.com/developers/docs/7.0.0/#com.qnx.doc.neutrino.lib_ref/topic/s/spawnp.html

gdbserver/ChangeLog:

2020-05-13  Michael Weghorn  <m.weghorn@posteo.de>

        * nto-low.cc (nto_process_target::create_inferior): Pass
        argv to spawnp function as char **.
---
 gdbserver/nto-low.cc | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/gdbserver/nto-low.cc b/gdbserver/nto-low.cc
index 642fe9ffd2..a88ad02f64 100644
--- a/gdbserver/nto-low.cc
+++ b/gdbserver/nto-low.cc
@@ -357,7 +357,6 @@ nto_process_target::create_inferior (const char *program,
   struct inheritance inherit;
   pid_t pid;
   sigset_t set;
-  std::string str_program_args = stringify_argv (program_args);
 
   TRACE ("%s %s\n", __func__, program);
   /* Clear any pending SIGUSR1's but keep the behavior the same.  */
@@ -371,7 +370,7 @@ nto_process_target::create_inferior (const char *program,
   inherit.flags |= SPAWN_SETGROUP | SPAWN_HOLD;
   inherit.pgroup = SPAWN_NEWPGROUP;
   pid = spawnp (program, 0, NULL, &inherit,
-		(char *) str_program_args.c_str (), 0);
+		program_args.data (), 0);
   sigprocmask (SIG_BLOCK, &set, NULL);
 
   if (pid == -1)
-- 
2.26.2

[PATCH v5 6/9] [PR gdbserver/25893]: Use construct_inferior_arguments which handles special chars

[Michael Weghorn]

Use the construct_inferior_arguments function instead of
stringify_argv to construct a string from the program
arguments in those places where that one is then passed
to fork_inferior (linux-low, lyn-low), since
construct_inferior_arguments properly takes care of
special characters, while stringify_argv does not.
Using construct_inferior_arguments seems "natural", since its
documentation also mentions that it "does the
same shell processing as fork_inferior".

Since construct_inferior_args has been extended to do
proper quoting for Windows shells in commit
5d60742e2dd3c9b475dce54b56043a358751bbb8
("Fix quoting of special characters for the MinGW build.",
2012-06-12), use it for the Windows case as well.
(I could not test that case myself, though.)

Adapt handling of empty args in function 'handle_v_run'
in gdbserver/server.cc to just insert an empty string
for an empty arg, since that one is now properly handled
in 'construct_inferior_arguments' already (and inserting
a "''" string in 'handle_v_run' would otherwise
cause that one to be treated as a string literally
containing two quote characters, which
'construct_inferior_args' would preserve by adding
extra escaping).

This makes gdbserver properly handle program args containing special
characters (like spaces), e.g. (example from PR25893)

  $ gdbserver localhost:50505 myprogram "hello world"

now properly handles "hello world" as a single arg, not two separate
ones ("hello", "world").

2020-05-13  Michael Weghorn  <m.weghorn@posteo.de>

        PR gdbserver/25893
        * linux-low.cc (linux_process_target::create_inferior),
        lynx-low.cc (lynx_process_target::create_inferior),
        win32-low.cc (win32_process_target::create_inferior): Use
        construct_inferior_arguments instead of stringify_argv
        to get string representation which properly escapes
        special characters.
        * server.cc (handle_v_run): Just pass empty program arg
        as such, since any further processing is now handled via
        construct_inferior_arguments.
---
 gdbserver/linux-low.cc | 2 +-
 gdbserver/lynx-low.cc  | 2 +-
 gdbserver/server.cc    | 2 +-
 gdbserver/win32-low.cc | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
index 3cd8d5594d..8ba39252c0 100644
--- a/gdbserver/linux-low.cc
+++ b/gdbserver/linux-low.cc
@@ -984,7 +984,7 @@ linux_process_target::create_inferior (const char *program,
   {
     maybe_disable_address_space_randomization restore_personality
       (cs.disable_randomization);
-    std::string str_program_args = stringify_argv (program_args);
+    std::string str_program_args = construct_inferior_arguments (program_args);
 
     pid = fork_inferior (program,
 			 str_program_args.c_str (),
diff --git a/gdbserver/lynx-low.cc b/gdbserver/lynx-low.cc
index 9aa140c129..a8e4e6079b 100644
--- a/gdbserver/lynx-low.cc
+++ b/gdbserver/lynx-low.cc
@@ -253,7 +253,7 @@ lynx_process_target::create_inferior (const char *program,
 				      const std::vector<char *> &program_args)
 {
   int pid;
-  std::string str_program_args = stringify_argv (program_args);
+  std::string str_program_args = construct_inferior_arguments (program_args);
 
   lynx_debug ("create_inferior ()");
 
diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index 27d0931f79..0313d18bb2 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -2957,7 +2957,7 @@ handle_v_run (char *own_buf)
       else if (p == next_p)
 	{
 	  /* Empty argument.  */
-	  new_argv.push_back (xstrdup ("''"));
+	  new_argv.push_back (xstrdup (""));
 	}
       else
 	{
diff --git a/gdbserver/win32-low.cc b/gdbserver/win32-low.cc
index 4eb63b7ca2..d5555a78fd 100644
--- a/gdbserver/win32-low.cc
+++ b/gdbserver/win32-low.cc
@@ -693,7 +693,7 @@ win32_process_target::create_inferior (const char *program,
   DWORD flags;
   PROCESS_INFORMATION pi;
   DWORD err;
-  std::string str_program_args = stringify_argv (program_args);
+  std::string str_program_args = construct_inferior_arguments (program_args);
   char *args = (char *) str_program_args.c_str ();
 
   /* win32_wait needs to know we're not attaching.  */
-- 
2.26.2

[PATCH v5 7/9] gdbsupport: Drop now unused function 'stringify_argv'

[Michael Weghorn]

The function did not properly escape special characters
and all uses have been replaced in previous commits, so
drop the now unused function.

gdbsupport/ChangeLog:

2020-05-13  Michael Weghorn  <m.weghorn@posteo.de>

        * common-utils.cc, common-utils.h (stringify_argv): Drop
        now unused function stringify_argv
---
 gdbsupport/common-utils.cc | 23 -----------------------
 gdbsupport/common-utils.h  |  4 ----
 2 files changed, 27 deletions(-)

diff --git a/gdbsupport/common-utils.cc b/gdbsupport/common-utils.cc
index ed05d619c7..b5e4d2928e 100644
--- a/gdbsupport/common-utils.cc
+++ b/gdbsupport/common-utils.cc
@@ -375,29 +375,6 @@ free_vector_argv (std::vector<char *> &v)
 
 /* See gdbsupport/common-utils.h.  */
 
-std::string
-stringify_argv (const std::vector<char *> &args)
-{
-  std::string ret;
-
-  if (!args.empty () && args[0] != NULL)
-    {
-      for (auto s : args)
-	if (s != NULL)
-	  {
-	    ret += s;
-	    ret += ' ';
-	  }
-
-      /* Erase the last whitespace.  */
-      ret.erase (ret.end () - 1);
-    }
-
-  return ret;
-}
-
-/* See gdbsupport/common-utils.h.  */
-
 ULONGEST
 align_up (ULONGEST v, int n)
 {
diff --git a/gdbsupport/common-utils.h b/gdbsupport/common-utils.h
index ba03427c6f..30ee412365 100644
--- a/gdbsupport/common-utils.h
+++ b/gdbsupport/common-utils.h
@@ -154,10 +154,6 @@ extern const char *skip_to_space (const char *inp);
    freeing all the elements.  */
 extern void free_vector_argv (std::vector<char *> &v);
 
-/* Given a vector of arguments ARGV, return a string equivalent to
-   joining all the arguments with a whitespace separating them.  */
-extern std::string stringify_argv (const std::vector<char *> &argv);
-
 /* Return true if VALUE is in [LOW, HIGH].  */
 
 template <typename T>
-- 
2.26.2

[PATCH v5 8/9] gdb/testsuite: support passing inferior arguments with native-gdbserver board

[Michael Weghorn]

From: Simon Marchi <simon.marchi@efficios.com>

This patch makes it possible to run tests requiring passing arguments to
the inferior with the native-gdbserver board.  The end goal is to write
a test that verifies passing arguments to the inferior works, and to
have that test exercise inferior arguments passed on the gdbserver
command line, when using the native-gdbserver target board (in addition
to the other boards).  This is done in the next patch.

With the native-gdbserver target board, gdbserver is started in
gdb_reload (implemented in config/gdbserver.exp), called in gdb_run_cmd.
gdb_run_cmd already supposedly accepts inferior arguments (although that
feature does not seem to be used anywhere), which it passes to the `run`
command, for non-stub target boards.  I've changed gdb_run_cmd so that
it forwards these arguments to gdb_reload as well.  gdb_reload passes
them to gdbserver_run, and they eventually make their way to the
gdbserver command line.

gdb_run_cmd currently accepts `args` (the varargs of tcl), which means
it receives inferior arguments as a list.  This won't work with
arguments with spaces, because they will end up being formatted with
curly braces like this:

    % set args [list hello "with spaces" world]
    hello {with spaces} world
    % puts "run $args"
    run hello {with spaces} world

I've changed it to accept a single string that is passed to `run` and
gdb_reload.  I've done the same change in gdb_start_cmd and
gdb_starti_cmd, although these two are not used with native-gdbserver.

I've changed all gdb_reload implementations in the tree to accept a new
inferior_args argument, although most of them don't do anything with it
(and don't need to).  People maintaining target boards out of tree will
need to do the same.

I found two tests to adjust to avoid adding new failures or errors.
These tests needed new [use_gdb_stub] checks, because they rely on
having GDB run new processes.  These are guarded by a [target_info
exists noargs], which made them get skipped on native-gdbserver.  But
now that the native-gdbserver board supports args, this is no longer
enough.

Note that with this change, noargs and use_gdb_stub are orthogonal.  It
took me a moment to grasp this, so I thought I would spell out the
different possible situations:

- !noargs and !use_gdb_stub: inferior process started by gdb, can pass
  args
- noargs and !use_gdb_stub: inferior process started by gdb (perhaps
  through extended-remote protocol, the simulator, some other target),
  but that target doesn't support inferior arguments
- noargs and use_gdb_stub: inferior process started by some other
  program to which GDB connects using the remote protocol, that program
  does not support passing args to the inferior process
- !noargs and use_gdb_stub: inferior process started by some other
  program to which GDB connects u sing the remote protocol, that program
  supports passing args to the inferior process

gdb/testsuite/ChangeLog:

	* lib/gdb.exp (gdb_run_cmd): Change argument from args to
	inferior_args.  Pass it to gdb_reload.
	(gdb_start_cmd, gdb_starti_cmd): Change argument from args to
	inferior_args.
	(gdb_reload): Add inferior_args argument.
	* config/gdbserver.exp (gdb_reload): Add inferior_args argument,
	pass it to gdbserver_run.
	* boards/native-gdbserver.exp: Do not set noargs.
	* boards/native-extended-gdbserver.exp (gdb_reload): Add
	inferior_args argument.
	* boards/stdio-gdbserver-base.exp (gdb_reload): Likewise.
	* gdb.base/a2-run.exp: Check for use_gdb_stub.
	* gdb.base/args.exp: Likewise.
---
 .../boards/native-extended-gdbserver.exp      |  2 +-
 gdb/testsuite/boards/native-gdbserver.exp     |  3 --
 gdb/testsuite/boards/stdio-gdbserver-base.exp |  2 +-
 gdb/testsuite/config/gdbserver.exp            | 12 +++----
 gdb/testsuite/gdb.base/a2-run.exp             | 11 +++++-
 gdb/testsuite/gdb.base/args.exp               |  6 ++++
 gdb/testsuite/lib/gdb.exp                     | 34 +++++++++++++------
 7 files changed, 46 insertions(+), 24 deletions(-)

diff --git a/gdb/testsuite/boards/native-extended-gdbserver.exp b/gdb/testsuite/boards/native-extended-gdbserver.exp
index 465c1cc8ab..3d5e782114 100644
--- a/gdb/testsuite/boards/native-extended-gdbserver.exp
+++ b/gdb/testsuite/boards/native-extended-gdbserver.exp
@@ -102,7 +102,7 @@ proc gdb_file_cmd { arg } {
     return [extended_gdbserver_load_last_file]
 }
 
-proc gdb_reload { } {
+proc gdb_reload { {inferior_args {}} } {
     return [extended_gdbserver_load_last_file]
 }
 
diff --git a/gdb/testsuite/boards/native-gdbserver.exp b/gdb/testsuite/boards/native-gdbserver.exp
index f9a507261f..823f5cfa3a 100644
--- a/gdb/testsuite/boards/native-gdbserver.exp
+++ b/gdb/testsuite/boards/native-gdbserver.exp
@@ -27,9 +27,6 @@ load_board_description "local-board"
 # This gdbserver can only run a process once per session.
 set_board_info gdb,do_reload_on_run 1
 
-# There's no support for argument-passing (yet).
-set_board_info noargs 1
-
 set_board_info use_gdb_stub 1
 set_board_info exit_is_reliable 1
 
diff --git a/gdb/testsuite/boards/stdio-gdbserver-base.exp b/gdb/testsuite/boards/stdio-gdbserver-base.exp
index aafcf6991b..720e2e823f 100644
--- a/gdb/testsuite/boards/stdio-gdbserver-base.exp
+++ b/gdb/testsuite/boards/stdio-gdbserver-base.exp
@@ -45,7 +45,7 @@ proc make_gdbserver_stdio_port {} {
     return "| [get_target_remote_pipe_cmd]"
 }
 
-proc gdb_reload { } {
+proc gdb_reload { {inferior_args {}} } {
     return [gdb_target_cmd "remote" [make_gdbserver_stdio_port]]
 }
 
diff --git a/gdb/testsuite/config/gdbserver.exp b/gdb/testsuite/config/gdbserver.exp
index cb053b63e4..a4e935f214 100644
--- a/gdb/testsuite/config/gdbserver.exp
+++ b/gdb/testsuite/config/gdbserver.exp
@@ -36,12 +36,8 @@
 #	Unles you have a gdbserver that can handle multiple sessions.
 #
 #   set_board_info noargs 1
-#	At present there is no provision in the remote protocol
-#	for passing arguments.  This test framework does not
-#	address the issue, so it's best to set this variable
-#	in your baseboard configuration file.  
-#	FIXME: there's no reason why the test harness couldn't
-#	pass commandline args when it spawns gdbserver.
+#	Set this if the board does not support passing arguments to the
+#	inferior process.
 #
 #   set_board_info gdb,noinferiorio 1
 #	Neither the traditional gdbserver nor the one in libremote
@@ -77,8 +73,8 @@ proc gdbserver_gdb_load { } {
     return [gdbserver_spawn ""]
 }
 
-proc gdb_reload { } {
-    return [gdbserver_run ""]
+proc gdb_reload { {inferior_args {}} } {
+    return [gdbserver_run $inferior_args]
 }
 
 proc gdb_reconnect { } {
diff --git a/gdb/testsuite/gdb.base/a2-run.exp b/gdb/testsuite/gdb.base/a2-run.exp
index 2e8ed60ec6..ea8f7ec95f 100644
--- a/gdb/testsuite/gdb.base/a2-run.exp
+++ b/gdb/testsuite/gdb.base/a2-run.exp
@@ -135,7 +135,7 @@ gdb_run_cmd 5
 gdb_test_stdio "" "120" "" "run \"$testfile\" with arg"
 
 # Run again with same arguments.
-gdb_run_cmd
+gdb_run_cmd 5
 
 setup_xfail "arm-*-coff"
 gdb_test_stdio "" "120" "" "run \"$testfile\" again with same args"
@@ -147,6 +147,15 @@ gdb_run_cmd
 
 gdb_test_stdio "" "usage:  factorial <number>" "" "run after setting args to nil"
 
+# The remaining tests pass inferior arguments through GDB, so doesn't
+# work with stub targets, where GDB connects to debug an already started
+# process.
+
+if [use_gdb_stub] {
+    verbose "Skipping rest of a2-run.exp because target is a stub."
+    return
+}
+
 # Use "set args" command to specify an argument and run again.
 gdb_test_no_output "set args 6"
 
diff --git a/gdb/testsuite/gdb.base/args.exp b/gdb/testsuite/gdb.base/args.exp
index 6c416fcfc8..ee75445c0b 100644
--- a/gdb/testsuite/gdb.base/args.exp
+++ b/gdb/testsuite/gdb.base/args.exp
@@ -23,6 +23,12 @@ if [target_info exists noargs] {
     return
 }
 
+# This test requires starting new inferior processes, skip it if the target
+# board is a stub.
+if [use_gdb_stub] {
+    return
+}
+
 standard_testfile
 
 if {[build_executable $testfile.exp $testfile \
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index f7d20bd94f..66c34c718c 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -246,10 +246,13 @@ proc target_can_use_run_cmd {} {
 # Using ``.*$'' could swallow up output that we attempt to match
 # elsewhere.
 #
+# INFERIOR_ARGS is passed as arguments to the start command, so may contain
+# inferior arguments.
+#
 # N.B. This function does not wait for gdb to return to the prompt,
 # that is the caller's responsibility.
 
-proc gdb_run_cmd {args} {
+proc gdb_run_cmd { {inferior_args {}} } {
     global gdb_prompt use_gdb_stub
 
     foreach command [gdb_init_commands] {
@@ -265,7 +268,7 @@ proc gdb_run_cmd {args} {
 
     if $use_gdb_stub {
 	if [target_info exists gdb,do_reload_on_run] {
-	    if { [gdb_reload] != 0 } {
+	    if { [gdb_reload $inferior_args] != 0 } {
 		return
 	    }
 	    send_gdb "continue\n"
@@ -310,7 +313,7 @@ proc gdb_run_cmd {args} {
 		    send_gdb "y\n" answer
 		}
 		-re "The program is not being run.*$gdb_prompt $" {
-		    if { [gdb_reload] != 0 } {
+		    if { [gdb_reload $inferior_args] != 0 } {
 			return
 		    }
 		    send_gdb "jump *$start\n"
@@ -325,11 +328,11 @@ proc gdb_run_cmd {args} {
     }
 
     if [target_info exists gdb,do_reload_on_run] {
-	if { [gdb_reload] != 0 } {
+	if { [gdb_reload $inferior_args] != 0 } {
 	    return
 	}
     }
-    send_gdb "run $args\n"
+    send_gdb "run $inferior_args\n"
 # This doesn't work quite right yet.
 # Use -notransfer here so that test cases (like chng-sym.exp)
 # may test for additional start-up messages.
@@ -348,10 +351,13 @@ proc gdb_run_cmd {args} {
 # Generic start command.  Return 0 if we could start the program, -1
 # if we could not.
 #
+# INFERIOR_ARGS is passed as arguments to the start command, so may contain
+# inferior arguments.
+#
 # N.B. This function does not wait for gdb to return to the prompt,
 # that is the caller's responsibility.
 
-proc gdb_start_cmd {args} {
+proc gdb_start_cmd { {inferior_args {}} } {
     global gdb_prompt use_gdb_stub
 
     foreach command [gdb_init_commands] {
@@ -369,7 +375,7 @@ proc gdb_start_cmd {args} {
 	return -1
     }
 
-    send_gdb "start $args\n"
+    send_gdb "start $inferior_args\n"
     # Use -notransfer here so that test cases (like chng-sym.exp)
     # may test for additional start-up messages.
     gdb_expect 60 {
@@ -387,10 +393,13 @@ proc gdb_start_cmd {args} {
 # Generic starti command.  Return 0 if we could start the program, -1
 # if we could not.
 #
+# INFERIOR_ARGS is passed as arguments to the starti command, so may contain
+# inferior arguments.
+#
 # N.B. This function does not wait for gdb to return to the prompt,
 # that is the caller's responsibility.
 
-proc gdb_starti_cmd {args} {
+proc gdb_starti_cmd { {inferior_args {}} } {
     global gdb_prompt use_gdb_stub
 
     foreach command [gdb_init_commands] {
@@ -408,7 +417,7 @@ proc gdb_starti_cmd {args} {
 	return -1
     }
 
-    send_gdb "starti $args\n"
+    send_gdb "starti $inferior_args\n"
     gdb_expect 60 {
 	-re "The program .* has been started already.*y or n. $" {
 	    send_gdb "y\n" answer
@@ -4818,8 +4827,13 @@ proc gdb_load { arg } {
 # either the first time or after already starting the program once,
 # for remote targets.  Most files that override gdb_load should now
 # override this instead.
+#
+# INFERIOR_ARGS contains the arguments to pass to the inferiors, as a
+# single string to get interpreted by a shell.  If the target board
+# overriding gdb_reload is a "stub", then it should arrange things such
+# these arguments make their way to the inferior process.
 
-proc gdb_reload { } {
+proc gdb_reload { {inferior_args {}} } {
     # For the benefit of existing configurations, default to gdb_load.
     # Specifying no file defaults to the executable currently being
     # debugged.
-- 
2.26.2

[PATCH v5 9/9] gdb/testsuite: add inferior arguments test

[Michael Weghorn]

From: Simon Marchi <simon.marchi@efficios.com>

Add a test for verifying different methods of passing arguments to the
inferior: the start, starti and run commands, as well as `set args`.

All these methods work naturally when using the unix or
native-extended-gdbserver target boards.  Since those are non-stub
boards, GDB runs new inferiors and therefore pass arguments to them.
With target boards where GDB connects to a stub, for example with
native-gdbserver, they don't really make sense.  The inferior process is
already started when GDB connects.

However, the "run" method is still tested with stub targets, because the
gdb_run_cmd procedure is adapted for stub targets.  Instead of issuing
the `run` command, it spawns whatever program is supposed to bring up
the stub (gdbserver, for example) using gdb_reload and makes GDB connect
to it.  So this allows us to exercise argument passing through the
gdbserver command line, when testing with the native-gdbserver board.

Note that there is already a gdb.base/args.exp, but this tests
specifically the --args switch of GDB.  Perhaps it could be integrated
in this new test, as a new "method".

gdb/testsuite/ChangeLog:

	* lib/gdb.exp (gdb_run_cmd): Return success or failure.
	* gdb.base/inferior-args.exp: New file.
	* gdb.base/inferior-args.c: New file.
---
 gdb/testsuite/gdb.base/inferior-args.c   |  26 +++++
 gdb/testsuite/gdb.base/inferior-args.exp | 124 +++++++++++++++++++++++
 gdb/testsuite/lib/gdb.exp                |  21 ++--
 3 files changed, 163 insertions(+), 8 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/inferior-args.c
 create mode 100644 gdb/testsuite/gdb.base/inferior-args.exp

diff --git a/gdb/testsuite/gdb.base/inferior-args.c b/gdb/testsuite/gdb.base/inferior-args.c
new file mode 100644
index 0000000000..1a038030d7
--- /dev/null
+++ b/gdb/testsuite/gdb.base/inferior-args.c
@@ -0,0 +1,26 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2020 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include <stdio.h>
+
+int main (int argc, char **argv)
+{
+  for (int i = 0; i < argc; i++)
+    printf ("[%d] %s\n", i, argv[i]);
+
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/inferior-args.exp b/gdb/testsuite/gdb.base/inferior-args.exp
new file mode 100644
index 0000000000..477da9b403
--- /dev/null
+++ b/gdb/testsuite/gdb.base/inferior-args.exp
@@ -0,0 +1,124 @@
+# Copyright 2020 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Test running an inferior with arguments.
+
+# This does not work on boards that don't support inferior arguments.
+if [target_info exists noargs] then {
+    verbose "skipping gdb.base/inferior-args.exp because of noargs"
+    return
+}
+
+standard_testfile .c
+
+if {[build_executable "failed to prepare" $testfile $srcfile debug] == -1} {
+    return
+}
+
+proc do_test { method } {
+    global binfile hex
+
+    # The second arg is an empty string on purpose.
+    set inferior_args { "first arg" "" "third-arg" }
+
+    clean_restart $binfile
+
+    if { $method == "start" } {
+	# The start command does not make sense for a stub.
+	if { [use_gdb_stub] } {
+	    return;
+	}
+
+	if { [gdb_start_cmd $inferior_args] < 0 } {
+	    fail "could not issue start command"
+	    return -1
+	}
+
+	# Consume up to the GDB prompt after the stop.
+	gdb_test "" ".*main.*" "stop at main"
+
+    } elseif { $method == "starti" } {
+	# The starti command does not make sense for a stub.
+	if { [use_gdb_stub] } {
+	    return;
+	}
+
+	if { [gdb_starti_cmd $inferior_args] < 0 } {
+	    fail "could not issue start command"
+	    return -1
+	}
+
+	# Consume up to the GDB prompt after the stop.
+	gdb_test "" "" "stop at first instruction"
+
+	# Put a breakpoint and continue until main.
+	if { ![gdb_breakpoint "main" message] } {
+	    fail "could not set breakpoint on main"
+	    return -1
+	}
+
+	if { [gdb_continue "main"] != 0 } {
+	    fail "could not continue to main"
+	    return -1
+	}
+
+    } elseif { $method == "run" } {
+	if { ![gdb_breakpoint "main" message] } {
+	    fail "could not set breakpoint on main"
+	    return -1
+	}
+
+	# The run command does not make sense for a stub, but GDB_RUN_CMD
+	# does the right thing when the target is a stub (start the stub,
+	# connect to it, and "continue").
+	#
+	# This allows us to test arguments passed on the gdbserver command
+	# line.
+	if { [gdb_run_cmd $inferior_args] < 0 } {
+	    fail "could not run"
+	    return -1
+	}
+
+	# Consume up to the GDB prompt after the stop.
+	gdb_test "" ".*main.*" "stop at main"
+
+    } elseif { $method == "set args" } {
+	# Using "set args" does not make sense with a stub.
+	if { [use_gdb_stub] } {
+	    return;
+	}
+
+	gdb_test_no_output "set args $inferior_args"
+
+	if { ![runto_main] } {
+	    fail "could not run to main"
+	    return -1
+	}
+
+    } else {
+	error "invalid method $method"
+    }
+
+    # Now that we are stopped at main, inspect argc/argv.
+    gdb_test "print argc" " = 4"
+    gdb_test "print argv\[0\]" " = $hex \".*\""
+    gdb_test "print argv\[1\]" " = $hex \"first arg\""
+    gdb_test "print argv\[2\]" " = $hex \"\""
+    gdb_test "print argv\[3\]" " = $hex \"third-arg\""
+}
+
+foreach_with_prefix method { "start" "starti" "run" "set args" } {
+    do_test $method
+}
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 66c34c718c..c2f39ee70a 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -242,6 +242,8 @@ proc target_can_use_run_cmd {} {
 
 # Generic run command.
 #
+# Return 0 if we could start the program, -1 if we could not.
+#
 # The second pattern below matches up to the first newline *only*.
 # Using ``.*$'' could swallow up output that we attempt to match
 # elsewhere.
@@ -269,14 +271,14 @@ proc gdb_run_cmd { {inferior_args {}} } {
     if $use_gdb_stub {
 	if [target_info exists gdb,do_reload_on_run] {
 	    if { [gdb_reload $inferior_args] != 0 } {
-		return
+		return -1
 	    }
 	    send_gdb "continue\n"
 	    gdb_expect 60 {
 		-re "Continu\[^\r\n\]*\[\r\n\]" {}
 		default {}
 	    }
-	    return
+	    return 0
 	}
 
 	if [target_info exists gdb,start_symbol] {
@@ -292,7 +294,7 @@ proc gdb_run_cmd { {inferior_args {}} } {
 	    # clever and not send a command when it has failed.
 	    if [expr $start_attempt > 3] {
 		perror "Jump to start() failed (retry count exceeded)"
-		return
+		return -1
 	    }
 	    set start_attempt [expr $start_attempt + 1]
 	    gdb_expect 30 {
@@ -301,7 +303,7 @@ proc gdb_run_cmd { {inferior_args {}} } {
 		}
 		-re "No symbol \"_start\" in current.*$gdb_prompt $" {
 		    perror "Can't find start symbol to run in gdb_run"
-		    return
+		    return -1
 		}
 		-re "No symbol \"start\" in current.*$gdb_prompt $" {
 		    send_gdb "jump *_start\n"
@@ -314,22 +316,23 @@ proc gdb_run_cmd { {inferior_args {}} } {
 		}
 		-re "The program is not being run.*$gdb_prompt $" {
 		    if { [gdb_reload $inferior_args] != 0 } {
-			return
+			return -1
 		    }
 		    send_gdb "jump *$start\n"
 		}
 		timeout {
 		    perror "Jump to start() failed (timeout)"
-		    return
+		    return -1
 		}
 	    }
 	}
-	return
+
+	return 0
     }
 
     if [target_info exists gdb,do_reload_on_run] {
 	if { [gdb_reload $inferior_args] != 0 } {
-	    return
+	    return -1
 	}
     }
     send_gdb "run $inferior_args\n"
@@ -346,6 +349,8 @@ proc gdb_run_cmd { {inferior_args {}} } {
 	    # There is no more input expected.
 	}
     }
+
+    return 0
 }
 
 # Generic start command.  Return 0 if we could start the program, -1
-- 
2.26.2

Re: [PATCH 2/2] gdb/testsuite: add inferior arguments test

[Michael Weghorn]

I've just sent an updated patch series (v5).

On 19/05/2020 19.13, Simon Marchi wrote:
> On 2020-05-15 4:07 p.m., Pedro Alves via Gdb-patches wrote:
>> On 5/10/20 4:59 PM, Simon Marchi via Gdb-patches wrote:
>>
>>> +++ b/gdb/testsuite/gdb.base/inferior-args.c
>>> @@ -0,0 +1,8 @@
>>> +#include <stdio.h>
>>> +
>>> +int main(int argc, char **argv) {
>>> +	for (int i = 0; i < argc; i++)
>>> +		printf("[%d] %s\n", i, argv[i]);
>>> +
>>> +	return 0;
>>> +}
>>
>> Formatting.  Also missing copyright header.

Done in v5.

>>
>>> +standard_testfile .c
>>> +
>>> +if {[build_executable "failed to prepare" $testfile $srcfile debug] == -1} {
>>> +    return
>>> +}
>>> +
>>> +clean_restart $binfile
>>
>> This seems unnecessary, since do_test already calls it.
>>
>> Otherwise, you can use prepare_for_testing here.

I've removed the extra call to 'clean_restart'.
> 
> Thanks for the comments.  Michael, would you mind addressing these changes
> in my patches in your branch?
> 
> Simon

I've addressed the comments in this email, but so far not those in the
other thread:
https://sourceware.org/pipermail/gdb-patches/2020-May/168795.html

Please let me know if you want me to look at those as well. Handling
that wasn't straightforward to me. I'd have to dig deeper (in particular
how tcl works) in order to properly address those.

Michael

Re: [PATCH v4 1/9] gdb: Move construct_inferior_arguments to gdbsupport

[Michael Weghorn]

On 13/05/2020 17.05, Simon Marchi wrote:
> On 2020-05-13 11:01 a.m., Michael Weghorn wrote:
>> On 13/05/2020 16.39, Simon Marchi wrote:
>>> On 2020-05-13 5:47 a.m., Michael Weghorn via Gdb-patches wrote:
>>>> This moves the function construct_inferior_arguments from
>>>> gdb/inferior.h and gdb/infcmd.c to gdbsupport/common-inferior.{h,cc}.
>>>> While at it, also move the function's comment to the header file
>>>> to align with current standards.
>>>>
>>>> The intention is to use it from gdbserver in a follow-up commit.
>>>
>>> Hi Michael,
>>>
>>> The series LGTM.  I think all we need now is to wait for your copyright assignment to
>>> be completed?
>>>
>>> Simon
>>>
>> Hi Simon,
>>
>> thank you again for all help.
>>
>> I've signed the copyright assignment last week and sent it by email, but
>> haven't received an FSF-signed version back so far.
>>
>> Michael
>>
> 
> Ok, let me know when you hear back.
> 
> Simon
> 

I have received a reply with the FSF-signed version of the copyright
assignment now.

Do you need additional information on that? (e.g. a copy of the
assignment or the lines in the "INFORMATION FOR THE MAINTAINER(S)"
section in the email that contains the answers of some to the questions
from the initial form from
http://git.savannah.gnu.org/cgit/gnulib.git/tree/doc/Copyright/request-assign.future
)

Michael

Re: [PATCH v4 1/9] gdb: Move construct_inferior_arguments to gdbsupport

[Simon Marchi]

On 2020-05-20 12:37 p.m., Michael Weghorn wrote:
> On 13/05/2020 17.05, Simon Marchi wrote:
>> On 2020-05-13 11:01 a.m., Michael Weghorn wrote:
>>> On 13/05/2020 16.39, Simon Marchi wrote:
>>>> On 2020-05-13 5:47 a.m., Michael Weghorn via Gdb-patches wrote:
>>>>> This moves the function construct_inferior_arguments from
>>>>> gdb/inferior.h and gdb/infcmd.c to gdbsupport/common-inferior.{h,cc}.
>>>>> While at it, also move the function's comment to the header file
>>>>> to align with current standards.
>>>>>
>>>>> The intention is to use it from gdbserver in a follow-up commit.
>>>>
>>>> Hi Michael,
>>>>
>>>> The series LGTM.  I think all we need now is to wait for your copyright assignment to
>>>> be completed?
>>>>
>>>> Simon
>>>>
>>> Hi Simon,
>>>
>>> thank you again for all help.
>>>
>>> I've signed the copyright assignment last week and sent it by email, but
>>> haven't received an FSF-signed version back so far.
>>>
>>> Michael
>>>
>>
>> Ok, let me know when you hear back.
>>
>> Simon
>>
> 
> I have received a reply with the FSF-signed version of the copyright
> assignment now.
> 
> Do you need additional information on that? (e.g. a copy of the
> assignment or the lines in the "INFORMATION FOR THE MAINTAINER(S)"
> section in the email that contains the answers of some to the questions
> from the initial form from
> http://git.savannah.gnu.org/cgit/gnulib.git/tree/doc/Copyright/request-assign.future
> )

No, thanks, I can see your entry in the copyright assigment file, so that's fine.

We can look into merging your series as soon as the last question about the way
of passing args to gdbserver is settled.

Simon

Re: [PATCH v4 8/9] gdb/testsuite: support passing inferior arguments with native-gdbserver board

[Simon Marchi]

On 2020-05-19 2:46 p.m., Tom de Vries wrote:
> On 19-05-2020 19:22, Simon Marchi wrote:
>> So I'd keep "inferior_args",
>> although we can make it accept a list instead of a string, and apply the
>> treatment you have shown above.
>>
>> Or, I like the procedures that accept optional arguments using shell-like switches,
>> so we could do:
>>
>>   set inferior_args [list a b c]
>>   gdb_run_cmd -inferior-args $inferior_args
>>
>> This is untested, I always forget how tcl converts between strings and lists
>> exactly, but you should see the point.  That makes it easier to add other parameters
>> in the future.
> 
> Yep, sounds good to me.

I think I'll do that, but as a separate change.  So I'll merge the series as-is, to get
the fix and test in, and then look into adding the `-inferior-args` option to gdb_run_cmd
and friends.

Simon

Re: [PATCH v5 1/9] gdb: Move construct_inferior_arguments to gdbsupport

[Simon Marchi]

On 2020-05-20 12:21 p.m., Michael Weghorn via Gdb-patches wrote:
> This moves the function construct_inferior_arguments from
> gdb/inferior.h and gdb/infcmd.c to gdbsupport/common-inferior.{h,cc}.
> While at it, also move the function's comment to the header file
> to align with current standards.
> 
> The intention is to use it from gdbserver in a follow-up commit.
> 
> gdb/ChangeLog:
> 
> 2020-05-13  Michael Weghorn  <m.weghorn@posteo.de>
> 
>         * infcmd.c, inferior.h: (construct_inferior_arguments):
>           Moved function from here to gdbsupport/common-inferior.{h,cc}
> 
> gdbsupport/ChangeLog:
> 
> w020-05-13  Michael Weghorn  <m.weghorn@posteo.de>
> 
>         * common-inferior.h, common-inferior.cc: (construct_inferior_arguments):
>         Move function here from gdb/infcmd.c, gdb/inferior.h

I've pushed the whole series, we can take a look at improving how inferior arguments
are passed to gdb_run_cmd as a separate change.

Thanks a lot for your contribution and perseverance!

Simon

Re: [PATCH v5 1/9] gdb: Move construct_inferior_arguments to gdbsupport

[Michael Weghorn]

On 25/05/2020 17.42, Simon Marchi wrote:
> On 2020-05-20 12:21 p.m., Michael Weghorn via Gdb-patches wrote:
>> This moves the function construct_inferior_arguments from
>> gdb/inferior.h and gdb/infcmd.c to gdbsupport/common-inferior.{h,cc}.
>> While at it, also move the function's comment to the header file
>> to align with current standards.
>>
>> The intention is to use it from gdbserver in a follow-up commit.
>>
>> gdb/ChangeLog:
>>
>> 2020-05-13  Michael Weghorn  <m.weghorn@posteo.de>
>>
>>         * infcmd.c, inferior.h: (construct_inferior_arguments):
>>           Moved function from here to gdbsupport/common-inferior.{h,cc}
>>
>> gdbsupport/ChangeLog:
>>
>> w020-05-13  Michael Weghorn  <m.weghorn@posteo.de>
>>
>>         * common-inferior.h, common-inferior.cc: (construct_inferior_arguments):
>>         Move function here from gdb/infcmd.c, gdb/inferior.h
> 
> I've pushed the whole series, we can take a look at improving how inferior arguments
> are passed to gdb_run_cmd as a separate change.
> 
> Thanks a lot for your contribution and perseverance!
> 
> Simon


Thanks for pushing and all help along the process!

Michael

Re: [PATCH v5 1/9] gdb: Move construct_inferior_arguments to gdbsupport

[Simon Marchi]

On 2020-05-26 02:17, Michael Weghorn wrote:
> On 25/05/2020 17.42, Simon Marchi wrote:
>> On 2020-05-20 12:21 p.m., Michael Weghorn via Gdb-patches wrote:
>>> This moves the function construct_inferior_arguments from
>>> gdb/inferior.h and gdb/infcmd.c to gdbsupport/common-inferior.{h,cc}.
>>> While at it, also move the function's comment to the header file
>>> to align with current standards.
>>>
>>> The intention is to use it from gdbserver in a follow-up commit.
>>>
>>> gdb/ChangeLog:
>>>
>>> 2020-05-13  Michael Weghorn  <m.weghorn@posteo.de>
>>>
>>>         * infcmd.c, inferior.h: (construct_inferior_arguments):
>>>           Moved function from here to gdbsupport/common-inferior.{h,cc}
>>>
>>> gdbsupport/ChangeLog:
>>>
>>> w020-05-13  Michael Weghorn  <m.weghorn@posteo.de>
>>>
>>>         * common-inferior.h, common-inferior.cc: (construct_inferior_arguments):
>>>         Move function here from gdb/infcmd.c, gdb/inferior.h
>>
>> I've pushed the whole series, we can take a look at improving how inferior arguments
>> are passed to gdb_run_cmd as a separate change.
>>
>> Thanks a lot for your contribution and perseverance!
>>
>> Simon
> 
> 
> Thanks for pushing and all help along the process!
> 
> Michael
> 

Hi Michael,

Can you take a look at this bug?

https://sourceware.org/bugzilla/show_bug.cgi?id=28392

Thanks,

Simon

Re: [PATCH v5 1/9] gdb: Move construct_inferior_arguments to gdbsupport

[Michael Weghorn]

On 28/09/2021 16.33, Simon Marchi wrote:
> Hi Michael,
> 
> Can you take a look at this bug?
> 
> https://sourceware.org/bugzilla/show_bug.cgi?id=28392
> 
> Thanks,
> 
> Simon
> 

Hi Simon,

yes, I hope to find some time later today or tomorrow to take a closer look.

I must admit I was not aware gdbserver supports shell argument globbing 
and variable substitution.

Michael