[PATCHv2 04/10] gdb/gdbserver: add new qDefaultExecAndArgs packet

[Andrew Burgess <aburgess@redhat.com>]

This commit adds a new remote protocol packet qDefaultExecAndArgs, and
updates GDB to use it.

When gdbserver is started a user can provide an executable and
arguments, these are used (by the remote target) to start an initial
inferior, this is the inferior to which GDB first connects.

When GDB is connected in extended-remote mode, if the user does a
'run' without specifying a new 'remote exec-file' then the executable
given on the gdbserver command line is reused to start the new
inferior.

Interestingly, the arguments given on the gdbserver command line are
only used when starting the first inferior, subsequent inferiors will
be passed an empty argument string by GDB.  This might catch out a
user, causing the rerun to behave differently than the first run.

In this commit I will add a new qDefaultExecAndArgs packet, which I
think will improve the experience in this area.

The new qDefaultExecAndArgs packet is sent from GDB, and gdbserver
replies with a packet that includes the executable filename and the
argument string that were used for starting the initial inferior.

On the GDB side this information can be used to update GDB's state,
the 'show remote exec-file' will reflect how gdbserver was started,
and 'show args' will reflect the arguments used for starting the
inferior.

As a result of updating the args, if the user restarts the inferior,
then this same argument string will be passed back to the remote
target, and used for the new inferior.  Thus, rerunning the inferior
will behave just like the initial inferior, which I think is a good
improvement.

Finally, GDB will warn if the user has 'set remote exec-file' and
then connects to a gdbserver that was started with some alternative
filename, like this:

  (gdb) set remote exec-file /tmp/foo
  (gdb) target remote | gdbserver --once - /tmp/bar
  ... snip ...
  warning: updating 'remote exec-file' to '/tmp/bar' to match remote target
  ... snip ...

I made the choice to have GDB update the remote exec-file setting to
match the remote, as, after the 'target remote', we are connected to
an inferior that is running /tmp/bar (in this case), so trying to hang
onto the non-matching user supplied setting doesn't seem helpful.

There is one case where I can see this choice being a problem, if a
user does:

  (gdb) set remote exec-file /tmp/foo
  (gdb) target extended-remote | gdbserver --once - /tmp/bar
  ... snip ...
  warning: updating 'remote exec-file' to '/tmp/bar' to match remote target
  ... snip ...
  (gdb) run

In this case, prior to this patch, they would 'run' /tmp/foo, while
after this patch, they will run /tmp/bar.  I think it is unfortunate
that I'm breaking this use case, but, I'm not _that_ sorry -- just
start the gdbserver with the correct executable and the problem goes
away.

Reviewed-By: Eli Zaretskii <eliz@gnu.org>
---
 gdb/NEWS                                      |  15 ++
 gdb/doc/gdb.texinfo                           |  30 +++
 gdb/remote.c                                  | 152 ++++++++++++-
 .../gdb.server/fetch-exec-and-args.c          |  34 +++
 .../gdb.server/fetch-exec-and-args.exp        | 207 ++++++++++++++++++
 gdbserver/server.cc                           |  33 +++
 6 files changed, 470 insertions(+), 1 deletion(-)
 create mode 100644 gdb/testsuite/gdb.server/fetch-exec-and-args.c
 create mode 100644 gdb/testsuite/gdb.server/fetch-exec-and-args.exp

diff --git a/gdb/NEWS b/gdb/NEWS
index c4b1f7a7e3b..d78929c1398 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -105,6 +105,14 @@
   'inferior' keyword with either the 'thread' or 'task' keywords when
   creating a breakpoint.
 
+* When connecting to a remote server, if the server supports the
+  qDefaultExecAndArgs packet, then GDB will copy the argument string
+  from the server and update the 'args' setting, as if 'set args ...'
+  had been used.  This means that the arguments are visible from GDB
+  using 'show args', and that, if using the extended-remote protocol,
+  subsequent runs of the inferior will use the same arguments as the
+  first run.
+
 * New commands
 
 set debug breakpoint on|off
@@ -278,6 +286,13 @@ info main
      inferior specific, then this field contains None.  This field can
      be written too.
 
+* New remote packets
+
+qDefaultExecAndArgs
+  This packet returns the executable filename and argument string with
+  which the server was started.  If no such information was given to
+  the server then this is reflected in the reply.
+
 *** Changes in GDB 13
 
 * MI version 1 is deprecated, and will be removed in GDB 14.
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 8be9725d1a2..fa062e25bca 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -44782,6 +44782,36 @@
 A badly formed request or an error was encountered.
 @end table
 
