gdb/gdbserver: rework argument splitting and joining

diff --git a/gdb/NEWS b/gdb/NEWS
index 7c8cf9af4c2..7f956fb4b5c 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -83,6 +83,14 @@ 
 
 * Support for native Thread Local Storage (TLS) variables on Windows.
 
+* There have been changes to how inferior arguments are passed to
+  remote targets.  When using the latest GDB and gdbserver the new
+  single-inf-arg feature will be used for argument passing.  When
+  using remote targets that don't support single-inf-arg, GDB will now
+  escape special shell characters (except for whitespace and quotes)
+  within arguments before passing them to the remote target.  Remote
+  targets will need updating to take this into account.
+
 * Configure changes
 
 ** --with-babeltrace has been removed.  The babeltrace library was
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 9de2c7a299e..9eabd970805 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -44065,6 +44065,19 @@  Packets
 @var{argument} string, which includes all inferior arguments,
 separated with whitespace.
 
+If @value{GDBN} didn't send @samp{single-inf-arg}, or the remote
+didn't choose to use this feature, then @value{GDBN} will split the
+inferior arguments before passing them to the remote target.  Single
+and double quotes surrounding any argument will have been removed by
+@value{GDBN}.  Any special shell characters within each argument will
+have been escaped with a backslash character except for quote and
+whitespace characters, these will not be escaped within the arguments,
+the remote target is expected to escape these characters itself.
+
+These slightly complex rules exist for backward compatibility reasons.
+When implementing a new remote target, it is suggested that the
+@samp{single-inf-arg} feature be supported.
+
 @c FIXME:  What about non-stop mode?
 
 This packet is only available in extended mode (@pxref{extended mode}).
diff --git a/gdb/testsuite/gdb.base/args.exp b/gdb/testsuite/gdb.base/args.exp
index d2d5a601b1e..51326283405 100644
--- a/gdb/testsuite/gdb.base/args.exp
+++ b/gdb/testsuite/gdb.base/args.exp
@@ -30,37 +30,6 @@  if {[build_executable $testfile.exp $testfile $srcfile] == -1} {
     return
 }
 
-# Assuming a running GDB.  Check the status of the single inferior
-# argument feature.  When this feature is on GDB passes inferior
-# arguments as a single combined string.  When this feature is off GDB
-# will split the inferior arguments into multiple separate strings.
-#
-# Return true when arguments are being split, or false when a single
-# combined string is being sent.
-
-proc is_argument_splitting_on {} {
-    set arg_splitting true
-
-    gdb_test_multiple "show remote single-inferior-argument-feature-packet" "" {
-	-re -wrap "Support for the 'single-inferior-argument-feature' packet on the current remote target is \"(on|off)\"\\.(.*)" {
-	    set value $expect_out(1,string)
-	    pass $gdb_test_name
-	    if { $value eq "on" } {
-		set arg_splitting false
-	    }
-	}
-	-re -wrap "Support for the 'single-inferior-argument-feature' packet on the current remote target is \"auto\", currently (enabled|disabled)\\.(.*)" {
-	    set value $expect_out(1,string)
-	    pass $gdb_test_name
-	    if { $value eq "enabled" } {
-		set arg_splitting false
-	    }
-	}
-    }
-
-    return $arg_splitting
-}
-
 # 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.
 #
