[PATCH] gdbserver: fix handling of single quote arguments

[PATCH] gdbserver: fix handling of single quote arguments

[Andrew Burgess]

I noticed that passing arguments containing single quotes to gdbserver
didn't work correctly:

  gdb -ex 'set sysroot' --args /tmp/show-args
  Reading symbols from /tmp/show-args...
  (gdb) target extended-remote | gdbserver --once --multi - /tmp/show-args
  Remote debugging using | gdbserver --once --multi - /tmp/show-args
  stdin/stdout redirected
  Process /tmp/show-args created; pid = 176054
  Remote debugging using stdio
  Reading symbols from /lib64/ld-linux-x86-64.so.2...
  (No debugging symbols found in /lib64/ld-linux-x86-64.so.2)
  0x00007ffff7fd3110 in _start () from /lib64/ld-linux-x86-64.so.2
  (gdb) set args \'
  (gdb) r
  The program being debugged has been started already.
  Start it from the beginning? (y or n) y
  Starting program: /tmp/show-args \'
  stdin/stdout redirected
  Process /tmp/show-args created; pid = 176088
  2 args are:
    /tmp/show-args
    \'
  Done.
  [Inferior 1 (process 176088) exited normally]
  (gdb) target native
  Done.  Use the "run" command to start a process.
  (gdb) run
  Starting program: /tmp/show-args \'
  2 args are:
    /tmp/show-args
    '
  Done.
  [Inferior 1 (process 176095) exited normally]
  (gdb) q

The 'shows-args' program used here just prints the arguments passed to
the inferior.

Notice that when starting the inferior using the extended-remote
target the second argument is "\'", while when running using native
target the argument is "'".  The second of these is correct, the \'
used with the "set args" command is just to show GDB that the single
quote is not opening an argument string.

It turns out that the extra backslash is injected on the gdbserver
side when gdbserver processes the arguments that GDB passes it, the
code that does this was added as part of this much larger commit:

  commit 2090129c36c7e582943b7d300968d19b46160d84
  Date:   Thu Dec 22 21:11:11 2016 -0500

      Share fork_inferior et al with gdbserver

In this commit I propose removing the specific code that adds what I
believe is a stray backslash.  I've extended an existing test to cover
this case, and I now see identical behaviour when using an
extended-remote target as with the native target.
---
 gdb/testsuite/gdb.base/inferior-args.exp | 7 +++++--
 gdbserver/server.cc                      | 6 ------
 2 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/gdb/testsuite/gdb.base/inferior-args.exp b/gdb/testsuite/gdb.base/inferior-args.exp
index 19bada6d2c7..3d3cd39a706 100644
--- a/gdb/testsuite/gdb.base/inferior-args.exp
+++ b/gdb/testsuite/gdb.base/inferior-args.exp
@@ -29,7 +29,7 @@ proc do_test { method } {
     global binfile hex
 
     # The second arg is an empty string on purpose.
-    set inferior_args { "first arg" "" "third-arg" }
+    set inferior_args { "first arg" "" "third-arg" "'" "\"" " " }
 
     clean_restart $binfile
 
@@ -109,11 +109,14 @@ proc do_test { method } {
     }
 
     # Now that we are stopped at main, inspect argc/argv.
-    gdb_test "print argc" " = 4"
+    gdb_test "print argc" " = 7"
     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\""
+    gdb_test "print argv\[4\]" " = $hex \"'\""
+    gdb_test "print argv\[5\]" " = $hex \"\\\\\"\""
+    gdb_test "print argv\[6\]" " = $hex \" \""
 }
 
 foreach_with_prefix method { "start" "starti" "run" "set args" } {
diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index c57270175b4..496b9bebb7d 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -3011,12 +3011,6 @@ handle_v_run (char *own_buf)
 		  need_quote = 1;
 		  break;
 
-		case '\'':
-		  /* Quote single quote.  */
-		  *tmp_full_arg = '\\';
-		  ++tmp_full_arg;
-		  break;
-
 		default:
 		  break;
 		}

base-commit: f586e3409b752748bf213520c2dbb0b44e0005d8
-- 
2.25.4

Re: [PATCH] gdbserver: fix handling of single quote arguments

