gdb: remove the !startup_with_shell path from construct_inferior_arguments

In the commit:

  commit 0df62bf
  Date:   Fri Oct 22 07:19:33 2021 +0000

      gdb: Support some escaping of args with startup-with-shell being off

nat/fork-inferior.c was updated such that when we are starting an
inferior without a shell we now remove escape characters.  The
benefits of this are explained in that commit, but having made this
change we can now make an additional change.

Currently, in construct_inferior_arguments, when startup_with_shell is
false we construct the inferior argument string differently than when
startup_with_shell is true; when true we apply some escaping to
special shell character, when false we don't.

This commit simplifies construct_inferior_arguments by removing the
!startup_with_shell case, and instead we now apply escaping in all
cases.  This is fine because, thanks to the above commit the escaping
will be correctly removed again when we call into nat/fork-inferior.c.

We should think of construct_inferior_arguments and
nat/fork-inferior.c as needing to cooperate in order for argument
handling to work correctly.

construct_inferior_arguments converts a list of separate arguments
into a single string, and nat/fork-inferior.c splits that single
string back into a list of arguments.  It is critical that, if
nat/fork-inferior.c is expecting to remove a "layer" of escapes, then
construct_inferior_arguments must add that expected "layer",
otherwise, we end up stripping more escapes than expected.

The great thing (I think) about the new configuration, is that GDB no
longer cares about startup_with_shell at the point the arguments are
being setup.  We only care about startup_with_shell at the point that
the inferior is started.  This means that a user can set the inferior
arguments, and then change the startup-with-shell setting, and GDB
will do what they expect.

Under the previous system, where construct_inferior_arguments changed
its behaviour based on startup_with_shell, the user had to change the
setting, and then set the arguments, otherwise, GDB might not do what
they expect.

There is one slight issue with this commit though, which will be
addressed by the next commit.

For GDB's native targets construct_inferior_arguments is reached via
two code paths; first when GDB starts and we combine arguments from
the command line, and second when the Python API is used to set the
arguments from a sequence.  It's the command line argument handling
which we are interested in.

Consider this:

  $ gdb --args /tmp/exec '$FOO'
  (gdb) show args
  Argument list to give program being debugged when it is started is "\$FOO".

Notice that the argument has become \$FOO, the '$' is now quoted.

This is because, by quoting the argument in the shell command that
started GDB, GDB was passed a literal $FOO with no quotes.  In order
to ensure that the inferior sees this same value, GDB added the extra
escape character.  When GDB starts with a shell we pass \$FOO, which
results in the inferior seeing a literal $FOO.

But what if the user _actually_ wanted to have the shell GDB uses to
start the inferior expand $FOO?  Well, it appears this can't be done
from the command line, but from the GDB prompt we can just do:

  (gdb) set args $FOO
  (gdb) show args
  Argument list to give program being debugged when it is started is "$FOO".

And now the inferior will see the shell expanded version of $FOO.

It might seem like we cannot achieve the same result from the GDB
command line, however, it is possible with this trick:

  $ gdb -eiex 'set startup-with-shell off' --args /tmp/exec '$FOO'
  (gdb) show args
  Argument list to give program being debugged when it is started is "$FOO".
  (gdb) show startup-with-shell
  Use of shell to start subprocesses is off.

And now the $FOO is not escaped, but GDB is no longer using a shell to
start the inferior, however, we can extend our command line like this:

  $ gdb -eiex 'set startup-with-shell off' \
        -ex 'set startup-with-shell on' \
	--args /tmp/exec '$FOO'
  (gdb) show args
  Argument list to give program being debugged when it is started is "$FOO".
  (gdb) show startup-with-shell
  Use of shell to start subprocesses is on.

Use an early-initialisation option to disable startup-with-shell, this
is done before command line argument processing, then a normal
initialisation option turns startup-with-shell back on after GDB has
processed the command line arguments!

