[2/5] gdb: make set/show args work with $_gdb_setting_str

Message ID 14b084ae3950c36f8e631404975d3468813ef3fc.1680608960.git.aburgess@redhat.com
State New
Headers
Series Fixes for per-inferior settings and $_gdb_setting_str() |

Commit Message

Andrew Burgess April 4, 2023, 12:45 p.m. UTC
  I noticed that $_gdb_setting_str was not working with 'args', e.g.:

  $ gdb -q --args /tmp/hello.x arg1 arg2 arg3
  Reading symbols from /tmp/hello.x...
  (gdb) show args
  Argument list to give program being debugged when it is started is "arg1 arg2 arg3".
  (gdb) print $_gdb_setting_str("args")
  $1 = ""

This is because the 'args' setting is implemented using a scratch
variable ('inferior_args_scratch') which is updated when the user does
'set args ...'.  There is then a function 'set_args_command' which is
responsible for copying the scratch area into the current inferior.

However, when the user sets the arguments via the command line the
scratch variable is not updated, instead the arguments are pushed
straight into the current inferior.

There is a second problem, when the current inferior changes the
scratch area is not updated, which means that the value returned will
only ever reflect the last call to 'set args ...' regardless of which
inferior is currently selected.

Luckily, the fix is pretty easy, set/show variables have an
alternative API which requires we provide some getter and setter
functions.  With this done the scratch variable can be removed and the
value returned will now always reflect the current inferior.

While working on set/show args I also rewrote show_args_command to
remove the use of deprecated_show_value_hack.
---
 gdb/infcmd.c                             | 41 +++++-----
 gdb/testsuite/gdb.multi/gdb-settings.c   | 22 ++++++
 gdb/testsuite/gdb.multi/gdb-settings.exp | 98 ++++++++++++++++++++++++
 3 files changed, 140 insertions(+), 21 deletions(-)
 create mode 100644 gdb/testsuite/gdb.multi/gdb-settings.c
 create mode 100644 gdb/testsuite/gdb.multi/gdb-settings.exp
  

Comments

Tom Tromey April 17, 2023, 4:37 p.m. UTC | #1
>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:

Andrew> Luckily, the fix is pretty easy, set/show variables have an
Andrew> alternative API which requires we provide some getter and setter
Andrew> functions.  With this done the scratch variable can be removed and the
Andrew> value returned will now always reflect the current inferior.

Wow, I didn't realize this had been implemented.

I think all options requiring the "two phase" approach should be
rewritten to this form, and the old form just removed entirely.

This is related to https://sourceware.org/bugzilla/show_bug.cgi?id=12188
but not exactly -- I guess we'd have to add a second getter API to
convert "auto" parameters to their "current" value.

Tom
  
Simon Marchi April 17, 2023, 6:04 p.m. UTC | #2
On 4/17/23 12:37, Tom Tromey wrote:
>>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Andrew> Luckily, the fix is pretty easy, set/show variables have an
> Andrew> alternative API which requires we provide some getter and setter
> Andrew> functions.  With this done the scratch variable can be removed and the
> Andrew> value returned will now always reflect the current inferior.
> 
> Wow, I didn't realize this had been implemented.
> 
> I think all options requiring the "two phase" approach should be
> rewritten to this form, and the old form just removed entirely.
> 
> This is related to https://sourceware.org/bugzilla/show_bug.cgi?id=12188
> but not exactly -- I guess we'd have to add a second getter API to
> convert "auto" parameters to their "current" value.

We implemented this downstream in ROCgdb, I'm currently working on
upstreaming it.  I'm going through all settings, trying to see which
ones could use it.

https://review.lttng.org/c/binutils-gdb/+/9764

Simon
  

Patch

diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index dd5808b17e7..2f0d4f38c85 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -66,14 +66,6 @@  static void step_1 (int, int, const char *);
 #define ERROR_NO_INFERIOR \
    if (!target_has_execution ()) error (_("The program is not being run."));
 
-/* Scratch area where string containing arguments to give to the
-   program will be stored by 'set args'.  As soon as anything is
-   stored, notice_args_set will move it into per-inferior storage.
-   Arguments are separated by spaces.  Empty string (pointer to '\0')
-   means no args.  */
-
-static std::string inferior_args_scratch;
-
 /* Scratch area where the new cwd will be stored by 'set cwd'.  */
 
 static std::string inferior_cwd_scratch;
@@ -136,26 +128,33 @@  set_inferior_args_vector (int argc, char **argv)
   current_inferior ()->set_args (std::move (n));
 }
 
-/* Notice when `set args' is run.  */
+/* Store the new value passed to 'set args'.  */
 
 static void
-set_args_command (const char *args, int from_tty, struct cmd_list_element *c)
+set_args_value (const std::string &args)
 {
-  /* CLI has assigned the user-provided value to inferior_args_scratch.
-     Now route it to current inferior.  */
-  current_inferior ()->set_args (inferior_args_scratch);
+  current_inferior ()->set_args (args);
 }
 