+@cindex query executable, remote request
+@cindex query program arguments, remote request
+@cindex @samp{qDefaultExecAndArgs} packet
+@item qDefaultExecAndArgs
+Return the program filename and argument string with which the remote
+server was started, if the remote server was started with such things.
+If the remote server was started without the filename of a program to
+execute, or without any arguments, then the reply indicates this.
+
+Reply:
+@table @samp
+@item U
+The program filename and arguments are not set.  If @var{GDBN} wants
+to start a new inferior, for example with @samp{vRun}, then it will
+need to provide the program filename to use.
+
+@item S;@var{PP@dots{}};@var{AA@dots{}}
+The program filename provided to the remote server when it started was
+@var{PP@dots{}}, which is a hex encoded string, and the argument
+string passed to the program when started by the server was
+@var{AA@dots{}}, which is also a hex encoded string.
+
+It is valid for either, or both, of @var{PP@dots{}} and
+@var{AA@dots{}} to be the empty string.
+
+@item E @var{NN}
+@itemx E.errtext
+Indicates an error was encountered.
+@end table
+
 @item Qbtrace:bts
 Enable branch tracing for the current thread using Branch Trace Store.
 
diff --git a/gdb/remote.c b/gdb/remote.c
index 1d40e3cf746..ae3fcdc0bb3 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -79,6 +79,7 @@
 #include <unordered_map>
 #include "async-event.h"
 #include "gdbsupport/selftest.h"
+#include "cli/cli-style.h"
 
 /* The remote target.  */
 
@@ -302,6 +303,9 @@ enum {
      packets and the tag violation stop replies.  */
   PACKET_memory_tagging_feature,
 
+  /* Support the qDefaultExecAndArgs packet.  */
+  PACKET_qDefaultExecAndArgs,
+
   PACKET_MAX
 };
 
@@ -691,6 +695,79 @@ struct remote_features
   packet_config m_protocol_packets[PACKET_MAX];
 };
 