@@ -127,20 +96,6 @@  proc args_test { name arglist {re_esc_list {}} {re_no_esc_list {}} } {
 			set arg {\\\"}
 		    }
 
-		    # If we are starting with a shell, we're not escaping
-		    # special shell characters in arguments passed to GDB,
-		    # we're using a remote target, and the arguments contain
-		    # a shell variable (indicated with a '$'), then this
-		    # test will currently fail if we are splitting the
-		    # arguments.
-		    if { $startup_with_shell eq "on"
-			 && $arg_flag eq "no-escape-args"
-			 && [gdb_protocol_is_remote]
-			 && [string first "\$" $arglist] != -1
-			 && [is_argument_splitting_on] } {
-			setup_xfail "*-*-*" gdb/28392
-		    }
-
 		    gdb_test "print argv\[$i\]" \
 			"${::valnum_re} = $::hex \"$arg\"" \
 			"argv\[$i\] for $name"
diff --git a/gdb/testsuite/gdb.base/startup-with-shell.exp b/gdb/testsuite/gdb.base/startup-with-shell.exp
index 6f30841bdb8..ac5f35247e5 100644
--- a/gdb/testsuite/gdb.base/startup-with-shell.exp
+++ b/gdb/testsuite/gdb.base/startup-with-shell.exp
@@ -58,11 +58,8 @@  proc initial_setup_simple { startup_with_shell run_args } {
 # it to ON_RE if startup-with-shell is on, otherwise compare to
 # OFF_RE.
 #
-# If PROBLEMATIC_ON is true then when startup-with-shell is on 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 } } {
+proc run_test { args on_re off_re testname } {
     foreach startup_with_shell { "on" "off" } {
 	with_test_prefix "$testname, startup_with_shell: ${startup_with_shell}" {
 	    if {![initial_setup_simple $startup_with_shell $args]} {
@@ -71,14 +68,8 @@  proc run_test { args on_re off_re testname { problematic_on false } } {
 
 	    if { $startup_with_shell } {
 		set re $on_re
-		set problematic $problematic_on
 	    } else {
 		set re $off_re
-		set problematic false
-	    }
-
-	    if { $problematic } {
-		setup_xfail "*-*-*" gdb/28392
 	    }
 
 	    gdb_test "print argv\[1\]" "${::valnum_re} = $::hex $re" $testname
@@ -94,15 +85,14 @@  proc run_test_same { args re testname } {
 }
 
 # Run the actual tests
-proc run_all_tests { { is_remote_with_split_args false } } {
+proc run_all_tests { } {
     # The regexp to match a single '\' character.
     set bs "\\\\"
 
     run_test "$::unique_file_dir/*.unique-extension" \
 	"\"$::unique_file\"" \
 	"\"$::unique_file_dir/\\\*\.unique-extension\"" \
-	"arg is glob" \
-	$is_remote_with_split_args
+	"arg is glob"
 
     run_test_same "$::unique_file_dir/\\*.unique-extension" \
 	"\"$::unique_file_dir/\\\*\.unique-extension\"" \
@@ -113,8 +103,7 @@  proc run_all_tests { { is_remote_with_split_args false } } {
 	run_test "\$TEST" \
 	    "\"1234\"" \
 	    "\"\\\$TEST\"" \
-	    "arg is shell variable" \
-	    $is_remote_with_split_args
+	    "arg is shell variable"
 
 	run_test_same "\\\$TEST" \
 	    "\"\\\$TEST\"" \
@@ -124,14 +113,12 @@  proc run_all_tests { { is_remote_with_split_args false } } {
     run_test "\$(echo foo)" \
 	"\"foo\"" \
 	"\"\\\$\\(echo\"" \
-	"arg is parameter expansion, command execution" \
-	$is_remote_with_split_args
+	"arg is parameter expansion, command execution"
 
     run_test "\$((2 + 3))" \
 	"\"5\"" \
 	"\"\\\$\\(\\(2\"" \
-	"arg is parameter expansion, expression evaluation" \
-	$is_remote_with_split_args
+	"arg is parameter expansion, expression evaluation"
 
     run_test_same "\"\\a\"" \
 	"\"${bs}${bs}a\"" \
@@ -183,7 +170,7 @@  if {[target_info gdb_protocol] == "extended-remote"} {
     with_test_prefix "single-inferior-arg disabled" {
 	save_vars { GDBFLAGS } {
 	    append GDBFLAGS " -ex \"set remote single-inferior-argument-feature-packet off\""
-	    run_all_tests true
+	    run_all_tests
 	}
     }
 }
diff --git a/gdb/unittests/remote-arg-selftests.c b/gdb/unittests/remote-arg-selftests.c
index 7d4e63deefd..c4efaab5382 100644
--- a/gdb/unittests/remote-arg-selftests.c
+++ b/gdb/unittests/remote-arg-selftests.c
@@ -40,22 +40,36 @@  struct arg_test_desc
 
 /* The list of tests.  */
 arg_test_desc desc[] = {
+  { "", {}, "" },
   { "abc", { "abc" }, "abc" },
   { "a b c", { "a", "b", "c" }, "a b c" },
   { "\"a b\" 'c d'", { "a b", "c d" }, "a\\ b c\\ d" },
   { "\\' \\\"", { "'", "\"" }, "\\' \\\"" },
-  { "'\\'", { "\\" }, "\\\\" },
-  { "\"\\\\\" \"\\\\\\\"\"", { "\\", "\\\"" }, "\\\\ \\\\\\\"" },
+  { "'\\'", { "\\\\" }, "\\\\" },
+  { "\"\\\\\" \"\\\\\\\"\"", { "\\\\", "\\\\\"" }, "\\\\ \\\\\\\"" },
   { "\\  \" \" ' '", { " ", " ", " "}, "\\  \\  \\ " },
   { "\"'\"", { "'" }, "\\'" },
-  { "'\"' '\\\"'", { "\"", "\\\"" } , "\\\" \\\\\\\""},
+  { "'\"' '\\\"'", { "\"", "\\\\\"" } , "\\\" \\\\\\\""},
   { "\"first arg\" \"\" \"third-arg\" \"'\" \"\\\"\" \"\\\\\\\"\" \" \" \"\"",
-    { "first arg", "", "third-arg", "'", "\"", "\\\""," ", "" },
+    { "first arg", "", "third-arg", "'", "\"", "\\\\\""," ", "" },
     "first\\ arg '' third-arg \\' \\\" \\\\\\\" \\  ''"},
   { "\"\\a\" \"\\&\" \"\\#\" \"\\<\" \"\\^\"",
-    { "\\a", "\\&", "\\#" , "\\<" , "\\^"},
+    { "\\\\a", "\\\\\\&", "\\\\\\#" , "\\\\\\<" , "\\\\\\^"},
+    "\\\\a \\\\\\& \\\\\\# \\\\\\< \\\\\\^" },
+  /* The following entry produces the same split output as the previous
+     one: inside double quotes, \x where x is not a special character, is
+     considered a literal backslash followed by a literal 'x', equivalent
+     to the explicitly escaped form \\x.  */
+  { "\"\\\\a\" \"\\\\&\" \"\\\\#\" \"\\\\<\" \"\\\\^\"",
+    { "\\\\a", "\\\\\\&", "\\\\\\#" , "\\\\\\<" , "\\\\\\^"},
     "\\\\a \\\\\\& \\\\\\# \\\\\\< \\\\\\^" },
   { "1 '\n' 3", { "1", "\n", "3" }, "1 '\n' 3" },
+  { "abc* abc\\* abc\\\\*", { "abc*", "abc\\*", "abc\\\\*" },
+    "abc* abc\\* abc\\\\*" },
+  { "\"ab\\ cd\" \"ef\\'gh\"", { "ab\\\\ cd", "ef\\\\'gh" },
+    "ab\\\\\\ cd ef\\\\\\'gh" },
+  { "\"$VAR\" \"ab`foo`cd\"", { "$VAR", "ab`foo`cd" }, "$VAR ab`foo`cd" },
+  { "'$VAR'", { "\\$VAR" }, "\\$VAR" },
 };
 
 /* Run the remote argument passing self tests.  */
diff --git a/gdbsupport/remote-args.cc b/gdbsupport/remote-args.cc
index 2abf61aa86b..8e26fbe0128 100644
--- a/gdbsupport/remote-args.cc
+++ b/gdbsupport/remote-args.cc
@@ -19,16 +19,235 @@ 
 #include "gdbsupport/common-inferior.h"
 #include "gdbsupport/buildargv.h"
 
+/* This file contains the function used for splitting an argument string
+   into separate arguments in preparation for sending the arguments over
+   the remote protocol, as well as the function to merge the separate
+   arguments back together into a single argument string.
+
+   The logic within these functions is slightly more complex than it should
+   be.  This is in order to maintain some level of backward compatibility.
+
+   In the following text examples of command line arguments will be given.
+   To avoid confusion, arguments, and argument strings, will be delimited
+   with '(' and ')', the parenthesis are not part of the argument itself.
+   This is clearer than using quotes, as some of the examples will include
+   quotes within the arguments.
+
+   Historically, the algorithm used to split the argument string into
+   separate arguments removed a level of quoting from the arguments.  For
+   example consider the following argument string: (abc* abc\*).  The
+   historic algorithm would split this into (abc*) and (abc*).  Notice that
+   the two arguments are identical.  On the remote end we are now destined
+   for failure, either we apply an escape to both '*' characters, or we
+   apply an escape to neither.  In either case, we get one of the arguments
+   wrong.  The historic approach was just broken.
+
+   However, the historic approach has been in place for many years.
+   Clearly not all arguments were corrupted in the manner described above,
+   so lots of things did work.  For example, the string: ("ab cd" "ef")
+   will be split into (ab cd) and (ef).  And the string ('"') will become
+   just (").
+
+   What we can observe from all of these examples, is that the historic
+   approach at the remote end was to simply apply an escape to every
+   special shell character, quotes, white space, as well as every other
+   special character (e.g. (*)).  The problem with this approach is that
+   sometimes special shell characters shouldn't be escaped.
+
+   If we could start from scratch, then the simple approach would be to
+   retain all escaping while splitting the argument string, and, where
+   quotes are used, convert this into backslash escaping as needed.  Thus
+   the argument string ("ab cd" "ef") would become (ab\ cd) and (ef).  And
+   the argument string (abc* abc\*) would become (abc*) (abc\*).  On the
+   remote end, joining these arguments is as simple as concatenation with a
+   single space character between.
+
+   However, if we change to this approach now, then consider ("ab cd").
+   Previously this was sent as (ab cd), but now it would be (ab\ cd).  This
+   breaks backward compatibility.  Remember, we need to think about newer
+   GDB being used with older gdbserver, or newer GDB being used with
+   targets that are not gdbserver at all.
+
+   And so, this is where the complexity comes in.
+
+   The strategy here is to split the arguments, removing all double and
+   single quotes.  While removing quotes, special shell characters are
+   escaped as needed.  But, white space characters, and quote characters
+   are not escaped.  These characters must always be escaped, and so we can
+   safely drop the escape in these cases, this provides some degree of
+   backward compatibility, but is still a breaking change from the old
+   scheme.  */
+
+/* Return true if C is a double or single quote character.  */
+
+static bool
+isquote (const char c)
+{
+  return c == '"' || c == '\'';
+}
+
 /* See remote-args.h.  */
 
 std::vector<std::string>
 gdb::remote_args::split (const std::string &args)
 {
-  std::vector<std::string> results;
+  std::vector<std::string> remote_args;
 
-  gdb_argv argv (args.c_str ());
-  for (int i = 0; argv[i] != nullptr; i++)
-    results.emplace_back (argv[i]);
+  const char *input = args.c_str ();
+  bool squote = false, dquote = false;
 
-  return results;
+#ifdef __MINGW32__
+  /* This holds all the characters considered special to the
+     Windows shells.  */
+  static const char special[] = "\"!&*|[]{}<>?`~^=;, \t\n";
+#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";
+#endif
+
+  /* Characters that are special within double quotes.  */
+  static const char dquote_special[] = "$`\\";
+
+  input = skip_spaces (input);
+
+  while (*input != '\0')
+    {
+      std::string arg;
+
+      while (*input != '\0')
+	{
+	  if (isspace (*input) && !squote && !dquote)
+	    break;
+	  else if (*input == '\\' && !squote)
+	    {
+	      if (input[1] == '\0')
+		arg += input[0];
+	      else if (input[1] == '\n')
+		++input;
+	      else if (dquote && input[1] == '"')
+		{
+		  arg += input[1];
+		  ++input;
+		}
+	      else if (dquote && strchr (dquote_special, input[1]) != nullptr)
+		{
+		  /* Within double quotes, these characters have special
+		     meaning.  If they are escaped with a backslash then we
+		     need to preserve the escape once we remove the
+		     quotes.  */
+		  arg += input[0];	/* Backslash.  */
+		  arg += input[1];	/* Special character.  */
+		  ++input;
+		}
+	      else if (dquote)
+		{
+		  /* Within double quotes, none of the remaining characters
+		     have any special meaning, the backslash before the
+		     character is a literal backslash.
+
+		     To retain the literal backslash with the quotes
+		     removed, we need to escape the backslash.  */
+		  arg += input[0];	/* Backslash.  */
+		  arg += input[0];	/* Backslash.  */
+
+		  /* If the character after the backslash has special
+		     meaning outside of the double quotes, then we need to
+		     escape it now, otherwise it will gain special meaning
+		     as we remove the surrounding quotes.  However, as per
+		     the comments at the head of this file; we don't
+		     escape white space or quotes.  */
+		  if (!isspace (input[1])
+		      && !isquote (input[1])
+		      && strchr (special, input[1]) != nullptr)
+		    arg += input[0];	/* Backslash.  */
+
+		  arg += input[1];	/* Character.  */
+		  ++input;
+		}
+	      else if (isspace (input[1]) || isquote (input[1]))
+		{
+		  /* We remove the escaping from white space and quote
+		     characters.  */
+		  arg += input[1];	/* Character.  */
+		  ++input;
+		}
+	      else
+		{
+		  /* For everything else, retain the escaping.  */
+		  arg += input[0];	/* Backslash.  */
+		  arg += input[1];	/* Character.  */
+		  ++input;
+		}
+	    }
+	  else if (squote)
+	    {
+	      /* Inside a single quote argument there are no special
+		 characters.  A single quote finishes the argument.  */
+	      if (*input == '\'')
+		squote = false;
+	      /* Don't add escaping to white space or quotes.  We already
+		 handled single quotes above, so the isquote call here will
+		 only find double quotes.  */
+	      else if (isspace (*input) || isquote (*input))
+		arg += *input;
+	      /* Any other special shell character needs a backslash adding
+		 to avoid the character gaining special meaning outside of
+		 the single quotes.  */
+	      else if (strchr (special, *input) != nullptr)
+		{
+		  arg += '\\';
+		  arg += *input;
+		}
+	      /* Any other character just gets added to the output.  */
+	      else
+		arg += *input;
+	    }
+	  else if (dquote)
+	    {
+	      /* Inside a double quoted argument.  A double quote closes
+		 the argument.  An escaped double quote will have been
+		 handled in the backslash handling block above.  */
+	      if (*input == '"')
+		dquote = false;
+	      /* Don't add escaping for white space or quotes.  We already
+		 handled double quotes above, so the isquote call here will
+		 only find single quotes.  */
+	      else if (isspace (*input) || isquote (*input))
+		arg += *input;
+	      /* Any character that is not one of the few characters that
+		 retains its special meaning without double quotes, but is
+		 otherwise a special character needs an escape character
+		 adding, to avoid the character gaining special meaning
+		 outside of the quotes.  */
+	      else if (strchr (dquote_special, *input) == nullptr
+		       && strchr (special, *input) != nullptr)
+		{
+		  arg += '\\';
+		  arg += *input;
+		}
+	      /* Anything else just gets passed through to the output.  */
+	      else
+		arg += *input;
+	    }
+	  else
+	    {
+	      /* Found a character outside of a single or double quoted
+		 argument, and not preceded by a backslash.  */
+	      if (*input == '\'')
+		squote = true;
+	      else if (*input == '"')
+		dquote = true;
+	      else
+		arg += *input;
+	    }
+	  ++input;
+	}
+
+      remote_args.push_back (std::move (arg));
+      input = skip_spaces (input);
+    }
+
+  return remote_args;
 }
diff --git a/gdbsupport/remote-args.h b/gdbsupport/remote-args.h
index d714702a9b8..b39eb20eeb7 100644
--- a/gdbsupport/remote-args.h
+++ b/gdbsupport/remote-args.h
@@ -62,7 +62,7 @@  template<typename T>
 inline std::string
 join (const std::vector<T> &args)
 {
-  return construct_inferior_arguments (gdb::array_view<const T> (args), true);
+  return construct_inferior_arguments (gdb::array_view<const T> (args), false);
 }
 
 } /* namespace remote_args */