Is this useful?  Yes, absolutely.  Is this a good user experience?
Absolutely not.  And I plan to add a new command line option to
GDB (and gdbserver) that will allow users to achieve the same
result (this trick doesn't work in gdbserver as there's no
early-initialisation there) without having to toggle the
startup-with-shell option.  The new option can be found in the series
here:

  https://inbox.sourceware.org/gdb-patches/[email protected]

The problem is that, that series is pretty long, and getting it
reviewed is just not possible.  So instead I'm posting the individual
patches in smaller blocks, to make reviews easier.

So, what's the problem?  Well, by removing the !startup_with_shell
code path from GDB, there is no longer a construct_inferior_arguments
code path that doesn't quote inferior arguments, and so there's no
longer a way, from the command line, to set an unquoted '$FOO' as an
inferior argument.  Obviously, this can still be done from GDB's CLI
prompt.

The trick above is completely untested, so this regression isn't going
to show up in the testsuite.

And the breakage is only temporary.  In the next commit I'll add a fix
which restores the above trick.

Of course, I hope that this fix will itself, only be temporary.  Once
the new command line options that I mentioned above are added, then
the fix I add in the next commit can be removed, and user should start
using the new command line option.

After this commit a whole set of tests that were added as xfail in the
above commit are now passing.

A change similar to this one can be found in this series:

  https://inbox.sourceware.org/gdb-patches/[email protected]/

which I reviewed before writing this patch.  I don't think there's any
one patch in that series that exactly corresponds with this patch
though, so I've listed the author of the original series as co-author
on this patch.

Co-Authored-By: Michael Weghorn <[email protected]>
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28392

Tested-By: Guinevere Larsen <[email protected]>

Author: T-J-Teru

gdb/testsuite/gdb.base/args.exp

@@ -29,15 +29,6 @@ if {[build_executable $testfile.exp $testfile $srcfile] == -1} {
     return -1
 }
 
-set startup_with_shell_modes { "on" }
-if {![gdb_protocol_is_remote]} {
-    lappend startup_with_shell_modes "off"
-} else {
-    # Some of these tests will not work when using the remote protocol
-    # due to bug PR gdb/28392.
-    unsupported "gdbserver 'startup-with-shell off' broken PR gdb/28392"
-}
-
 # NAME is the name to use for the tests and ARGLIST is the list of
 # arguments that are passed to GDB when it is started.
 #
@@ -55,7 +46,7 @@ proc args_test { name arglist {re_list {}} } {
 	set re_list $arglist
     }
 