[Andreas Schwab]

On Sep 27 2023, Andrew Burgess via Gdb-patches wrote:

> In this commit I propose removing the specific code that adds what I
> believe is a stray backslash.  I've extended an existing test to cover
> this case, and I now see identical behaviour when using an
> extended-remote target as with the native target.

It seems like arguments containing newlines, and a trailing empty
argument are also mishandled.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

[PATCHv2 0/5] Fixes for passing arguments to gdbserver

[Andrew Burgess]

Some fixes for passing arguments to gdbserver.

---

Andrew Burgess (5):
  gdbserver: fix handling of single quote arguments
  gdbserver: fix handling of trailing empty argument
  gdbserver: handle newlines in inferior arguments
  gdbserver: cleanup in handle_v_run
  gdb/testsuite: cleanup in gdb.base/args.exp

 gdb/testsuite/gdb.base/args.exp          | 91 +++++++++---------------
 gdb/testsuite/gdb.base/inferior-args.exp | 12 +++-
 gdbserver/server.cc                      | 55 +++-----------
 3 files changed, 52 insertions(+), 106 deletions(-)


base-commit: f586e3409b752748bf213520c2dbb0b44e0005d8
-- 
2.25.4

[PATCHv2 1/5] gdbserver: fix handling of single quote arguments

[Andrew Burgess]

I noticed that passing arguments containing single quotes to gdbserver
didn't work correctly:

  gdb -ex 'set sysroot' --args /tmp/show-args
  Reading symbols from /tmp/show-args...
  (gdb) target extended-remote | gdbserver --once --multi - /tmp/show-args
  Remote debugging using | gdbserver --once --multi - /tmp/show-args
  stdin/stdout redirected
  Process /tmp/show-args created; pid = 176054
  Remote debugging using stdio
  Reading symbols from /lib64/ld-linux-x86-64.so.2...
  (No debugging symbols found in /lib64/ld-linux-x86-64.so.2)
  0x00007ffff7fd3110 in _start () from /lib64/ld-linux-x86-64.so.2
  (gdb) set args \'
  (gdb) r
  The program being debugged has been started already.
  Start it from the beginning? (y or n) y
  Starting program: /tmp/show-args \'
  stdin/stdout redirected
  Process /tmp/show-args created; pid = 176088
  2 args are:
    /tmp/show-args
    \'
  Done.
  [Inferior 1 (process 176088) exited normally]
  (gdb) target native
  Done.  Use the "run" command to start a process.
  (gdb) run
  Starting program: /tmp/show-args \'
  2 args are:
    /tmp/show-args
    '
  Done.
  [Inferior 1 (process 176095) exited normally]
  (gdb) q

The 'shows-args' program used here just prints the arguments passed to
the inferior.

Notice that when starting the inferior using the extended-remote
target the second argument is "\'", while when running using native
target the argument is "'".  The second of these is correct, the \'
used with the "set args" command is just to show GDB that the single
quote is not opening an argument string.

It turns out that the extra backslash is injected on the gdbserver
side when gdbserver processes the arguments that GDB passes it, the
code that does this was added as part of this much larger commit:

  commit 2090129c36c7e582943b7d300968d19b46160d84
  Date:   Thu Dec 22 21:11:11 2016 -0500

      Share fork_inferior et al with gdbserver

In this commit I propose removing the specific code that adds what I
believe is a stray backslash.  I've extended an existing test to cover
this case, and I now see identical behaviour when using an
extended-remote target as with the native target.

This partially fixes PR gdb/27989, though there are still some issues
with newline handling which I'll address in a later commit.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=27989
---
 gdb/testsuite/gdb.base/args.exp          | 4 ++--
 gdb/testsuite/gdb.base/inferior-args.exp | 7 +++++--
 gdbserver/server.cc                      | 6 ------
 3 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/gdb/testsuite/gdb.base/args.exp b/gdb/testsuite/gdb.base/args.exp