+/* Data structure used to hold the results of the qDefaultExecAndArgs
+   packet.  */
+
+struct remote_exec_and_args_info
+{
+  /* The result state reflects whether the packet is supported by this
+     remote target, and then which bits of state the remote target actually
+     returned to GDB .  */
+  enum class state
+  {
+    /* The remote does not support the qDefaultExecAndArgs packet.  GDB
+       should not make assumptions about the remote target's executable
+       and arguments.  */
+    UNKNOWN,
+
+    /* The remote does understand the qDefaultExecAndArgs, no executable
+       (and/or arguments) were set at the remote end.  If GDB wants the
+       remote to start an inferior it will need to provide this information.  */
+    UNSET,
+
+    /* The remote does understand the qDefaultExecAndArgs, an executable
+       and/or arguments were set at the remote end and this information is
+       held within this object.  */
+    SET
+  };
+
+  /* Create an empty instance, STATE should be state::UNKNOWN or
+     state::UNSET only.  */
+  remote_exec_and_args_info (state state = state::UNKNOWN)
+    : m_state (state)
+  {
+    gdb_assert (m_state != state::SET);
+  }
+
+  /* Create an instance in state::SET, move EXEC and ARGS into this
+     instance.  */
+  remote_exec_and_args_info (std::string &&exec, std::string &&args)
+    : m_state (state::SET),
+      m_exec (exec),
+      m_args (args)
+  { /* Nothing.  */ }
+
+  /* Is this object in state::SET?  */
+  bool is_set () const
+  {
+    return m_state == state::SET;
+  }
+
+  /* Return the argument string.  Only call when is_set returns true.  */
+  const std::string &args () const
+  {
+    gdb_assert (m_state == state::SET);
+    return m_args;
+  }
+
+  /* Return the executable string.  Only call when is_set returns true.  */
+  const std::string &exec () const
+  {
+    gdb_assert (m_state == state::SET);
+    return m_exec;
+  }
+
+private:
+  /* The state of this instance.  */
+  state m_state = state::UNKNOWN;
+
+  /* The executable path returned from the remote target.  */
+  std::string m_exec;
+
+  /* The argument string returned from the remote target.  */
+  std::string m_args;
+};
+
 class remote_target : public process_stratum_target
 {
 public:
@@ -1255,6 +1332,9 @@ class remote_target : public process_stratum_target
 
 private:
 
+  /* Fetch the executable filename and argument string from the remote.  */
+  remote_exec_and_args_info fetch_default_executable_and_arguments ();
+
   bool start_remote_1 (int from_tty, int extended_p);
 
   /* The remote state.  Don't reference this directly.  Use the
@@ -1707,7 +1787,7 @@ show_remote_exec_file (struct ui_file *file, int from_tty,
 {
   const std::string &filename = get_remote_exec_file ();
   if (filename.empty ())
-    gdb_printf (file, _("The remote exec-file is unset, the default remote "
+    gdb_printf (file, _("The remote exec-file is \"\", the default remote "
 			"executable will be used.\n"));
   else
     gdb_printf (file, "The remote exec-file is \"%s\".\n", filename.c_str ());
@@ -4913,6 +4993,55 @@ struct scoped_mark_target_starting
   scoped_restore_tmpl<bool> m_restore_starting_up;
 };
 
+/* See declaration in class above.   */
+
+remote_exec_and_args_info
+remote_target::fetch_default_executable_and_arguments ()
+{
+  if (m_features.packet_support (PACKET_qDefaultExecAndArgs) == PACKET_DISABLE)
+    return {};
+
+  struct remote_state *rs = get_remote_state ();
+
+  putpkt ("qDefaultExecAndArgs");
+  getpkt (&rs->buf, 0);
+
+  auto packet_result
+    = m_features.packet_ok (rs->buf, PACKET_qDefaultExecAndArgs);
+  if (packet_result == PACKET_UNKNOWN)
+    return {};
+
+  if (packet_result == PACKET_ERROR)
+    {
+      warning (_("Remote error: %s"), rs->buf.data ());
+      return {};
+    }
+
+  /* First character should be 'U', to indicate no information is set in
+     the server, or 'S' followed by the filename and arguments.  We treat
+     anything that is not a 'S' as if it were 'U'.  */
+  if (rs->buf[0] != 'S')
+    return { remote_exec_and_args_info::state::UNSET };
+
+  if (rs->buf[1] != ';')
+    {
+      warning (_("missing first ';' in qDefaultExecAndArgs reply"));
+      return { remote_exec_and_args_info::state::UNSET };
+    }
+
+  const char *p = &rs->buf[2];
+  const char *e = p;
+  for (; *e != ';' && *e != '\0'; ++e)
+    ;
+  std::string filename = hex2str (p, (e - p) / 2);
+
+  if (*e == ';')
+    ++e;
+  std::string args = hex2str (e, strlen (e) / 2);
+
+  return { std::move (filename), std::move (args) };
+}
+
 /* Helper for remote_target::start_remote, start the remote connection and
    sync state.  Return true if everything goes OK, otherwise, return false.
    This function exists so that the scoped_restore created within it will
@@ -4997,6 +5126,24 @@ remote_target::start_remote_1 (int from_tty, int extended_p)
 	rs->noack_mode = 1;
     }
 
+  auto exec_and_args = fetch_default_executable_and_arguments ();
+
+  /* Update the inferior with the executable and argument string from the
+     target, these will be used when restarting the inferior, and also
+     allow the user to view this state.  */
+  if (exec_and_args.is_set ())
+    {
+      current_inferior ()->set_args (exec_and_args.args ());
+      const std::string &remote_exec = get_remote_exec_file ();
+      if (!remote_exec.empty () && remote_exec != exec_and_args.exec ())
+	warning (_("updating 'remote exec-file' to '%ps' to match "
+		   "remote target"),
+		 styled_string (file_name_style.style (),
+				exec_and_args.exec ().c_str ()));
+      set_pspace_remote_exec_file (current_program_space,
+				   exec_and_args.exec ());
+    }
+
   if (extended_p)
     {
       /* Tell the remote that we are using the extended protocol.  */
@@ -15414,6 +15561,9 @@ Show the maximum size of the address (in bits) in a memory packet."), NULL,
   add_packet_config_cmd (PACKET_memory_tagging_feature,
 			 "memory-tagging-feature", "memory-tagging-feature", 0);
 
+  add_packet_config_cmd (PACKET_qDefaultExecAndArgs, "qDefaultExecAndArgs",
+			 "fetch-exec-and-args", 0);
+
   /* Assert that we've registered "set remote foo-packet" commands
      for all packet configs.  */
   {
diff --git a/gdb/testsuite/gdb.server/fetch-exec-and-args.c b/gdb/testsuite/gdb.server/fetch-exec-and-args.c
new file mode 100644
index 00000000000..f48c8b4c6d3
--- /dev/null
+++ b/gdb/testsuite/gdb.server/fetch-exec-and-args.c
@@ -0,0 +1,34 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2023 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/>.  */
+
+/* Simple test, do some work with the arguments so GDB has a chance to
+   break and check that the arguments are correct.  */
+
+volatile int global_counter;
+
+int
+main (int argc, char *argv[])
+{
+  int i;
+
+  global_counter = 0;		/* Break here.  */
+
+  for (i = 0; i < argc; ++i)
+    argv[i] = (char *) 0;
+
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.server/fetch-exec-and-args.exp b/gdb/testsuite/gdb.server/fetch-exec-and-args.exp
new file mode 100644
index 00000000000..22c22d0c0b2
--- /dev/null
+++ b/gdb/testsuite/gdb.server/fetch-exec-and-args.exp
@@ -0,0 +1,207 @@
+# This testcase is part of GDB, the GNU debugger.
+
+# Copyright 2023 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 the qDefaultExecAndArgs packet, specifically, the argument
+# fetching component of the packet.
+
+# Skip test if target does not support argument passing.
+require {!target_info exists noargs}
+
+load_lib gdbserver-support.exp
+
+standard_testfile .c
+
+require allow_gdbserver_tests
+
+if {[build_executable "failed to build" $binfile $srcfile]} {
+    return -1
+}
+
+# Used as an override for extended_gdbserver_load_last_file in
+# configs/native-extended-gdbserver.exp, prevents the remote exec-file
+# from being set.
+proc do_nothing {} { return 0 }
+
+# Check the 'show args' output.  If PACKET is 'on' then we expect to
+# see the arguments 'a b c', otherwise we don't expect to see any
+# arguments.
+proc check_show_args { packet } {
+    if { $packet } {
+	set args_re "a b c"
+    } else {
+	set args_re ""
+    }
+
+    gdb_test "show args" \
+	"Argument list to give program being debugged when it is started is \"$args_re\"\\."
+}
+
+# Check the 'show remote exec-file' output.  PACKET is either 'on' or
+# 'off' and reflects whether the qDefaultExecAndArgs packet is turned
+# on or off.  FILENAME is what we expect to see included in the
+# output, and is converted to a regexp by this function.
+proc check_remote_exec_file { packet filename } {
+    if { $filename eq "" } {
+	set remote_exec_re \
+	    "The remote exec-file is \"\", the default remote executable will be used\\."
+    } else {
+	set remote_exec_re \
+	    "The remote exec-file is \"[string_to_regexp $filename]\"\\."
+    }
+
+    gdb_test "show remote exec-file" $remote_exec_re
+}
+
+# Check the inferior has 4 arguments.  Arg 0 will be the program name,
+# while 1, 2, and 3 should be a, b, and c respectively.
+proc check_full_args {} {
+    set exec_filename ""
+    gdb_test "print argc" " = 4"
+    gdb_test_multiple "print argv\[0\]" "" {
+	-re -wrap " = $::hex \"(.*/${::testfile})\"" {
+	    set exec_filename $expect_out(1,string)
+	    pass $gdb_test_name
+	}
+    }
+    gdb_test "print argv\[1\]" " = $::hex \"a\""
+    gdb_test "print argv\[2\]" " = $::hex \"b\""
+    gdb_test "print argv\[3\]" " = $::hex \"c\""
+
+    return $exec_filename
+}
+
+# Check that GDB can fetch the arguments from the remote using the
+# qDefaultExecAndArgs packet.  When PACKET is 'on' we allow GDB to use
+# the packet, but when PACKET is 'off' we disable use of the
+# qDefaultExecAndArgs packet and ensure GDB falls back to the expected
+# behaviour.
+proc_with_prefix test_exec_and_arg_fetch { packet } {
+    clean_restart $::binfile
+
+    # Make sure we're disconnected, in case we're testing with an
+    # extended-remote board, therefore already connected.
+    gdb_test "disconnect" ".*"
+
+    gdb_test "set remote fetch-exec-and-args ${packet}" \
+	"Support for the 'qDefaultExecAndArgs' packet on future remote targets is set to \"${packet}\"\\."
+
+    gdbserver_run "a b c"
+
+    gdb_breakpoint [gdb_get_line_number "Break here"]
+    gdb_continue_to_breakpoint "run to breakpoint"
+
+    # Look in the inferior to check the arguments were passed
+    # correctly.  We get back the name of the executable the inferior
+    # is running.  If PACKET is 'on' then we expect GDB to have
+    # automatically fetched this executable name from the remote.
+    set exec_filename [check_full_args]
+    if { !$packet } {
+	set exec_filename ""
+    }
+
+    # Check 'show args' to ensure GDB sees the correct arguments.
+    check_show_args $packet
+
+    # Check 'show remote exec-file' to ensure GDB sees the correct
+    # filename.
+    check_remote_exec_file $packet $exec_filename
+
+    # Below this point we rely on restarting the inferior, which
+    # relies on the extended-remote protocol.
+    if {[target_info gdb_protocol] ne "extended-remote"} {
+	return
+    }
+
+    with_test_prefix "rerun" {
+	# Don't restart GDB, but re-run the inferior.
+	gdb_run_cmd
+	gdb_test "" \
+	    "Breakpoint $::decimal, main \\(\[^)\]+\\).*" \
+	    "rerun until breakpoint in main"
+
+	# If the packet is enabled then we expect the arguments to
+	# still be correct, otherwise, we should have defaulted back
+	# to no additional arguments.
+	if { $packet } {
+	    check_full_args
+	} else {
+	    gdb_test "print argc" " = 1"
+	}
+
+	# Check 'show args' to ensure GDB sees the correct arguments.
+	check_show_args ${packet}
+
+	# Check 'show remote exec-file' to ensure GDB sees the correct
+	# filename.
+	check_remote_exec_file $packet $exec_filename
+    }
+}
+
+# Start GDB and set 'remote exec-file' to some random file.  Then
+# start gdbserver with the name of the actual executable.  Connect to
+# gdbserver from GDB and check that GDB gives a warning about the
+# remove exec-file value having changed.
+proc_with_prefix test_remote_exec_warning {} {
+    clean_restart
+
+    gdb_test "disconnect" ".*"
+
+    # Set the file GDB is going to debug.  For extended-remote boards
+    # this also sets the remote exec-file.
+    gdb_file_cmd $::binfile
+
+    set invalid_remote_exec "/xxx/yyy/zzz"
+    gdb_test_no_output "set remote exec-file $invalid_remote_exec"
+    check_remote_exec_file on $invalid_remote_exec
+
+    # Start gdbserver.
+    set test "start gdbserver"
+    set target_exec [gdbserver_download_current_prog]
+    set target_exec_and_args "$target_exec a b c"
+    set catchres [catch {set res [gdbserver_start "" "$target_exec_and_args"]} errmsg]
+    if { $catchres != 0 } {
+	fail "$test: $errmsg"
+    } else {
+	pass "$test"
+    }
+
+    # And connect to gdbserver.  Check for the warning GDB emits when
+    # the remote exec-file is updated.
+    set gdbserver_protocol [lindex $res 0]
+    set gdbserver_gdbport [lindex $res 1]
+    set test "connect to gdbserver"
+    set extra_re "warning: updating 'remote exec-file' to '[string_to_regexp $target_exec]' to match remote target"
+    set res [gdb_target_cmd_ext $gdbserver_protocol $gdbserver_gdbport $extra_re]
+    if { $res == 0 } {
+	pass $test
+    } elseif { $res == 1 }  {
+	fail $test
+    } else {
+	unsupported $test
+    }
+}
+
+# This override prevents the remote exec-file from being set when
+# using the extended-remote protocol.  This is harmless when using
+# other boards.
+with_override extended_gdbserver_load_last_file do_nothing {
+    foreach_with_prefix packet { on off } {
+	test_exec_and_arg_fetch ${packet}
+    }
+
+    test_remote_exec_warning
+}
diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index c57270175b4..e749194e039 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -2697,6 +2697,39 @@ handle_query (char *own_buf, int packet_len, int *new_packet_len_p)
       return;
     }
 
+  if (strcmp ("qDefaultExecAndArgs", own_buf) == 0)
+    {
+      if (program_path.get () == nullptr)
+	sprintf (own_buf, "U");
+      else
+	{
+	  std::string packet ("S;");
+
+	  packet += bin2hex ((const gdb_byte *) program_path.get (),
+			     strlen (program_path.get ()));
+	  packet += ";";
+
+	  std::string args;
+	  for (const char * arg : program_args)
+	    {
+	      if (!args.empty ())
+		args += " ";
+	      args += std::string (arg);
+	    }
+	  packet += bin2hex ((const gdb_byte *) args.c_str (), args.size ());
+
+	  if (packet.size () > PBUFSIZ)
+	    {
+	      sprintf (own_buf, "E.Program name and arguments too long.");
+	      return;
+	    }
+
+	  strcpy (own_buf, packet.c_str ());
+	  *new_packet_len_p = packet.size ();
+	}
+      return;
+    }
+
   /* Otherwise we didn't know what packet it was.  Say we didn't
      understand it.  */
   own_buf[0] = 0;
-- 
2.25.4