-    foreach_with_prefix startup_with_shell $::startup_with_shell_modes {
+    foreach_with_prefix startup_with_shell { on off } {
 	save_vars { ::GDBFLAGS } {
 	    set ::GDBFLAGS "$::GDBFLAGS --args $::binfile $arglist"
 

gdb/testsuite/gdb.base/inferior-args.exp

@@ -174,22 +174,48 @@ set bs "\\\\"
 lappend item [list "$hex \"$bs\"\"" "$hex \"$bs$bs$bs\"\""]
 lappend test_desc_list $item
 
-set startup_with_shell_modes { "on" }
-if {![gdb_protocol_is_remote]} {
-    lappend startup_with_shell_modes "off"
-} else {
-    # Due to PR gdb/28392 gdbserver doesn't currently support having
-    # startup-with-shell off, and then attempting to pass arguments
-    # containing whitespace.
-    unsupported "bug gdb/28392: gdbserver doesn't support this"
-}
-
+# test three
+# ----------
+#
+# This test focuses on sending special shell characters within a
+# double quote argument, and each special character is prefixed with a
+# backslash.
+#
+# In a POSIX shell, within a double quoted argument, only $ (dollar),
+# ` (backtick), " (double quote), \ (backslash), and newline can be
+# escaped.  All other backslash characters are literal backslashes.
+#
+# As with the previous test, the double quotes are lost when the
+# arguments are sent through gdbserver_start, as such, this test isn't
+# going to work when using the native-gdbserver board, hence we set
+# the second arguemnt to 'false'.
+lappend test_desc_list [list "test three" \
+			    false \
+			    { "\&" "\<" "\#" "\^" "\>" "\$" "\`" } \
+			    [list "$hex \"\\\\\\\\&\"" \
+				 "$hex \"\\\\\\\\<\"" \
+				 "$hex \"\\\\\\\\#\"" \
+				 "$hex \"\\\\\\\\\\^\"" \
+				 "$hex \"\\\\\\\\>\"" \
+				 "$hex \"\\\$\"" \
+				 "$hex \"`\""]]
+
+# test four
+# ---------
+#
+# This test passes two arguments, a single and double quote, each
+# escaped with a backslash.
+lappend test_desc_list [list "test four" \
+			    true \
+			    { \' \" } \
+			    [list "$hex \"'\"" \
+				 "$hex \"\\\\\"\""]]
 
 foreach desc $test_desc_list {
     lassign $desc name stub_suitable args re_list
     with_test_prefix $name {
 	foreach_with_prefix set_method { "start" "starti" "run" "set args" } {
-	    foreach_with_prefix startup_with_shell $startup_with_shell_modes {
+	    foreach_with_prefix startup_with_shell { on off } {
 		do_test $set_method $startup_with_shell $args $re_list \
 		    $stub_suitable
 	    }

gdb/testsuite/gdb.base/startup-with-shell.exp

@@ -59,12 +59,8 @@ proc initial_setup_simple { startup_with_shell run_args } {
 # If PROBLEMATIC_ON is true then when startup-with-shell is on we
 # expect the comparison to fail, so setup an xfail.
 #
-# If PROBLEMATIC_OFF is true then when startup-with-shell is off we
-# expect the comparison to fail, so setup an xfail.
-#
 # TESTNAME is a string used in the test names.
-proc run_test { args on_re off_re testname { problematic_on false } \
-		    { problematic_off false } } {
+proc run_test { args on_re off_re testname { problematic_on false } } {
     foreach startup_with_shell { "on" "off" } {
 	with_test_prefix "$testname, startup_with_shell: ${startup_with_shell}" {
 	    if {![initial_setup_simple $startup_with_shell $args]} {
@@ -76,7 +72,7 @@ proc run_test { args on_re off_re testname { problematic_on false } \
 		set problematic $problematic_on
 	    } else {
 		set re $off_re
-		set problematic $problematic_off
+		set problematic false
 	    }
 
 	    if { $problematic } {
@@ -91,9 +87,8 @@ proc run_test { args on_re off_re testname { problematic_on false } \
 # This is like the run_test proc except that RE is used as the
 # expected argument regexp when startup-with-shell is both on and off.
 # For the other arguments, see run_test.
-proc run_test_same { args re testname { problematic_on false } \
-			 { problematic_off false } } {
-    run_test $args $re $re $testname $problematic_on $problematic_off
+proc run_test_same { args re testname } {
+    run_test $args $re $re $testname
 }
 
 # The regexp to match a single '\' character.
@@ -129,48 +124,40 @@ save_vars { env(TEST) } {
 
 run_test_same "\"\\a\"" \
     "\"${bs}${bs}a\"" \
-    "retain backslash in double quote arg" \
-    false $is_remote_p
+    "retain backslash in double quote arg"
 
 run_test_same "'\\a'" \
     "\"${bs}${bs}a\"" \
-    "retain backslash in single quote arg" \
-    false $is_remote_p
+    "retain backslash in single quote arg"
 
 run_test_same "\"\\\$\"" \
     "\"\\\$\"" \
     "'\$' can be escaped in double quote arg"
 
 run_test_same "'\\\$'" \
     "\"${bs}${bs}\\\$\"" \
-    "'\$' is not escaped in single quote arg" \
-    false $is_remote_p
+    "'\$' is not escaped in single quote arg"
 
 run_test_same "\"\\`\"" \
     "\"\\`\"" \
     "'`' can be escaped in double quote arg"
 
 run_test_same "'\\`'" \
     "\"${bs}${bs}`\"" \
-    "'`' is not escaped in single quote arg" \
-    false $is_remote_p
+    "'`' is not escaped in single quote arg"
 
 run_test_same "\"\\\"\"" \
     "\"${bs}\"\"" \
-    "'\"' can be escaped in double quote arg" \
-    false $is_remote_p
+    "'\"' can be escaped in double quote arg"
 
 run_test_same "'\\\"'" \
     "\"${bs}${bs}${bs}\"\"" \
-    "'\"' is not escaped in single quote arg" \
-    false $is_remote_p
+    "'\"' is not escaped in single quote arg"
 
 run_test_same "\"\\\\\"" \
     "\"${bs}${bs}\"" \
-    "'\\' can be escaped in double quote arg" \
-    false $is_remote_p
+    "'\\' can be escaped in double quote arg"
 
 run_test_same "'\\\\'" \
     "\"${bs}${bs}${bs}${bs}\"" \
-    "'\\' is not escaped in single quote arg" \
-    false $is_remote_p
+    "'\\' is not escaped in single quote arg"

gdbsupport/common-inferior.cc

@@ -31,92 +31,66 @@ construct_inferior_arguments (gdb::array_view<char * const> argv)
 {
   std::string result;
 
-  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 = '"';
+  /* 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 = '\'';
+  /* 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
-      for (int i = 0; i < argv.size (); ++i)
+  for (int i = 0; i < argv.size (); ++i)
+    {
+      if (i > 0)
+	result += ' ';
+
+      /* Need to handle empty arguments specially.  */
+      if (argv[i][0] == '\0')
 	{
-	  if (i > 0)
-	    result += ' ';
+	  result += quote;
+	  result += quote;
+	}
+      else
+	{
+#ifdef __MINGW32__
+	  bool quoted = false;
 
-	  /* Need to handle empty arguments specially.  */
-	  if (argv[i][0] == '\0')
+	  if (strpbrk (argv[i], special))
 	    {
-	      result += quote;
+	      quoted = true;
 	      result += quote;
 	    }
-	  else
+#endif
+	  for (char *cp = argv[i]; *cp != '\0'; ++cp)
 	    {
-#ifdef __MINGW32__
-	      bool quoted = false;
-
-	      if (strpbrk (argv[i], special))
+	      if (*cp == '\n')
 		{
-		  quoted = true;
+		  /* A newline cannot be quoted with a backslash (it
+		     just disappears), only by putting it inside
+		     quotes.  */
+		  result += quote;
+		  result += '\n';
 		  result += quote;
 		}
-#endif
-	      for (char *cp = argv[i]; *cp; ++cp)
+	      else
 		{
-		  if (*cp == '\n')
-		    {
-		      /* A newline cannot be quoted with a backslash (it
-			 just disappears), only by putting it inside
-			 quotes.  */
-		      result += quote;
-		      result += '\n';
-		      result += quote;
-		    }
-		  else
-		    {
 #ifdef __MINGW32__
-		      if (*cp == quote)
+		  if (*cp == quote)
 #else
-		      if (strchr (special, *cp) != NULL)
+		    if (strchr (special, *cp) != NULL)
 #endif
-			result += '\\';
-		      result += *cp;
-		    }
+		      result += '\\';
+		  result += *cp;
 		}
+	    }
 #ifdef __MINGW32__
-	      if (quoted)
-		result += quote;
+	  if (quoted)
+	    result += quote;
 #endif
-	    }
-	}
-    }
-  else
-    {
-      /* In this case we can't handle arguments that contain spaces,
-	 tabs, or newlines -- see breakup_args().  */
-      for (char *arg : argv)
-	{
-	  char *cp = strchr (arg, ' ');
-	  if (cp == NULL)
-	    cp = strchr (arg, '\t');
-	  if (cp == NULL)
-	    cp = strchr (arg, '\n');
-	  if (cp != NULL)
-	    error (_("can't handle command-line "
-		     "argument containing whitespace"));
-	}
-
-      for (int i = 0; i < argv.size (); ++i)
-	{
-	  if (i > 0)
-	    result += " ";
-	  result += argv[i];
 	}
     }