-/* Notice when `show args' is run.  */
+/* Return the value for 'show args' to display.  */
+
+static const std::string &
+get_args_value ()
+{
+  return current_inferior ()->args ();
+}
+
+/* Callback to implement 'show args' command.  */
 
 static void
 show_args_command (struct ui_file *file, int from_tty,
 		   struct cmd_list_element *c, const char *value)
 {
-  /* Note that we ignore the passed-in value in favor of computing it
-     directly.  */
-  deprecated_show_value_hack (file, from_tty, c,
-			      current_inferior ()->args ().c_str ());
+  /* Ignore the passed in value, pull the argument directly from the
+     inferior.  However, these should always be the same.  */
+  gdb_printf (_("\
+Argument list to give program being debugged when it is started is \"%s\".\n"),
+	      current_inferior ()->args ().c_str ());
 }
 
 /* See gdbsupport/common-inferior.h.  */
@@ -3154,12 +3153,12 @@  is restored."),
   add_alias_cmd ("tty", tty_set_show.set, class_run, 0, &cmdlist);
 
   auto args_set_show
-    = add_setshow_string_noescape_cmd ("args", class_run,
-				       &inferior_args_scratch, _("\
+    = add_setshow_string_noescape_cmd ("args", class_run, _("\
 Set argument list to give program being debugged when it is started."), _("\
 Show argument list to give program being debugged when it is started."), _("\
 Follow this command with any number of args, to be passed to the program."),
-				       set_args_command,
+				       set_args_value,
+				       get_args_value,
 				       show_args_command,
 				       &setlist, &showlist);
   set_cmd_completer (args_set_show.set, filename_completer);
diff --git a/gdb/testsuite/gdb.multi/gdb-settings.c b/gdb/testsuite/gdb.multi/gdb-settings.c
new file mode 100644
index 00000000000..3a264f239ed
--- /dev/null
+++ b/gdb/testsuite/gdb.multi/gdb-settings.c
@@ -0,0 +1,22 @@ 
+/* 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/>.  */
+
+int
+main ()
+{
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.multi/gdb-settings.exp b/gdb/testsuite/gdb.multi/gdb-settings.exp
new file mode 100644
index 00000000000..d2741d16157
--- /dev/null
+++ b/gdb/testsuite/gdb.multi/gdb-settings.exp
@@ -0,0 +1,98 @@ 
+# 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 use of $_gdb_setting_str with multiple inferiors.  Mostly
+# this test is interested in checking that per-inferior settings work
+# correctly.
+
+standard_testfile
+
+require !use_gdb_stub
+
+if {[build_executable "failed to prepare" $testfile $srcfile]} {
+    return -1
+}
+
+# Return a string containing the args to pass to inferior NUM.
+
+proc inferior_args {num} {
+    return "a_${num}_1 a_${num}_2 a_${num}_3"
+}
+
+# Start inferior NUM.
+
+proc start_inferior {num} {
+    with_test_prefix "start_inferior $num" {
+	global srcfile binfile
+
+	set args [inferior_args ${num}]
+
+	if {$num != 1} {
+	    gdb_test "add-inferior" "Added inferior $num.*" \
+		"add empty inferior"
+	    gdb_test "inferior $num" "Switching to inferior $num.*" \
+		"switch to inferior"
+
+	    gdb_test_no_output "set args $args"
+	}
+
+	gdb_test {print $_gdb_setting_str("args")} \
+	    " = \"$args\"" \
+	    "check args before loading a file"
+
+	gdb_load $binfile
+
+	if {[gdb_start_cmd] < 0} {
+	    fail "could not start"
+	    return -1
+	}
+	gdb_test "" ".*reakpoint .*, main .*${srcfile}.*" "start"
+
+	gdb_test {print $_gdb_setting_str("args")} \
+	    " = \"$args\"" \
+	    "check after after starting"
+    }
+
+    return 0
+}
+
+# For the first inferior we set the arguments on the command line.
+save_vars { GDBFLAGS } {
+    set args [inferior_args 1]
+    append GDBFLAGS " --args ${binfile} $args"
+    clean_restart
+}
+
+# An arbitrary number of inferiors to start.
+set NUM_INFS 3
+
+# Start each inferior, setting (and checking) its args as we do.
+for {set i 1} {$i <= $NUM_INFS} {incr i} {
+    if {[start_inferior $i] < 0} {
+	return -1
+    }
+}
+
+# Switch back to each inferior and check the args are correct.
+for {set i 1} {$i <= $NUM_INFS} {incr i} {
+    gdb_test "inferior $i" "Switching to inferior $i .*"
+
+    set args [inferior_args $i]
+    gdb_test {print $_gdb_setting_str("args")} \
+	" = \"$args\"" \
+	"check args after after switching to inferior $i"
+}