index 0b55c4444aa..092b44bd61d 100644
--- a/gdb/testsuite/gdb.base/args.exp
+++ b/gdb/testsuite/gdb.base/args.exp
@@ -94,10 +94,10 @@ save_vars { GDBFLAGS } {
     # Try with arguments containing literal single quotes.
 
     set GDBFLAGS "$old_gdbflags --args $binfile 1 '' 3"
-    args_test "one empty with single quotes" {{1} {''} {3}} $single_quotes_newline_kfail
+    args_test "one empty with single quotes" {{1} {''} {3}}
 
     set GDBFLAGS "$old_gdbflags --args $binfile 1 '' '' 3"
-    args_test "two empty with single quotes" {{1} {''} {''} {3}} $single_quotes_newline_kfail
+    args_test "two empty with single quotes" {{1} {''} {''} {3}}
 
     # try with arguments containing literal newlines.
 
diff --git a/gdb/testsuite/gdb.base/inferior-args.exp b/gdb/testsuite/gdb.base/inferior-args.exp
index 19bada6d2c7..3d3cd39a706 100644
--- a/gdb/testsuite/gdb.base/inferior-args.exp
+++ b/gdb/testsuite/gdb.base/inferior-args.exp
@@ -29,7 +29,7 @@ proc do_test { method } {
     global binfile hex
 
     # The second arg is an empty string on purpose.
-    set inferior_args { "first arg" "" "third-arg" }
+    set inferior_args { "first arg" "" "third-arg" "'" "\"" " " }
 
     clean_restart $binfile
 
@@ -109,11 +109,14 @@ proc do_test { method } {
     }
 
     # Now that we are stopped at main, inspect argc/argv.
-    gdb_test "print argc" " = 4"
+    gdb_test "print argc" " = 7"
     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\""
+    gdb_test "print argv\[4\]" " = $hex \"'\""
+    gdb_test "print argv\[5\]" " = $hex \"\\\\\"\""
+    gdb_test "print argv\[6\]" " = $hex \" \""
 }
 
 foreach_with_prefix method { "start" "starti" "run" "set args" } {
diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index c57270175b4..496b9bebb7d 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -3011,12 +3011,6 @@ handle_v_run (char *own_buf)
 		  need_quote = 1;
 		  break;
 
-		case '\'':
-		  /* Quote single quote.  */
-		  *tmp_full_arg = '\\';
-		  ++tmp_full_arg;
-		  break;
-
 		default:
 		  break;
 		}
-- 
2.25.4

[PATCHv2 2/5] gdbserver: fix handling of trailing empty argument

[Andrew Burgess]

When I posted the previous patch for review Andreas Schwab pointed out
that passing a trailing empty argument also doesn't work.

The fix for this is in the same area of code as the previous patch,
but is sufficiently different that I felt it deserved a patch of its
own.

I noticed that passing arguments containing single quotes to gdbserver
didn't work correctly:

  gdb -ex 'set sysroot' --args /tmp/show-args
  Reading symbols from /tmp/show-args...
  (gdb) target extended-remote | gdbserver --once --multi - /tmp/show-args
  Remote debugging using | gdbserver --once --multi - /tmp/show-args
  stdin/stdout redirected
  Process /tmp/show-args created; pid = 176054
  Remote debugging using stdio
  Reading symbols from /lib64/ld-linux-x86-64.so.2...
  (No debugging symbols found in /lib64/ld-linux-x86-64.so.2)
  0x00007ffff7fd3110 in _start () from /lib64/ld-linux-x86-64.so.2
  (gdb) set args abc ""
  (gdb) run
  The program being debugged has been started already.
  Start it from the beginning? (y or n) y
  Starting program: /tmp/show-args \'
  stdin/stdout redirected
  Process /tmp/show-args created; pid = 176088
  2 args are:
    /tmp/show-args
    abc
  Done.
  [Inferior 1 (process 176088) exited normally]
  (gdb) target native
  Done.  Use the "run" command to start a process.
  (gdb) run
  Starting program: /tmp/show-args \'
  2 args are:
    /tmp/show-args
    abc

  Done.
  [Inferior 1 (process 176095) exited normally]
  (gdb) q

The 'shows-args' program used here just prints the arguments passed to
the inferior.

Notice that when starting the inferior using the extended-remote
target there is only a single argument 'abc', while when using the
native target there is a second argument, the blank line, representing
the empty argument.

The problem here is that the vRun packet coming from GDB looks like
this (I've removing the trailing checksum):

  $vRun;PROGRAM_NAME;616263;

If we compare this to a packet with only a single argument and no
trailing empty argument:

  $vRun;PROGRAM_NAME;616263

Notice the lack of the trailing ';' character here.

The problem is that gdbserver processes this string in a loop.  At
each point we maintain a pointer to the character just after a ';',
and then we process everything up to either the next ';' character, or
to the end of the string.

We break out of this loop when the character we start with (in that
loop iteration) is the null-character.  This means in the trailing
empty argument case, we abort the loop before doing anything with the
empty argument.

In this commit I've updated the loop, we now break out using a 'break'
statement at the end of the loop if the (sub-)string we just processed
was empty, with this change we now notice the trailing empty
argument.

I've updated the test case to cover this issue.
---
 gdb/testsuite/gdb.base/inferior-args.exp | 9 ++++++---
 gdbserver/server.cc                      | 8 +++++---
 2 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/gdb/testsuite/gdb.base/inferior-args.exp b/gdb/testsuite/gdb.base/inferior-args.exp
index 3d3cd39a706..2c920ab14ec 100644
--- a/gdb/testsuite/gdb.base/inferior-args.exp
+++ b/gdb/testsuite/gdb.base/inferior-args.exp
@@ -28,8 +28,10 @@ if {[build_executable "failed to prepare" $testfile $srcfile \
 proc do_test { method } {
     global binfile hex
 
-    # The second arg is an empty string on purpose.
-    set inferior_args { "first arg" "" "third-arg" "'" "\"" " " }
+    # The second arg is an empty string on purpose.  The last argument
+    # must be the empty argument -- we once had a bug where that
+    # wouldn't work!
+    set inferior_args { "first arg" "" "third-arg" "'" "\"" " " "" }
 
     clean_restart $binfile
 
@@ -109,7 +111,7 @@ proc do_test { method } {
     }
 
     # Now that we are stopped at main, inspect argc/argv.
-    gdb_test "print argc" " = 7"
+    gdb_test "print argc" " = 8"
     gdb_test "print argv\[0\]" " = $hex \".*\""
     gdb_test "print argv\[1\]" " = $hex \"first arg\""
     gdb_test "print argv\[2\]" " = $hex \"\""
@@ -117,6 +119,7 @@ proc do_test { method } {
     gdb_test "print argv\[4\]" " = $hex \"'\""
     gdb_test "print argv\[5\]" " = $hex \"\\\\\"\""
     gdb_test "print argv\[6\]" " = $hex \" \""
+    gdb_test "print argv\[7\]" " = $hex \"\""
 }
 
 foreach_with_prefix method { "start" "starti" "run" "set args" } {
diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index 496b9bebb7d..d78eb5a7d94 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -2969,7 +2969,9 @@ handle_v_run (char *own_buf)
   char *new_program_name = NULL;
   int i;
 
-  for (i = 0, p = own_buf + strlen ("vRun;"); *p; p = next_p, ++i)
+  for (i = 0, p = own_buf + strlen ("vRun;");
+       /* Exit condition is at the end of the loop.  */;
+       p = next_p + 1, ++i)
     {
       next_p = strchr (p, ';');
       if (next_p == NULL)
@@ -3032,8 +3034,8 @@ handle_v_run (char *own_buf)
 	    new_argv.push_back (full_arg);
 	  xfree (arg);
 	}
-      if (*next_p)
-	next_p++;
+      if (*next_p == '\0')
+	break;
     }
 
   if (new_program_name == NULL)
-- 
2.25.4

[PATCHv2 3/5] gdbserver: handle newlines in inferior arguments

[Andrew Burgess]

Similarly to how single quotes were mishandled, which was fixed two
commits ago, this commit fixes handling of newlines in arguments
passed to gdbserver.

We already had a test that covered this, gdb.base/args.exp, which,
when run with the native-extended-gdbserver board contained several
KFAIL covering this situation.

In this commit I remove the unnecessary, attempt to quote incoming
newlines within arguments, and do some minimal cleanup of the related
code.  There is additional cleanup that can be done, but I'm leaving
that for the next commit.

Then I've removed the KFAIL from the test case, and performed some
minimal cleanup there too.

After this commit the gdb.base/args.exp is 100% passing with the
native-extended-gdbserver board file.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=27989
---
 gdb/testsuite/gdb.base/args.exp | 20 +++++---------------
 gdbserver/server.cc             | 17 -----------------
 2 files changed, 5 insertions(+), 32 deletions(-)

diff --git a/gdb/testsuite/gdb.base/args.exp b/gdb/testsuite/gdb.base/args.exp
index 092b44bd61d..0e2dc8b1399 100644
--- a/gdb/testsuite/gdb.base/args.exp
+++ b/gdb/testsuite/gdb.base/args.exp
@@ -30,10 +30,10 @@ if {[build_executable $testfile.exp $testfile \
     return -1
 }
 
-# If SINGLE_QUOTES_NEWLINE_KFAIL true, arguments made of exactly '' or a
-# newline character will fail, so kfail those tests.
+# NAME is the name to use for the tests and ARGLIST is the list of
+# expected arguments.
 
-proc args_test { name arglist {single_quotes_newline_kfail false}} {
+proc args_test { name arglist } {
     global srcdir
     global subdir
     global testfile
@@ -51,10 +51,6 @@ proc args_test { name arglist {single_quotes_newline_kfail false}} {
 
     set i 1
     foreach arg $arglist {
-	if { $single_quotes_newline_kfail
-	     && ($arg == {''} || $arg == {\\n}) } {
-	    setup_kfail "gdb/27989" "*-*-*"
-	}
 	gdb_test "print argv\[$i\]" "\\\$$decimal = $hex \"$arg\"" \
 	    "argv\[$i\] for $name"
 	set i [expr $i + 1]
@@ -68,12 +64,6 @@ proc args_test { name arglist {single_quotes_newline_kfail false}} {
 save_vars { GDBFLAGS } {
     set old_gdbflags $GDBFLAGS
 
-    # Single quotes and newlines are not well handled by the extended-remote
-    # target:  https://sourceware.org/bugzilla/show_bug.cgi?id=27989
-    set single_quotes_newline_kfail \
-	[expr { [target_info exists gdb_protocol] \
-	        && [target_info gdb_protocol] == "extended-remote" }]
-
     set GDBFLAGS "$old_gdbflags --args $binfile 1 3"
     args_test basic {{1} {3}}
 
@@ -102,8 +92,8 @@ save_vars { GDBFLAGS } {
     # try with arguments containing literal newlines.
 
     set GDBFLAGS "$old_gdbflags --args $binfile 1 {\n} 3"
-    args_test "one newline" {{1} {\\n} {3}} $single_quotes_newline_kfail
+    args_test "one newline" {{1} {\\n} {3}}
 
     set GDBFLAGS "$old_gdbflags --args $binfile 1 {\n} {\n} 3"
-    args_test "two newlines" {{1} {\\n} {\\n} {3}} $single_quotes_newline_kfail
+    args_test "two newlines" {{1} {\\n} {\\n} {3}}
 }
diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index d78eb5a7d94..84b8712e668 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -2997,34 +2997,17 @@ handle_v_run (char *own_buf)
 	  /* These are pointers used to navigate the strings above.  */
 	  char *tmp_arg = arg;
 	  char *tmp_full_arg = full_arg;
-	  int need_quote = 0;
 
 	  hex2bin (p, (gdb_byte *) arg, len);
 	  arg[len] = '\0';
 
 	  while (*tmp_arg != '\0')
 	    {
-	      switch (*tmp_arg)
-		{
-		case '\n':
-		  /* Quote \n.  */
-		  *tmp_full_arg = '\'';
-		  ++tmp_full_arg;
-		  need_quote = 1;
-		  break;
-
-		default:
-		  break;
-		}
-
 	      *tmp_full_arg = *tmp_arg;
 	      ++tmp_full_arg;
 	      ++tmp_arg;
 	    }
 
-	  if (need_quote)
-	    *tmp_full_arg++ = '\'';
-
 	  /* Finish FULL_ARG and push it into the vector containing
 	     the argv.  */
 	  *tmp_full_arg = '\0';
-- 
2.25.4

[PATCHv2 4/5] gdbserver: cleanup in handle_v_run

[Andrew Burgess]

After the previous commit there is now a redundant string copy in
handle_v_run, this commit cleans that up.

There should be no functional change after this commit.
---
 gdbserver/server.cc | 24 +++++-------------------
 1 file changed, 5 insertions(+), 19 deletions(-)

diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index 84b8712e668..e02cdb83b51 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -2989,33 +2989,19 @@ handle_v_run (char *own_buf)
 	}
       else
 	{
+	  /* The length of the decoded argument.  */
 	  size_t len = (next_p - p) / 2;
-	  /* ARG is the unquoted argument received via the RSP.  */
+
+	  /* Buffer to decode the argument into.  */
 	  char *arg = (char *) xmalloc (len + 1);
-	  /* FULL_ARGS will contain the quoted version of ARG.  */
-	  char *full_arg = (char *) xmalloc ((len + 1) * 2);
-	  /* These are pointers used to navigate the strings above.  */
-	  char *tmp_arg = arg;
-	  char *tmp_full_arg = full_arg;
 
 	  hex2bin (p, (gdb_byte *) arg, len);
 	  arg[len] = '\0';
 
-	  while (*tmp_arg != '\0')
-	    {
-	      *tmp_full_arg = *tmp_arg;
-	      ++tmp_full_arg;
-	      ++tmp_arg;
-	    }
-
-	  /* Finish FULL_ARG and push it into the vector containing
-	     the argv.  */
-	  *tmp_full_arg = '\0';
 	  if (i == 0)
-	    new_program_name = full_arg;
+	    new_program_name = arg;
 	  else
-	    new_argv.push_back (full_arg);
-	  xfree (arg);
+	    new_argv.push_back (arg);
 	}
       if (*next_p == '\0')
 	break;
-- 
2.25.4

[PATCHv2 5/5] gdb/testsuite: cleanup in gdb.base/args.exp

[Andrew Burgess]

The last few commits resolved the KFAILs in gdb.base/args.exp.  With
those out of the way we can clean up this test script a little.

In this commit I have:

  - Stopped passing 'nowarnings' flag when building the source file.
    I see no reason why this source should issue a warning,

  - Moved setup of GDBFLAGS into args_test proc, callers that passed a
    newline needed a small tweak, and also the matching code needs
    updating for newline handling, but I think this is nicer, the
    argument lists are now given just once,

  - Updated comment on args_test,

  - Updated other comments.

There should be no change in what is tested after this commit.
---
 gdb/testsuite/gdb.base/args.exp | 89 ++++++++++++++-------------------
 1 file changed, 37 insertions(+), 52 deletions(-)

diff --git a/gdb/testsuite/gdb.base/args.exp b/gdb/testsuite/gdb.base/args.exp
index 0e2dc8b1399..43ea6e5caa8 100644
--- a/gdb/testsuite/gdb.base/args.exp
+++ b/gdb/testsuite/gdb.base/args.exp
@@ -24,76 +24,61 @@ require !use_gdb_stub
 
 standard_testfile
 
-if {[build_executable $testfile.exp $testfile \
-	 $srcfile {debug nowarnings}] == -1} {
+if {[build_executable $testfile.exp $testfile $srcfile] == -1} {
     untested "failed to compile"
     return -1
 }
 
 # NAME is the name to use for the tests and ARGLIST is the list of
-# expected arguments.
+# arguments that are passed to GDB when it is started.
 
 proc args_test { name arglist } {
-    global srcdir
-    global subdir
-    global testfile
-    global hex
-    global decimal
-
-    clean_restart $testfile
-
-    runto_main
-    gdb_breakpoint [gdb_get_line_number "set breakpoint here"]
-    gdb_continue_to_breakpoint "breakpoint for $name"
-
-    set expected_len [expr 1 + [llength $arglist]]
-    gdb_test "print argc" "\\\$$decimal = $expected_len" "argc for $name"
-
-    set i 1
-    foreach arg $arglist {
-	gdb_test "print argv\[$i\]" "\\\$$decimal = $hex \"$arg\"" \
-	    "argv\[$i\] for $name"
-	set i [expr $i + 1]
+    save_vars { ::GDBFLAGS } {
+	set ::GDBFLAGS "$::GDBFLAGS --args $::binfile $arglist"
+
+	clean_restart $::binfile
+
+	runto_main
+	gdb_breakpoint [gdb_get_line_number "set breakpoint here"]
+	gdb_continue_to_breakpoint "breakpoint for $name"
+
+	set expected_len [expr 1 + [llength $arglist]]
+	gdb_test "print argc" "\\\$$::decimal = $expected_len" "argc for $name"
+
+	set i 1
+	foreach arg $arglist {
+	    if { $arg eq "\n" } {
+		set arg {\\n}
+	    }
+	    verbose -log "APB: regexp '$arg'"
+	    gdb_test "print argv\[$i\]" "\\\$$::decimal = $::hex \"$arg\"" \
+		"argv\[$i\] for $name"
+	    set i [expr $i + 1]
+	}
     }
 }
 
-#
 # Test that the --args are processed correctly.
-#
 
-save_vars { GDBFLAGS } {
-    set old_gdbflags $GDBFLAGS
+args_test basic {{1} {3}}
 
-    set GDBFLAGS "$old_gdbflags --args $binfile 1 3"
-    args_test basic {{1} {3}}
+# Test that the --args are processed correctly even if one of them is
+# empty.
 
-    #
-    # Test that the --args are processed correctly even if one of them is empty.
-    # The syntax needed is a little peculiar; DejaGNU treats the arguments as a
-    # list and expands them itself, since no shell redirection is involved.
-    #
-    set GDBFLAGS "$old_gdbflags --args $binfile 1 {} 3"
-    args_test "one empty" {{1} {} {3}}
+args_test "one empty" {{1} {} {3}}
 
-    #
-    # try with 2 empty args
-    #
-    set GDBFLAGS "$old_gdbflags --args $binfile 1 {} {} 3"
-    args_test "two empty" {{1} {} {} 3}
+# Try with 2 empty args.
 
-    # Try with arguments containing literal single quotes.
+args_test "two empty" {{1} {} {} 3}
 
-    set GDBFLAGS "$old_gdbflags --args $binfile 1 '' 3"
-    args_test "one empty with single quotes" {{1} {''} {3}}
+# Try with arguments containing literal single quotes.
 
-    set GDBFLAGS "$old_gdbflags --args $binfile 1 '' '' 3"
-    args_test "two empty with single quotes" {{1} {''} {''} {3}}
+args_test "one empty with single quotes" {{1} {''} {3}}
 
-    # try with arguments containing literal newlines.
+args_test "two empty with single quotes" {{1} {''} {''} {3}}
 
-    set GDBFLAGS "$old_gdbflags --args $binfile 1 {\n} 3"
-    args_test "one newline" {{1} {\\n} {3}}
+# Try with arguments containing literal newlines.
 
-    set GDBFLAGS "$old_gdbflags --args $binfile 1 {\n} {\n} 3"
-    args_test "two newlines" {{1} {\\n} {\\n} {3}}
-}
+args_test "one newline" {{1} "\n" {3}}
+
+args_test "two newlines" {{1} "\n" "\n" {3}}
-- 
2.25.4

Re: [PATCHv2 4/5] gdbserver: cleanup in handle_v_run

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:

Andrew> After the previous commit there is now a redundant string copy in
Andrew> handle_v_run, this commit cleans that up.

Andrew> +	  /* Buffer to decode the argument into.  */
Andrew>  	  char *arg = (char *) xmalloc (len + 1);
... 
Andrew>  	  hex2bin (p, (gdb_byte *) arg, len);
Andrew>  	  arg[len] = '\0';
 
Not really your problem, but since you're changing it anyway, using the
byte_vector form of hex2bin would remove some manual memory management
here.

Tom

Re: [PATCHv2 4/5] gdbserver: cleanup in handle_v_run

[Andrew Burgess]

Tom Tromey <tom@tromey.com> writes:

>>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:
>
> Andrew> After the previous commit there is now a redundant string copy in
> Andrew> handle_v_run, this commit cleans that up.
>
> Andrew> +	  /* Buffer to decode the argument into.  */
> Andrew>  	  char *arg = (char *) xmalloc (len + 1);
> ... 
> Andrew>  	  hex2bin (p, (gdb_byte *) arg, len);
> Andrew>  	  arg[len] = '\0';
>  
> Not really your problem, but since you're changing it anyway, using the
> byte_vector form of hex2bin would remove some manual memory management
> here.

I see two problems with that plan.  First, we currently run hex2bin on a
slice of a larger string buffer, the slice is not null-terminated.  The
current hex2bin that returns a gdb::byte_vector expects a
null-terminated string (it uses strlen internally).  We can easily solve
this by adding an overload of hex2bin that takes a gdb::string_view,
except...

... for the second problem, which is the result of calling hex2bin is
passed off to either new_program_name or the new_argv vector.  There's
no way to "release" the memory from a gdb::byte_vector, so we'd end up
copying the contents into a malloc'd buffer anyway, like:

  gdb::byte_vector vec = hex2bin (gdb::string_view (p, (next_p - p)));
  const char *arg = xstrdup ((const char *) vec.data ());

Which I'm not sure is really any better than we have right now?

Did you have something else in mind?  Let me know and I'm happy to have
a look at an alternative approach.

Thanks,
Andrew

Re: [PATCHv2 4/5] gdbserver: cleanup in handle_v_run

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:

Andrew> Which I'm not sure is really any better than we have right now?

Andrew> Did you have something else in mind?  Let me know and I'm happy to have
Andrew> a look at an alternative approach.

Thanks, I agree your current approach is the better of the two.

Tom

Re: [PATCHv2 5/5] gdb/testsuite: cleanup in gdb.base/args.exp

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:

Andrew> +	    verbose -log "APB: regexp '$arg'"

Stray addition?

Tom

Re: [PATCHv2 0/5] Fixes for passing arguments to gdbserver

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:

Andrew> Some fixes for passing arguments to gdbserver.

This all looks good to me.  Thanks for working on this.

https://sourceware.org/bugzilla/show_bug.cgi?id=28392
points to this series:
https://sourceware.org/pipermail/gdb-patches/2021-October/182723.html

We really do need a patch tracker... the current situation is not good.

Anyway I wonder how your patches compare.

Approved-By: Tom Tromey <tom@tromey.com>

Tom

Re: [PATCHv2 0/5] Fixes for passing arguments to gdbserver

[Andrew Burgess]

Tom Tromey <tom@tromey.com> writes:

>>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:
>
> Andrew> Some fixes for passing arguments to gdbserver.
>
> This all looks good to me.  Thanks for working on this.
>
> https://sourceware.org/bugzilla/show_bug.cgi?id=28392
> points to this series:
> https://sourceware.org/pipermail/gdb-patches/2021-October/182723.html
>
> We really do need a patch tracker... the current situation is not good.
>
> Anyway I wonder how your patches compare.

So I took a look at this older series, and got it compiling.  It is a
super-set of my changes.

On the whole the older series looks really promising, however, it does
change the vRun remote packet, which I don't think is acceptable as it
was initially suggested[1].

I think this series from me does still offer some value, so I'm going to
go ahead and merge it, though I've acknowledged the author of the original
series with a Co-Authored-By tag, though I had not read that series when
I created my work.

When I get a chance, I'll try to revisit the older series and see what
can be done with it.

[1] All arguments are now passed as a single string within the vRun
rather than a vector of arguments.  I don't think this is going to be
acceptable.  If this really is the only way to solve the quoting problem
then, at a minimum, we'd need a feature flag to indicate this change in
behaviour.


>
> Approved-By: Tom Tromey <tom@tromey.com>
>

I removed the stray 'verbose' output from patch #4, and pushed this
series.

Thanks,
Andrew

Re: [PATCHv2 0/5] Fixes for passing arguments to gdbserver

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:

Andrew> [1] All arguments are now passed as a single string within the vRun
Andrew> rather than a vector of arguments.  I don't think this is going to be
Andrew> acceptable.  If this really is the only way to solve the quoting problem
Andrew> then, at a minimum, we'd need a feature flag to indicate this change in
Andrew> behaviour.

Yeah, I think we should avoid this change if at all possible.

Tom