[PATCH 0/5] Fixes for per-inferior settings and $_gdb_setting_str()

[PATCH 0/5] Fixes for per-inferior settings and $_gdb_setting_str()

[Andrew Burgess]

While working on another patch I spotted that
$_gdb_setting_str("args") wasn't working correctly.  Turns out args,
cwd, and inferior-tty all have the same problems.  This series
resolves these issues.

Thanks,
Andrew

---

Andrew Burgess (5):
  gdb: cleanup command creation in infcmd.c
  gdb: make set/show args work with $_gdb_setting_str
  gdb: make set/show cwd work with $_gdb_setting_str
  gdb: make set/show inferior-tty work with $_gdb_setting_str
  gdb: make deprecated_show_value_hack static

 gdb/cli/cli-setshow.c                     |   2 +-
 gdb/command.h                             |   7 +-
 gdb/infcmd.c                              | 127 ++++++++++------------
 gdb/testsuite/gdb.base/inferior-clone.exp |   9 ++
 gdb/testsuite/gdb.multi/gdb-settings.c    |  22 ++++
 gdb/testsuite/gdb.multi/gdb-settings.exp  | 121 +++++++++++++++++++++
 6 files changed, 213 insertions(+), 75 deletions(-)
 create mode 100644 gdb/testsuite/gdb.multi/gdb-settings.c
 create mode 100644 gdb/testsuite/gdb.multi/gdb-settings.exp


base-commit: 60a13bbcdfb0ce008a77563cea0c34c830d7b170
-- 
2.25.4

[PATCH 1/5] gdb: cleanup command creation in infcmd.c

[Andrew Burgess]

In infcmd.c, in order to add command completion to some of the 'set'
commands, we are currently creating the command, then looking up the
command by calling lookup_cmd.

This is no longer necessary, we already return the relevant
cmd_list_element object when the set/show command is created, and we
can use that to set the command completion callback.

I don't know if there's actually any tests for completion of these
commands, but I manually checked, and each command still appears to
offer the expected filename completion.

There should be no user visible changes after this commit.
---
 gdb/infcmd.c | 59 +++++++++++++++++++++++-----------------------------
 1 file changed, 26 insertions(+), 33 deletions(-)

diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index a68611538f2..dd5808b17e7 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -3138,55 +3138,48 @@ _initialize_infcmd ()
 {
   static struct cmd_list_element *info_proc_cmdlist;
   struct cmd_list_element *c = nullptr;
-  const char *cmd_name;
 
   /* Add the filename of the terminal connected to inferior I/O.  */
-  add_setshow_optional_filename_cmd ("inferior-tty", class_run,
-				     &inferior_io_terminal_scratch, _("\
-Set terminal for future runs of program being debugged."), _("\
+  auto tty_set_show =
+    add_setshow_optional_filename_cmd ("inferior-tty", class_run,
+				       &inferior_io_terminal_scratch, _("\
+Set terminal for future runs of program being debugged."), _("		\
 Show terminal for future runs of program being debugged."), _("\
 Usage: set inferior-tty [TTY]\n\n\
 If TTY is omitted, the default behavior of using the same terminal as GDB\n\
 is restored."),
-				     set_inferior_tty_command,
-				     show_inferior_tty_command,
-				     &setlist, &showlist);
-  cmd_name = "inferior-tty";
-  c = lookup_cmd (&cmd_name, setlist, "", nullptr, -1, 1);
-  gdb_assert (c != nullptr);
-  add_alias_cmd ("tty", c, class_run, 0, &cmdlist);
-
-  cmd_name = "args";
-  add_setshow_string_noescape_cmd (cmd_name, class_run,
-				   &inferior_args_scratch, _("\
+				       set_inferior_tty_command,
+				       show_inferior_tty_command,
+				       &setlist, &showlist);
+  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, _("\
 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,
-				   show_args_command,
-				   &setlist, &showlist);
-  c = lookup_cmd (&cmd_name, setlist, "", nullptr, -1, 1);
-  gdb_assert (c != nullptr);
-  set_cmd_completer (c, filename_completer);
-
-  cmd_name = "cwd";
-  add_setshow_string_noescape_cmd (cmd_name, class_run,
-				   &inferior_cwd_scratch, _("\
+				       set_args_command,
+				       show_args_command,
+				       &setlist, &showlist);
+  set_cmd_completer (args_set_show.set, filename_completer);
+
+  auto cwd_set_show =
+    add_setshow_string_noescape_cmd ("cwd", class_run,
+				     &inferior_cwd_scratch, _("\
 Set the current working directory to be used when the inferior is started.\n\
 Changing this setting does not have any effect on inferiors that are\n\
 already running."),
-				   _("\
+				     _("\
 Show the current working directory that is used when the inferior is started."),
-				   _("\
+				     _("\
 Use this command to change the current working directory that will be used\n\
 when the inferior is started.  This setting does not affect GDB's current\n\
 working directory."),
-				   set_cwd_command,
-				   show_cwd_command,
-				   &setlist, &showlist);
-  c = lookup_cmd (&cmd_name, setlist, "", nullptr, -1, 1);
-  gdb_assert (c != nullptr);
-  set_cmd_completer (c, filename_completer);
+				     set_cwd_command,
+				     show_cwd_command,
+				     &setlist, &showlist);
+  set_cmd_completer (cwd_set_show.set, filename_completer);
 
   c = add_cmd ("environment", no_class, environment_info, _("\
 The environment to give the program, or one variable's value.\n\
-- 
2.25.4

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

[Andrew Burgess]

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

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"
+}
-- 
2.25.4

[PATCH 3/5] gdb: make set/show cwd work with $_gdb_setting_str

[Andrew Burgess]

The previous commit fixed set/show args when used with
$_gdb_setting_str, this commit fixes set/show cwd.

Instead of using a scratch variable which is then pushed into the
current inferior from a set callback, move to the API that allows for
getters and setters, and store the value directly within the current
inferior.

Update the existing test to check the cwd setting.
---
 gdb/infcmd.c                             | 15 +++++----------
 gdb/testsuite/gdb.multi/gdb-settings.exp | 23 +++++++++++++++++++++++
 2 files changed, 28 insertions(+), 10 deletions(-)

diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 2f0d4f38c85..2e26ae9b934 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -66,10 +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 the new cwd will be stored by 'set cwd'.  */
-
-static std::string inferior_cwd_scratch;
-
 /* Scratch area where 'set inferior-tty' will store user-provided value.
    We'll immediate copy it into per-inferior storage.  */
 
@@ -165,12 +161,12 @@ get_inferior_cwd ()
   return current_inferior ()->cwd ();
 }
 
-/* Handle the 'set cwd' command.  */
+/* Store the new value passed to 'set cwd'.  */
 
 static void
-set_cwd_command (const char *args, int from_tty, struct cmd_list_element *c)
+set_cwd_value (const std::string &args)
 {
-  current_inferior ()->set_cwd (inferior_cwd_scratch);
+  current_inferior ()->set_cwd (args);
 }
 
 /* Handle the 'show cwd' command.  */
@@ -3164,8 +3160,7 @@ Follow this command with any number of args, to be passed to the program."),
   set_cmd_completer (args_set_show.set, filename_completer);
 
   auto cwd_set_show =
-    add_setshow_string_noescape_cmd ("cwd", class_run,
-				     &inferior_cwd_scratch, _("\
+    add_setshow_string_noescape_cmd ("cwd", class_run, _("\
 Set the current working directory to be used when the inferior is started.\n\
 Changing this setting does not have any effect on inferiors that are\n\
 already running."),
@@ -3175,7 +3170,7 @@ Show the current working directory that is used when the inferior is started."),
 Use this command to change the current working directory that will be used\n\
 when the inferior is started.  This setting does not affect GDB's current\n\
 working directory."),
-				     set_cwd_command,
+				     set_cwd_value, get_inferior_cwd,
 				     show_cwd_command,
 				     &setlist, &showlist);
   set_cmd_completer (cwd_set_show.set, filename_completer);
diff --git a/gdb/testsuite/gdb.multi/gdb-settings.exp b/gdb/testsuite/gdb.multi/gdb-settings.exp
index d2741d16157..70e0752a5bb 100644
--- a/gdb/testsuite/gdb.multi/gdb-settings.exp
+++ b/gdb/testsuite/gdb.multi/gdb-settings.exp
@@ -33,6 +33,10 @@ proc inferior_args {num} {
     return "a_${num}_1 a_${num}_2 a_${num}_3"
 }
 
+proc inferior_cwd {num} {
+    return [standard_output_file inf_${num}_dir]
+}
+
 # Start inferior NUM.
 
 proc start_inferior {num} {
@@ -41,6 +45,9 @@ proc start_inferior {num} {
 
 	set args [inferior_args ${num}]
 
+	set dir [inferior_cwd ${num}]
+	remote_exec host "mkdir -p $dir"
+
 	if {$num != 1} {
 	    gdb_test "add-inferior" "Added inferior $num.*" \
 		"add empty inferior"
@@ -50,10 +57,17 @@ proc start_inferior {num} {
 	    gdb_test_no_output "set args $args"
 	}
 
+	gdb_test_no_output "set cwd $dir" \
+	    "set cwd for inferior ${num}"
+
 	gdb_test {print $_gdb_setting_str("args")} \
 	    " = \"$args\"" \
 	    "check args before loading a file"
 
+	gdb_test {print $_gdb_setting_str("cwd")} \
+	    " = \"$dir\"" \
+	    "check cwd before loading a file"
+
 	gdb_load $binfile
 
 	if {[gdb_start_cmd] < 0} {
@@ -65,6 +79,10 @@ proc start_inferior {num} {
 	gdb_test {print $_gdb_setting_str("args")} \
 	    " = \"$args\"" \
 	    "check after after starting"
+
+	gdb_test {print $_gdb_setting_str("cwd")} \
+	    " = \"$dir\"" \
+	    "check cwd after starting"
     }
 
     return 0
@@ -95,4 +113,9 @@ for {set i 1} {$i <= $NUM_INFS} {incr i} {
     gdb_test {print $_gdb_setting_str("args")} \
 	" = \"$args\"" \
 	"check args after after switching to inferior $i"
+
+    set dir [inferior_cwd $i]
+    gdb_test {print $_gdb_setting_str("cwd")} \
+	" = \"$dir\"" \
+	"check cwd after after switching to inferior $i"
 }
-- 
2.25.4

[PATCH 4/5] gdb: make set/show inferior-tty work with $_gdb_setting_str

[Andrew Burgess]

Like the previous two commits, this commit fixes set/show inferior-tty
to work with $_gdb_setting_str.

Instead of using a scratch variable which is then pushed into the
current inferior from a set callback, move to the API that allows for
getters and setters, and store the value directly within the current
inferior.

Update an existing test to check the inferior-tty setting.
---
 gdb/infcmd.c                              | 30 +++++++++++++----------
 gdb/testsuite/gdb.base/inferior-clone.exp |  9 +++++++
 2 files changed, 26 insertions(+), 13 deletions(-)

diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 2e26ae9b934..de586f830f6 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -66,11 +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 'set inferior-tty' will store user-provided value.
-   We'll immediate copy it into per-inferior storage.  */
-
-static std::string inferior_io_terminal_scratch;
-
 /* Pid of our debugged inferior, or 0 if no inferior now.
    Since various parts of infrun.c test this to see whether there is a program
    being debugged it should be nonzero (currently 3 is used) for remote
@@ -94,15 +89,24 @@ static bool finish_print = true;
 
 \f
 
+/* Store the new value passed to 'set inferior-tty'.  */
+
 static void
-set_inferior_tty_command (const char *args, int from_tty,
-			  struct cmd_list_element *c)
+set_tty_value (const std::string &tty)
 {
-  /* CLI has assigned the user-provided value to inferior_io_terminal_scratch.
-     Now route it to current inferior.  */
-  current_inferior ()->set_tty (inferior_io_terminal_scratch);
+  current_inferior ()->set_tty (tty);
 }
 
+/* Get the current 'inferior-tty' value.  */
+
+static const std::string &
+get_tty_value ()
+{
+  return current_inferior ()->tty ();
+}
+
+/* Implement 'show inferior-tty' command.  */
+
 static void
 show_inferior_tty_command (struct ui_file *file, int from_tty,
 			   struct cmd_list_element *c, const char *value)
@@ -3136,14 +3140,14 @@ _initialize_infcmd ()
 
   /* Add the filename of the terminal connected to inferior I/O.  */
   auto tty_set_show =
-    add_setshow_optional_filename_cmd ("inferior-tty", class_run,
-				       &inferior_io_terminal_scratch, _("\
+    add_setshow_optional_filename_cmd ("inferior-tty", class_run, _("\
 Set terminal for future runs of program being debugged."), _("		\
 Show terminal for future runs of program being debugged."), _("\
 Usage: set inferior-tty [TTY]\n\n\
 If TTY is omitted, the default behavior of using the same terminal as GDB\n\
 is restored."),
-				       set_inferior_tty_command,
+				       set_tty_value,
+				       get_tty_value,
 				       show_inferior_tty_command,
 				       &setlist, &showlist);
   add_alias_cmd ("tty", tty_set_show.set, class_run, 0, &cmdlist);
diff --git a/gdb/testsuite/gdb.base/inferior-clone.exp b/gdb/testsuite/gdb.base/inferior-clone.exp
index 8e0378323f9..806ec0ad68c 100644
--- a/gdb/testsuite/gdb.base/inferior-clone.exp
+++ b/gdb/testsuite/gdb.base/inferior-clone.exp
@@ -30,6 +30,8 @@ with_test_prefix "setup inferior 1" {
     gdb_test_no_output "set environment FOO foo"
     gdb_test_no_output "set environment BAR bar"
     gdb_test_no_output "set inferior-tty some_tty"
+    gdb_test {print $_gdb_setting_str("inferior-tty")} \
+	" = \"some_tty\""
 }
 
 # Check that properties of inferior 1 have been copied
@@ -45,6 +47,8 @@ with_test_prefix "inferior 2" {
     gdb_test "show environment ENVVAR" "ENVVAR = var"
     gdb_test "show inferior-tty" \
 	"Terminal for future runs of program being debugged is \"some_tty\"\."
+    gdb_test {print $_gdb_setting_str("inferior-tty")} \
+	" = \"some_tty\""
 }
 
 # Change this second inferior, to check that the next clone-inferior
@@ -54,6 +58,9 @@ with_test_prefix "update inferior 2" {
     gdb_test_no_output "set args foo"
     gdb_test_no_output "set cwd /somewhere/else"
     gdb_test_no_output "set environment FOO oof"
+    gdb_test_no_output "set inferior-tty another_tty"
+    gdb_test {print $_gdb_setting_str("inferior-tty")} \
+	" = \"another_tty\""
 }
 
 with_test_prefix "inferior 1" {
@@ -67,6 +74,8 @@ with_test_prefix "inferior 1" {
     gdb_test "show environment ENVVAR" "ENVVAR = var"
     gdb_test "show inferior-tty" \
 	"Terminal for future runs of program being debugged is \"some_tty\"\."
+    gdb_test {print $_gdb_setting_str("inferior-tty")} \
+	" = \"some_tty\""
 }
 
 # Tweak inferior 1 a bit more.
-- 
2.25.4

[PATCH 5/5] gdb: make deprecated_show_value_hack static

[Andrew Burgess]

The deprecated_show_value_hack function is now only used inside
cli-setshow.c, so lets make the function static to discourage its use
anywhere else.

There should be no user visible changes after this commit
---
 gdb/cli/cli-setshow.c | 2 +-
 gdb/command.h         | 7 +------
 2 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/gdb/cli/cli-setshow.c b/gdb/cli/cli-setshow.c
index dad3e606620..07233e36a38 100644
--- a/gdb/cli/cli-setshow.c
+++ b/gdb/cli/cli-setshow.c
@@ -117,7 +117,7 @@ parse_cli_boolean_value (const char *arg)
 }
 
 \f
-void
+static void
 deprecated_show_value_hack (struct ui_file *ignore_file,
 			    int ignore_from_tty,
 			    struct cmd_list_element *c,
diff --git a/gdb/command.h b/gdb/command.h
index e9c9f160e1f..a4641bb1fd5 100644
--- a/gdb/command.h
+++ b/gdb/command.h
@@ -656,16 +656,11 @@ extern void complete_on_enum (completion_tracker &tracker,
 extern void help_list (struct cmd_list_element *, const char *,
 		       enum command_class, struct ui_file *);
 
-/* Method for show a set/show variable's VALUE on FILE.  If this
-   method isn't supplied deprecated_show_value_hack() is called (which
-   is not good).  */
+/* Method for show a set/show variable's VALUE on FILE.  */
 typedef void (show_value_ftype) (struct ui_file *file,
 				 int from_tty,
 				 struct cmd_list_element *cmd,
 				 const char *value);
-/* NOTE: i18n: This function is not i18n friendly.  Callers should
-   instead print the value out directly.  */
-extern show_value_ftype deprecated_show_value_hack;
 
 /* Various sets of extra literals accepted.  */
 extern const literal_def integer_unlimited_literals[];
-- 
2.25.4

Re: [PATCH 1/5] gdb: cleanup command creation in infcmd.c

[Tom Tromey]

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

Andrew> In infcmd.c, in order to add command completion to some of the 'set'
Andrew> commands, we are currently creating the command, then looking up the
Andrew> command by calling lookup_cmd.

Andrew> +  auto tty_set_show =
Andrew> +    add_setshow_optional_filename_cmd ("inferior-tty", class_run,

A nit: in the gdb style, normally the = goes after the line break.

Tom

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

[Tom Tromey]

>>>>> "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

Re: [PATCH 5/5] gdb: make deprecated_show_value_hack static

[Tom Tromey]

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

Andrew> The deprecated_show_value_hack function is now only used inside
Andrew> cli-setshow.c, so lets make the function static to discourage its use
Andrew> anywhere else.

Andrew> There should be no user visible changes after this commit

FWIW I wouldn't mind if we removed nearly all "show" functions and just
made some variant of deprecated_show_value_hack the standard way to
print things.  Instead of munging the help text (which is
i18n-unfriendly) it could just print the option name:

(gdb) show mumble var
Current setting of "mumble variable": 23

For "auto" variables, I guess we'd eventually need to implement that
"second getter" idea and then we could remove even more of these
functions, maybe all of them.

Tom

Re: [PATCH 0/5] Fixes for per-inferior settings and $_gdb_setting_str()

[Tom Tromey]

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

Andrew> While working on another patch I spotted that
Andrew> $_gdb_setting_str("args") wasn't working correctly.  Turns out args,
Andrew> cwd, and inferior-tty all have the same problems.  This series
Andrew> resolves these issues.

I sent a minor comment but nothing really worth a repost.

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

thank you,
Tom

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

[Simon Marchi]

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

Re: [PATCH 0/5] Fixes for per-inferior settings and $_gdb_setting_str()

[Simon Marchi]

On 4/4/23 08:45, Andrew Burgess via Gdb-patches wrote:
> While working on another patch I spotted that
> $_gdb_setting_str("args") wasn't working correctly.  Turns out args,
> cwd, and inferior-tty all have the same problems.  This series
> resolves these issues.

That rang a bell, it's because I have fixed some of these downstream in
ROCgdb, and haven't sent it upstream yet.  The downstream commit is
here:

https://github.com/ROCm-Developer-Tools/ROCgdb/commit/54d0a79614071fd62df5e2dfe793ff26cc7e31e6

I'll take a look at your patches, I presume that they will do mostly the
same thing.  And if my patch does more (according to the commit message,
it fixes "remote exec-file" and "tdesc filename"), I can send what
remains after.

Simon

Re: [PATCH 0/5] Fixes for per-inferior settings and $_gdb_setting_str()

[Simon Marchi]

On 4/17/23 14:09, Simon Marchi via Gdb-patches wrote:
> On 4/4/23 08:45, Andrew Burgess via Gdb-patches wrote:
>> While working on another patch I spotted that
>> $_gdb_setting_str("args") wasn't working correctly.  Turns out args,
>> cwd, and inferior-tty all have the same problems.  This series
>> resolves these issues.
> 
> That rang a bell, it's because I have fixed some of these downstream in
> ROCgdb, and haven't sent it upstream yet.  The downstream commit is
> here:
> 
> https://github.com/ROCm-Developer-Tools/ROCgdb/commit/54d0a79614071fd62df5e2dfe793ff26cc7e31e6
> 
> I'll take a look at your patches, I presume that they will do mostly the
> same thing.  And if my patch does more (according to the commit message,
> it fixes "remote exec-file" and "tdesc filename"), I can send what
> remains after.

So, my patch adds some tests for gdb.parameter (Python), MI and the with
command.  However, they are sprinkled in different files.  I like that
you have a test file specifically to exercise the settings that are
per-inferior.  Your test could probably be augmented to exercise all
these ways to get a setting.

You series looks good to me, up to you if you want to merge it as-is or
improve it based on what I had.

Simon

Re: [PATCH 5/5] gdb: make deprecated_show_value_hack static

[Andrew Burgess]

Tom Tromey <tom@tromey.com> writes:

>>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:
>
> Andrew> The deprecated_show_value_hack function is now only used inside
> Andrew> cli-setshow.c, so lets make the function static to discourage its use
> Andrew> anywhere else.
>
> Andrew> There should be no user visible changes after this commit
>
> FWIW I wouldn't mind if we removed nearly all "show" functions and just
> made some variant of deprecated_show_value_hack the standard way to
> print things.  Instead of munging the help text (which is
> i18n-unfriendly) it could just print the option name:
>
> (gdb) show mumble var
> Current setting of "mumble variable": 23

I have a branch which I occasionally work on which is filling in all the
missing show functions -- which is the opposite to what you are
suggesting.

Didn't plan to post it anytime soon though, as I only work on it when
I'm /really/ bored.

Thanks,
Andrew

Re: [PATCH 0/5] Fixes for per-inferior settings and $_gdb_setting_str()

[Andrew Burgess]

Simon Marchi <simark@simark.ca> writes:

> On 4/4/23 08:45, Andrew Burgess via Gdb-patches wrote:
>> While working on another patch I spotted that
>> $_gdb_setting_str("args") wasn't working correctly.  Turns out args,
>> cwd, and inferior-tty all have the same problems.  This series
>> resolves these issues.
>
> That rang a bell, it's because I have fixed some of these downstream in
> ROCgdb, and haven't sent it upstream yet.  The downstream commit is
> here:
>
> https://github.com/ROCm-Developer-Tools/ROCgdb/commit/54d0a79614071fd62df5e2dfe793ff26cc7e31e6

In case I miss this patch when you upstream it, the removal of
show_remote_exec_file should be dropped.  The generic code doesn't
handle internationalisation, and though show_remote_exec_file itself
also doesn't do internationalisation, the correct (IMHO) thing would be
to fix show_remote_exec_file, not remove it.

>
> I'll take a look at your patches, I presume that they will do mostly the
> same thing.  And if my patch does more (according to the commit message,
> it fixes "remote exec-file" and "tdesc filename"), I can send what
> remains after.

I'll take a look at the testing in your patch and see if I can extend
the tests in my patch a little.

Thanks,
Andrew

Re: [PATCH 0/5] Fixes for per-inferior settings and $_gdb_setting_str()

[Andrew Burgess]

Simon Marchi <simark@simark.ca> writes:

> On 4/17/23 14:09, Simon Marchi via Gdb-patches wrote:
>> On 4/4/23 08:45, Andrew Burgess via Gdb-patches wrote:
>>> While working on another patch I spotted that
>>> $_gdb_setting_str("args") wasn't working correctly.  Turns out args,
>>> cwd, and inferior-tty all have the same problems.  This series
>>> resolves these issues.
>> 
>> That rang a bell, it's because I have fixed some of these downstream in
>> ROCgdb, and haven't sent it upstream yet.  The downstream commit is
>> here:
>> 
>> https://github.com/ROCm-Developer-Tools/ROCgdb/commit/54d0a79614071fd62df5e2dfe793ff26cc7e31e6
>> 
>> I'll take a look at your patches, I presume that they will do mostly the
>> same thing.  And if my patch does more (according to the commit message,
>> it fixes "remote exec-file" and "tdesc filename"), I can send what
>> remains after.
>
> So, my patch adds some tests for gdb.parameter (Python), MI and the with
> command.  However, they are sprinkled in different files.  I like that
> you have a test file specifically to exercise the settings that are
> per-inferior.  Your test could probably be augmented to exercise all
> these ways to get a setting.
>
> You series looks good to me, up to you if you want to merge it as-is or
> improve it based on what I had.

I reworked the tests taking onboard some of the ideas from your tests,
and pushed this series.

Do you plan to upstream your patch anytime soon?  I had been planning to
look at other per-inferior settings once this series was merged, but I
don't want to tread on your toes any more than I already have...

Thanks,
Andrew

Re: [PATCH 5/5] gdb: make deprecated_show_value_hack static

[Tom Tromey]

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

>> FWIW I wouldn't mind if we removed nearly all "show" functions and just
>> made some variant of deprecated_show_value_hack the standard way to
>> print things.  Instead of munging the help text (which is
>> i18n-unfriendly) it could just print the option name:
>> 
>> (gdb) show mumble var
>> Current setting of "mumble variable": 23

Andrew> I have a branch which I occasionally work on which is filling in all the
Andrew> missing show functions -- which is the opposite to what you are
Andrew> suggesting.

Forgot to reply to this when you first sent it.

For me the attraction of removing these is that it simplifies things --
less code, less to get wrong.  Maybe it would require us to also
regularize & automate the "auto means xyz in this context" concept, but
that would also be beneficial (I think there's a bug in the Python
component about this).  I also don't really mind having a "computer-ish"
output format here, as opposed to natural language text.

However if you want to do it the other way, that's also fine.

Tom