[PATCH 0/2] Add more DAP launch parameters

[PATCH 0/2] Add more DAP launch parameters

[Tom Tromey]

This series adds some more Python attributes (and methods), and then
uses these to implement some new launch parameters for DAP.

Inferior.main_name is added but not used.  I plan to use this to
implement a "stop at main" launch parameter, after we work out a few
details.  Meanwhile it seemed harmless and potentially useful.

Tom

---
Tom Tromey (2):
      Add attributes and methods to gdb.Inferior
      Add "args" and "env" parameters to DAP launch request

 gdb/NEWS                                 |  10 ++
 gdb/doc/gdb.texinfo                      |  13 ++-
 gdb/doc/python.texi                      |  43 ++++++++
 gdb/python/lib/gdb/dap/launch.py         |  20 +++-
 gdb/python/py-inferior.c                 | 168 +++++++++++++++++++++++++++++++
 gdb/testsuite/gdb.ada/py_range.exp       |   4 +
 gdb/testsuite/gdb.dap/args-env.c         |  28 ++++++
 gdb/testsuite/gdb.dap/args-env.exp       |  90 +++++++++++++++++
 gdb/testsuite/gdb.python/py-inferior.exp |  36 +++++++
 gdb/testsuite/lib/dap-support.exp        |  39 +++++--
 10 files changed, 439 insertions(+), 12 deletions(-)
---
base-commit: d9cc4b060dd23724e1acf974aed3d1b72c8459e5
change-id: 20230509-dap-args-596e21f0b48d

Best regards,
-- 
Tom Tromey <tromey@adacore.com>

[PATCH 1/2] Add attributes and methods to gdb.Inferior

[Tom Tromey]

This adds two new attributes and three new methods to gdb.Inferior.

The attributes let Python code see the command-line arguments and the
name of "main".  Argument setting is also supported.

The methods let Python code manipulate the inferior's environment
variables.
---
 gdb/NEWS                                 |  10 ++
 gdb/doc/python.texi                      |  43 ++++++++
 gdb/python/py-inferior.c                 | 168 +++++++++++++++++++++++++++++++
 gdb/testsuite/gdb.ada/py_range.exp       |   4 +
 gdb/testsuite/gdb.python/py-inferior.exp |  36 +++++++
 5 files changed, 261 insertions(+)

diff --git a/gdb/NEWS b/gdb/NEWS
index 6aa0d5171f2..8e260039b43 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -170,6 +170,16 @@ info main
      (program-counter) values, and can be used as the frame-id when
      calling gdb.PendingFrame.create_unwind_info.
 
+  ** gdb.Inferior now has a new "arguments" attribute.  This holds the
+     command-line arguments to the inferior, if known.
+
+  ** gdb.Inferior now has a new "main_name" attribute.  This holds the
+     name of the inferior's "main", if known.
+
+  ** gdb.Inferior now has new methods "clear_env", "set_env", and
+     "unset_env".  These can be used to modify the inferior's
+     environment before it is started.
+
 *** Changes in GDB 13
 
 * MI version 1 is deprecated, and will be removed in GDB 14.
diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
index 7c3a3ccd379..cd56a67e35a 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -3446,10 +3446,30 @@ Boolean signaling whether the inferior was created using `attach', or
 started by @value{GDBN} itself.
 @end defvar
 
+@defvar Inferior.main_name
+A string holding the name of this inferior's ``main'' function, if it
+can be determined.  If the name of main is not known, this is
+@code{None}.
+@end defvar
+
 @defvar Inferior.progspace
 The inferior's program space.  @xref{Progspaces In Python}.
 @end defvar
 
+@defvar Inferior.arguments
+The inferior's command line arguments, if known.  This corresponds to
+the @code{set args} and @code{show args} commands.  @xref{Arguments}.
+
+When accessed, the value is a string holding all the arguments.  The
+contents are quoted as they would be when passed to the shell.  If
+there are no arguments, the value is @code{None}.
+
+Either a string or a sequence of strings can be assigned to this
+attribute.  When a string is assigned, it is assumed to have any
+necessary quoting for the shell; when a sequence is assigned, the
+quoting is applied by @value{GDBN}.
+@end defvar
+
 A @code{gdb.Inferior} object has the following methods:
 
 @defun Inferior.is_valid ()
@@ -3517,6 +3537,29 @@ the same functionality, but use of @code{Inferior.thread_from_thread_handle}
 is deprecated.
 @end defun
 
+
+The environment that will be passed to the inferior can be changed
+from Python.  These methods only take effect when the inferior is
+started -- they will not affect an inferior that is already executing.
+
+@findex Inferior.clear_env
+@defun Inferior.clear_env ()
+Clear the current environment variables that will be passed to this
+inferior.
+@end defun
+
+@findex Inferior.set_env
+@defun Inferior.set_env (name, value)
+Set the environment variable @var{name} to have the indicated value.
+Both parameters must be strings.
+@end defun
+
+@findex Inferior.unset_env
+@defun Inferior.unset_env (name)
+Unset the environment variable @var{name}.  @var{name} must be a
+string.
+@end defun
+
 @node Events In Python
 @subsubsection Events In Python
 @cindex inferior events in Python
diff --git a/gdb/python/py-inferior.c b/gdb/python/py-inferior.c
index 982d0f803a0..8f3703cc738 100644
--- a/gdb/python/py-inferior.c
+++ b/gdb/python/py-inferior.c
@@ -770,6 +770,161 @@ infpy_repr (PyObject *obj)
 			       inf->num, inf->pid);
 }
 
+/* Implement clear_env.  */
+
+static PyObject *
+infpy_clear_env (PyObject *obj)
+{
+  inferior_object *self = (inferior_object *) obj;
+
+  INFPY_REQUIRE_VALID (self);
+
+  self->inferior->environment.clear ();
+  Py_RETURN_NONE;
+}
+
+/* Implement set_env.  */
+
+static PyObject *
+infpy_set_env (PyObject *obj, PyObject *args, PyObject *kw)
+{
+  inferior_object *self = (inferior_object *) obj;
+  INFPY_REQUIRE_VALID (self);
+
+  const char *name, *val;
+  static const char *keywords[] = { "name", "value", nullptr };
+
+  if (!gdb_PyArg_ParseTupleAndKeywords (args, kw, "ss", keywords,
+					&name, &val))
+    return nullptr;
+
+  self->inferior->environment.set (name, val);
+  Py_RETURN_NONE;
+}
+
+/* Implement unset_env.  */
+
+static PyObject *
+infpy_unset_env (PyObject *obj, PyObject *args)
+{
+  inferior_object *self = (inferior_object *) obj;
+  INFPY_REQUIRE_VALID (self);
+
+  const char *name;
+  if (!PyArg_ParseTuple (args, "s", &name))
+    return nullptr;
+
+  self->inferior->environment.unset (name);
+  Py_RETURN_NONE;
+}
+
+/* Getter for "arguments".  */
+
+static PyObject *
+infpy_get_args (PyObject *self, void *closure)
+{
+  inferior_object *inf = (inferior_object *) self;
+
+  INFPY_REQUIRE_VALID (inf);
+
+  const std::string &args = inf->inferior->args ();
+  if (args.empty ())
+    Py_RETURN_NONE;
+
+  return host_string_to_python_string (args.c_str ()).release ();
+}
+
+/* Setter for "arguments".  */
+
+static int
+infpy_set_args (PyObject *self, PyObject *value, void *closure)
+{
+  inferior_object *inf = (inferior_object *) self;
+
+  if (!inf->inferior)
+    {
+      PyErr_SetString (PyExc_RuntimeError, _("Inferior no longer exists."));
+      return -1;
+    }
+
+  if (value == nullptr)
+    {
+      PyErr_SetString (PyExc_TypeError, _("Cannot delete 'arguments' attribute."));
+      return -1;
+    }
+
+  if (PyUnicode_Check (value))
+    {
+      gdb::unique_xmalloc_ptr<char> str = python_string_to_host_string (value);
+      if (str == nullptr)
+	return -1;
+      inf->inferior->set_args (std::string (str.get ()));
+    }
+  else if (PySequence_Check (value))
+    {
+      std::vector<gdb::unique_xmalloc_ptr<char>> args;
+      Py_ssize_t len = PySequence_Size (value);
+      if (len == -1)
+	return -1;
+      for (Py_ssize_t i = 0; i < len; ++i)
+	{
+	  gdbpy_ref<> item (PySequence_ITEM (value, i));
+	  if (item == nullptr)
+	    return -1;
+	  gdb::unique_xmalloc_ptr<char> str
+	    = python_string_to_host_string (item.get ());
+	  if (str == nullptr)
+	    return -1;
+	  args.push_back (std::move (str));
+	}
+      std::vector<char *> argvec;
+      for (const auto &arg : args)
+	argvec.push_back (arg.get ());
+      gdb::array_view<char * const> view (argvec.data (), argvec.size ());
+      inf->inferior->set_args (view);
+    }
+  else
+    {
+      PyErr_SetString (PyExc_TypeError,
+		       _("string or sequence required for 'arguments'"));
+      return -1;
+    }
+  return 0;
+}
+
+/* Getter for "main_name".  */
+
+static PyObject *
+infpy_get_main_name (PyObject *self, void *closure)
+{
+  inferior_object *inf = (inferior_object *) self;
+
+  INFPY_REQUIRE_VALID (inf);
+
+  const char *name = nullptr;
+  try
+    {
+      /* This is unfortunate but the implementation of main_name can
+	 reach into memory, among other things.  */
+      scoped_restore_current_inferior restore_inferior;
+      set_current_inferior (inf->inferior);
+
+      scoped_restore_current_program_space restore_current_progspace;
+      set_current_program_space (inf->inferior->pspace);
+
+      name = main_name ();
+    }
+  catch (const gdb_exception &except)
+    {
+      /* We can just ignore this.  */
+      name = nullptr;
+    }
+
+  if (name == nullptr)
+    Py_RETURN_NONE;
+
+  return host_string_to_python_string (name).release ();
+}
 
 static void
 infpy_dealloc (PyObject *obj)
@@ -844,6 +999,8 @@ GDBPY_INITIALIZE_FILE (gdbpy_initialize_inferior);
 
 static gdb_PyGetSetDef inferior_object_getset[] =
 {
+  { "arguments", infpy_get_args, infpy_set_args,
+    "Arguments to this program.", nullptr },
   { "num", infpy_get_num, NULL, "ID of inferior, as assigned by GDB.", NULL },
   { "connection", infpy_get_connection, NULL,
     "The gdb.TargetConnection for this inferior.", NULL },
@@ -854,6 +1011,8 @@ static gdb_PyGetSetDef inferior_object_getset[] =
   { "was_attached", infpy_get_was_attached, NULL,
     "True if the inferior was created using 'attach'.", NULL },
   { "progspace", infpy_get_progspace, NULL, "Program space of this inferior" },
+  { "main_name", infpy_get_main_name, nullptr,
+    "Name of 'main' function, if known.", nullptr },
   { NULL }
 };
 
@@ -889,6 +1048,15 @@ Return thread object corresponding to thread handle." },
   { "architecture", (PyCFunction) infpy_architecture, METH_NOARGS,
     "architecture () -> gdb.Architecture\n\
 Return architecture of this inferior." },
+  { "clear_env", (PyCFunction) infpy_clear_env, METH_NOARGS,
+    "clear_env () -> None\n\
+Clear environment of this inferior." },
+  { "set_env", (PyCFunction) infpy_set_env, METH_VARARGS | METH_KEYWORDS,
+    "set_env (name, value) -> None\n\
+Set an environment variable of this inferior." },
+  { "unset_env", infpy_unset_env, METH_VARARGS,
+    "unset_env (name) -> None\n\
+Unset an environment of this inferior." },
   { NULL }
 };
 
diff --git a/gdb/testsuite/gdb.ada/py_range.exp b/gdb/testsuite/gdb.ada/py_range.exp
index 2972db21827..833493bad95 100644
--- a/gdb/testsuite/gdb.ada/py_range.exp
+++ b/gdb/testsuite/gdb.ada/py_range.exp
@@ -42,3 +42,7 @@ gdb_test "python print(gdb.parse_and_eval('si').type)" \
     "foo\\.small_integer" "print type"
 gdb_test "python print(gdb.parse_and_eval('si').type.name)" \
     "foo\\.small_integer" "print type name"
+
+gdb_test "python print(gdb.selected_inferior().main_name)" \
+    "_ada_foo" \
+    "print main name"
diff --git a/gdb/testsuite/gdb.python/py-inferior.exp b/gdb/testsuite/gdb.python/py-inferior.exp
index 424050a6166..58deece5d5f 100644
--- a/gdb/testsuite/gdb.python/py-inferior.exp
+++ b/gdb/testsuite/gdb.python/py-inferior.exp
@@ -342,3 +342,39 @@ with_test_prefix "architecture" {
 	"True" \
 	"inferior architecture matches frame architecture"
 }
+
+gdb_test "python print(gdb.selected_inferior().main_name)" \
+    "main" \
+    "print main name"
+
+gdb_test_no_output "set args x y z"
+gdb_test "python print(gdb.selected_inferior().arguments)" \
+    "x y z" \
+    "print arguments"
+
+gdb_test_no_output "python gdb.selected_inferior().arguments = 'a b c'" \
+    "set arguments from string"
+gdb_test "show args" \
+    [string_to_regexp "Argument list to give program being debugged when it is started is \"a b c\"."] \
+    "show args from string"
+
+gdb_test_no_output "python gdb.selected_inferior().arguments = \['a', 'b c'\]" \
+    "set arguments from list"
+gdb_test "show args" \
+    [string_to_regexp "Argument list to give program being debugged when it is started is \"a b\\ c\"."] \
+    "show args from list"
+
+gdb_test_no_output "python gdb.selected_inferior().clear_env()" \
+    "clear environment"
+gdb_test_no_output "show environment"
+
+gdb_test_no_output "python gdb.selected_inferior().set_env('DEI', 'value')" \
+    "set environment variable"
+gdb_test "show environment" \
+    "DEI=value" \
+    "examine environment variable"
+
+gdb_test_no_output "python gdb.selected_inferior().unset_env('DEI')" \
+    "unset environment variable"
+gdb_test_no_output "show environment" \
+    "environment is empty again"

-- 
2.40.0

[PATCH 2/2] Add "args" and "env" parameters to DAP launch request

[Tom Tromey]

This patch augments the DAP launch request with some optional new
parameters that let the client control the command-line arguments and
the environment of the inferior.
---
 gdb/doc/gdb.texinfo                | 13 +++++-
 gdb/python/lib/gdb/dap/launch.py   | 20 ++++++++-
 gdb/testsuite/gdb.dap/args-env.c   | 28 ++++++++++++
 gdb/testsuite/gdb.dap/args-env.exp | 90 ++++++++++++++++++++++++++++++++++++++
 gdb/testsuite/lib/dap-support.exp  | 39 +++++++++++++----
 5 files changed, 178 insertions(+), 12 deletions(-)

diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 8c4177c1901..343612060c6 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -38998,10 +38998,21 @@ Generally, @value{GDBN} implements the Debugger Adapter Protocol as
 written.  However, in some cases, extensions are either needed or even
 expected.
 
-@value{GDBN} defines a parameter that can be passed to the
+@value{GDBN} defines some parameters that can be passed to the
 @code{launch} request:
 
 @table @code
+@item args
+If provided, this should be an array of strings.  These strings are
+provided as command-line arguments to the inferior, as if by
+@code{set args}.  @xref{Arguments}.
+
+@item env
+If provided, this should be an object where each value is a string.
+The environment of the inferior will be set to exactly as passed in,
+as if by a sequence of invocations of @code{set environment} and
+@code{unset environment}.  @xref{Environment}.
+
 @item program
 If provided, this is a string that specifies the program to use.  This
 corresponds to the @code{file} command.  @xref{Files}.
diff --git a/gdb/python/lib/gdb/dap/launch.py b/gdb/python/lib/gdb/dap/launch.py
index b4102cc28cc..21499a339e1 100644
--- a/gdb/python/lib/gdb/dap/launch.py
+++ b/gdb/python/lib/gdb/dap/launch.py
@@ -13,20 +13,36 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
+import gdb
 from .events import ExecutionInvoker
 from .server import request, capability
-from .startup import send_gdb
+from .startup import send_gdb, in_gdb_thread
 
 
 _program = None
 
 
+@in_gdb_thread
+def _set_args_env(args, env):
+    inf = gdb.selected_inferior()
+    inf.arguments = args
+    if env is not None:
+        inf.clear_env()
+        for name, value in env.items():
+            inf.set_env(name, value)
+
+
+# Any parameters here are necessarily extensions -- DAP requires this
+# from implementations.  Any additions or changes here should be
+# documented in the gdb manual.
 @request("launch")
-def launch(*, program=None, **args):
+def launch(*, program=None, args=[], env=None, **extra):
     if program is not None:
         global _program
         _program = program
         send_gdb(f"file {_program}")
+    if len(args) > 0 or env is not None:
+        send_gdb(lambda: _set_args_env(args, env))
 
 
 @capability("supportsConfigurationDoneRequest")
diff --git a/gdb/testsuite/gdb.dap/args-env.c b/gdb/testsuite/gdb.dap/args-env.c
new file mode 100644
index 00000000000..bc7f1d4b38e
--- /dev/null
+++ b/gdb/testsuite/gdb.dap/args-env.c
@@ -0,0 +1,28 @@
+/* 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/>.  */
+
+#include <stdio.h>
+#include <stdlib.h>
+
+int
+main (int argc, char *argv[])
+{
+  const char *value = getenv ("DEI");
+  const char *no_value = getenv ("NOSUCHVARIABLE");
+
+  return 0; /* BREAK */
+}
diff --git a/gdb/testsuite/gdb.dap/args-env.exp b/gdb/testsuite/gdb.dap/args-env.exp
new file mode 100644
index 00000000000..96fbb28d9ce
--- /dev/null
+++ b/gdb/testsuite/gdb.dap/args-env.exp
@@ -0,0 +1,90 @@
+# 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 environment variables and command line arguments via DAP.
+
+require allow_dap_tests !use_gdb_stub
+
+load_lib dap-support.exp
+
+standard_testfile
+
+if {[build_executable ${testfile}.exp $testfile] == -1} {
+    return
+}
+
+if {[dap_launch $testfile {a "b c"} {{DEI something}}] == ""} {
+    return
+}
+
+set line [gdb_get_line_number "BREAK"]
+set obj [dap_check_request_and_response "set breakpoint by line number" \
+	     setBreakpoints \
+	     [format {o source [o path [%s]] breakpoints [a [o line [i %d]]]} \
+		  [list s $srcfile] $line]]
+set line_bpno [dap_get_breakpoint_number $obj]
+
+dap_check_request_and_response "start inferior" configurationDone
+dap_wait_for_event_and_check "inferior started" thread "body reason" started
+
+dap_wait_for_event_and_check "stopped at line breakpoint" stopped \
+    "body reason" breakpoint \
+    "body hitBreakpointIds" $line_bpno
+
+set bt [lindex [dap_check_request_and_response "backtrace" stackTrace \
+		    {o threadId [i 1]}] \
+	    0]
+set frame_id [dict get [lindex [dict get $bt body stackFrames] 0] id]
+
+set obj [dap_check_request_and_response \
+	     "evaluate argc in function" \
+	     evaluate [format {o expression [s argc] frameId [i %s]} \
+			   $frame_id]]
+dap_match_values "argc in function" [lindex $obj 0] \
+    "body result" 3
+
+set obj [dap_check_request_and_response \
+	     "evaluate first argument in function" \
+	     evaluate [format {o expression [s {argv[1]}] frameId [i %s]} \
+			   $frame_id]]
+set val [dict get [lindex $obj 0] body result]
+# This ends up with some extra quoting.
+gdb_assert {[string first "\\\"a\\\"" $val] != -1} \
+    "first argument in function"
+
+set obj [dap_check_request_and_response \
+	     "evaluate second argument in function" \
+	     evaluate [format {o expression [s {argv[2]}] frameId [i %s]} \
+			   $frame_id]]
+set val [dict get [lindex $obj 0] body result]
+# This ends up with some extra quoting.
+gdb_assert {[string first "\\\"b c\\\"" $val] != -1} \
+    "second argument in function"
+
+set obj [dap_check_request_and_response "evaluate value in function" \
+	     evaluate [format {o expression [s value] frameId [i %s]} \
+			   $frame_id]]
+set val [dict get [lindex $obj 0] body result]
+# This ends up with some extra quoting.
+gdb_assert {[string first "\\\"something\\\"" $val] != -1} \
+    "value in function"
+
+set obj [dap_check_request_and_response "evaluate no_value in function" \
+	     evaluate [format {o expression [s no_value] frameId [i %s]} \
+			   $frame_id]]
+dap_match_values "no_value in function" [lindex $obj 0] \
+    "body result" 0
+
+dap_shutdown
diff --git a/gdb/testsuite/lib/dap-support.exp b/gdb/testsuite/lib/dap-support.exp
index 6bb9b6e6377..ead295bdbfe 100644
--- a/gdb/testsuite/lib/dap-support.exp
+++ b/gdb/testsuite/lib/dap-support.exp
@@ -236,17 +236,38 @@ proc _dap_initialize {name} {
 # Start gdb, send a DAP initialize request, and then a launch request
 # specifying FILE as the program to use for the inferior.  Returns the
 # empty string on failure, or the response object from the launch
-# request.  After this is called, gdb will be ready to accept
-# breakpoint requests.  NAME is used as the test name.  It has a
-# reasonable default but can be overridden in case a test needs to
-# launch gdb more than once.
-proc dap_launch {file {name startup}} {
-    if {[_dap_initialize "$name - initialize"] == ""} {
+# request.  If specified, ARGS is a list of command-line arguments,
+# and ENV is a list of pairs of the form {VAR VALUE} that is used to
+# populate the inferior's environment.  After this is called, gdb will
+# be ready to accept breakpoint requests.
+proc dap_launch {file {args {}} {env {}}} {
+    if {[_dap_initialize "startup - initialize"] == ""} {
 	return ""
     }
-    return [dap_check_request_and_response "$name - launch" launch \
-		[format {o program [%s]} \
-		     [list s [standard_output_file $file]]]]
+    set params "o program"
+    append params " [format {[%s]} [list s [standard_output_file $file]]]"
+
+    if {[llength $args] > 0} {
+	append params " args"
+	set arglist ""
+	foreach arg $args {
+	    append arglist " \[s [list $arg]\]"
+	}
+	append params " \[a $arglist\]"
+    }
+
+    if {[llength $env] > 0} {
+	append params " env"
+	set envlist ""
+	foreach pair $env {
+	    lassign $pair var value
+	    append envlist " $var"
+	    append envlist " [format {[%s]} [list s $value]]"
+	}
+	append params " \[o $envlist\]"
+    }
+
+    return [dap_check_request_and_response "startup - launch" launch $params]
 }
 
 # Cleanly shut down gdb.  NAME is used as the test name.

-- 
2.40.0

Re: [PATCH 2/2] Add "args" and "env" parameters to DAP launch request

[Eli Zaretskii]

> Date: Tue, 09 May 2023 10:02:00 -0600
> From: Tom Tromey via Gdb-patches <gdb-patches@sourceware.org>
> 
> This patch augments the DAP launch request with some optional new
> parameters that let the client control the command-line arguments and
> the environment of the inferior.
> ---
>  gdb/doc/gdb.texinfo                | 13 +++++-
>  gdb/python/lib/gdb/dap/launch.py   | 20 ++++++++-
>  gdb/testsuite/gdb.dap/args-env.c   | 28 ++++++++++++
>  gdb/testsuite/gdb.dap/args-env.exp | 90 ++++++++++++++++++++++++++++++++++++++
>  gdb/testsuite/lib/dap-support.exp  | 39 +++++++++++++----
>  5 files changed, 178 insertions(+), 12 deletions(-)

Thanks.

> +@item env
> +If provided, this should be an object where each value is a string.

This doesn't say what should those string values be.  Shouldn't they
be of the format "@var{variable}=@var{value}"?

> +The environment of the inferior will be set to exactly as passed in,
> +as if by a sequence of invocations of @code{set environment} and
> +@code{unset environment}.  @xref{Environment}.

This could potentially confuse: if this is equivalent to "set
environment", then the variables are _added_ to the one inherited from
the parent process, no?  Also, when would this be equivalent to "unset
environment"?

Reviewed-By: Eli Zaretskii <eliz@gnu.org>

Re: [PATCH 1/2] Add attributes and methods to gdb.Inferior

[Eli Zaretskii]

> Date: Tue, 09 May 2023 10:01:59 -0600
> From: Tom Tromey via Gdb-patches <gdb-patches@sourceware.org>
> 
> This adds two new attributes and three new methods to gdb.Inferior.
> 
> The attributes let Python code see the command-line arguments and the
> name of "main".  Argument setting is also supported.
> 
> The methods let Python code manipulate the inferior's environment
> variables.
> ---
>  gdb/NEWS                                 |  10 ++
>  gdb/doc/python.texi                      |  43 ++++++++
>  gdb/python/py-inferior.c                 | 168 +++++++++++++++++++++++++++++++
>  gdb/testsuite/gdb.ada/py_range.exp       |   4 +
>  gdb/testsuite/gdb.python/py-inferior.exp |  36 +++++++
>  5 files changed, 261 insertions(+)

Thanks.

> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -170,6 +170,16 @@ info main
>       (program-counter) values, and can be used as the frame-id when
>       calling gdb.PendingFrame.create_unwind_info.
>  
> +  ** gdb.Inferior now has a new "arguments" attribute.  This holds the
> +     command-line arguments to the inferior, if known.
> +
> +  ** gdb.Inferior now has a new "main_name" attribute.  This holds the
> +     name of the inferior's "main", if known.
> +
> +  ** gdb.Inferior now has new methods "clear_env", "set_env", and
> +     "unset_env".  These can be used to modify the inferior's
> +     environment before it is started.
> +
>  *** Changes in GDB 13

This part is OK.

> +The environment that will be passed to the inferior can be changed
> +from Python.  These methods only take effect when the inferior is

"These methods" seems to allude to something that was described
before, but there's nothing.  So I would suggest either

  The methods described below only take effect when...

or

  The environment that will be passed to the inferior can be changed
  from Python by using the following methods.  These methods only take
  effect when...

Reviewed-By: Eli Zaretskii <eliz@gnu.org>

Re: [PATCH 1/2] Add attributes and methods to gdb.Inferior

[Andrew Burgess]

Tom Tromey via Gdb-patches <gdb-patches@sourceware.org> writes:

> This adds two new attributes and three new methods to gdb.Inferior.
>
> The attributes let Python code see the command-line arguments and the
> name of "main".  Argument setting is also supported.
>
> The methods let Python code manipulate the inferior's environment
> variables.
> ---
>  gdb/NEWS                                 |  10 ++
>  gdb/doc/python.texi                      |  43 ++++++++
>  gdb/python/py-inferior.c                 | 168 +++++++++++++++++++++++++++++++
>  gdb/testsuite/gdb.ada/py_range.exp       |   4 +
>  gdb/testsuite/gdb.python/py-inferior.exp |  36 +++++++
>  5 files changed, 261 insertions(+)
>
> diff --git a/gdb/NEWS b/gdb/NEWS
> index 6aa0d5171f2..8e260039b43 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -170,6 +170,16 @@ info main
>       (program-counter) values, and can be used as the frame-id when
>       calling gdb.PendingFrame.create_unwind_info.
>  
> +  ** gdb.Inferior now has a new "arguments" attribute.  This holds the
> +     command-line arguments to the inferior, if known.
> +
> +  ** gdb.Inferior now has a new "main_name" attribute.  This holds the
> +     name of the inferior's "main", if known.
> +
> +  ** gdb.Inferior now has new methods "clear_env", "set_env", and
> +     "unset_env".  These can be used to modify the inferior's
> +     environment before it is started.
> +
>  *** Changes in GDB 13
>  
>  * MI version 1 is deprecated, and will be removed in GDB 14.
> diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
> index 7c3a3ccd379..cd56a67e35a 100644
> --- a/gdb/doc/python.texi
> +++ b/gdb/doc/python.texi
> @@ -3446,10 +3446,30 @@ Boolean signaling whether the inferior was created using `attach', or
>  started by @value{GDBN} itself.
>  @end defvar
>  
> +@defvar Inferior.main_name
> +A string holding the name of this inferior's ``main'' function, if it
> +can be determined.  If the name of main is not known, this is
> +@code{None}.
> +@end defvar
> +
>  @defvar Inferior.progspace
>  The inferior's program space.  @xref{Progspaces In Python}.
>  @end defvar
>  
> +@defvar Inferior.arguments
> +The inferior's command line arguments, if known.  This corresponds to
> +the @code{set args} and @code{show args} commands.  @xref{Arguments}.
> +
> +When accessed, the value is a string holding all the arguments.  The
> +contents are quoted as they would be when passed to the shell.  If
> +there are no arguments, the value is @code{None}.
> +
> +Either a string or a sequence of strings can be assigned to this
> +attribute.  When a string is assigned, it is assumed to have any
> +necessary quoting for the shell; when a sequence is assigned, the
> +quoting is applied by @value{GDBN}.
> +@end defvar
> +
>  A @code{gdb.Inferior} object has the following methods:
>  
>  @defun Inferior.is_valid ()
> @@ -3517,6 +3537,29 @@ the same functionality, but use of @code{Inferior.thread_from_thread_handle}
>  is deprecated.
>  @end defun
>  
> +
> +The environment that will be passed to the inferior can be changed
> +from Python.  These methods only take effect when the inferior is
> +started -- they will not affect an inferior that is already executing.
> +
> +@findex Inferior.clear_env
> +@defun Inferior.clear_env ()
> +Clear the current environment variables that will be passed to this
> +inferior.
> +@end defun
> +
> +@findex Inferior.set_env
> +@defun Inferior.set_env (name, value)
> +Set the environment variable @var{name} to have the indicated value.
> +Both parameters must be strings.
> +@end defun
> +
> +@findex Inferior.unset_env
> +@defun Inferior.unset_env (name)
> +Unset the environment variable @var{name}.  @var{name} must be a
> +string.
> +@end defun
> +
>  @node Events In Python
>  @subsubsection Events In Python
>  @cindex inferior events in Python
> diff --git a/gdb/python/py-inferior.c b/gdb/python/py-inferior.c
> index 982d0f803a0..8f3703cc738 100644
> --- a/gdb/python/py-inferior.c
> +++ b/gdb/python/py-inferior.c
> @@ -770,6 +770,161 @@ infpy_repr (PyObject *obj)
>  			       inf->num, inf->pid);
>  }
>  
> +/* Implement clear_env.  */
> +
> +static PyObject *
> +infpy_clear_env (PyObject *obj)
> +{
> +  inferior_object *self = (inferior_object *) obj;
> +
> +  INFPY_REQUIRE_VALID (self);
> +
> +  self->inferior->environment.clear ();
> +  Py_RETURN_NONE;
> +}
> +
> +/* Implement set_env.  */
> +
> +static PyObject *
> +infpy_set_env (PyObject *obj, PyObject *args, PyObject *kw)
> +{
> +  inferior_object *self = (inferior_object *) obj;
> +  INFPY_REQUIRE_VALID (self);
> +
> +  const char *name, *val;
> +  static const char *keywords[] = { "name", "value", nullptr };
> +
> +  if (!gdb_PyArg_ParseTupleAndKeywords (args, kw, "ss", keywords,
> +					&name, &val))
> +    return nullptr;
> +
> +  self->inferior->environment.set (name, val);
> +  Py_RETURN_NONE;
> +}
> +
> +/* Implement unset_env.  */
> +
> +static PyObject *
> +infpy_unset_env (PyObject *obj, PyObject *args)
> +{
> +  inferior_object *self = (inferior_object *) obj;
> +  INFPY_REQUIRE_VALID (self);
> +
> +  const char *name;
> +  if (!PyArg_ParseTuple (args, "s", &name))

I'd really prefer gdb_PyArg_ParseTupleAndKeywords be used here too.  I
know there's only a single argument, but I think it would be nice if our
API was consistent in always accepting keywords.

> +    return nullptr;
> +
> +  self->inferior->environment.unset (name);
> +  Py_RETURN_NONE;
> +}
> +
> +/* Getter for "arguments".  */
> +
> +static PyObject *
> +infpy_get_args (PyObject *self, void *closure)
> +{
> +  inferior_object *inf = (inferior_object *) self;
> +
> +  INFPY_REQUIRE_VALID (inf);
> +
> +  const std::string &args = inf->inferior->args ();
> +  if (args.empty ())
> +    Py_RETURN_NONE;
> +
> +  return host_string_to_python_string (args.c_str ()).release ();
> +}
> +
> +/* Setter for "arguments".  */
> +
> +static int
> +infpy_set_args (PyObject *self, PyObject *value, void *closure)
> +{
> +  inferior_object *inf = (inferior_object *) self;
> +
> +  if (!inf->inferior)
> +    {
> +      PyErr_SetString (PyExc_RuntimeError, _("Inferior no longer exists."));
> +      return -1;
> +    }
> +
> +  if (value == nullptr)
> +    {
> +      PyErr_SetString (PyExc_TypeError, _("Cannot delete 'arguments' attribute."));

Line length?

> +      return -1;
> +    }
> +
> +  if (PyUnicode_Check (value))

I prefer gdbpy_is_string, which is used more.  Though PyUnicode_Check is
present in a few places.

> +    {
> +      gdb::unique_xmalloc_ptr<char> str = python_string_to_host_string (value);
> +      if (str == nullptr)
> +	return -1;
> +      inf->inferior->set_args (std::string (str.get ()));
> +    }
> +  else if (PySequence_Check (value))
> +    {
> +      std::vector<gdb::unique_xmalloc_ptr<char>> args;
> +      Py_ssize_t len = PySequence_Size (value);
> +      if (len == -1)
> +	return -1;
> +      for (Py_ssize_t i = 0; i < len; ++i)
> +	{
> +	  gdbpy_ref<> item (PySequence_ITEM (value, i));
> +	  if (item == nullptr)
> +	    return -1;
> +	  gdb::unique_xmalloc_ptr<char> str
> +	    = python_string_to_host_string (item.get ());
> +	  if (str == nullptr)
> +	    return -1;
> +	  args.push_back (std::move (str));
> +	}
> +      std::vector<char *> argvec;
> +      for (const auto &arg : args)
> +	argvec.push_back (arg.get ());
> +      gdb::array_view<char * const> view (argvec.data (), argvec.size ());
> +      inf->inferior->set_args (view);
> +    }
> +  else
> +    {
> +      PyErr_SetString (PyExc_TypeError,
> +		       _("string or sequence required for 'arguments'"));
> +      return -1;
> +    }
> +  return 0;
> +}
> +
> +/* Getter for "main_name".  */
> +
> +static PyObject *
> +infpy_get_main_name (PyObject *self, void *closure)
> +{
> +  inferior_object *inf = (inferior_object *) self;
> +
> +  INFPY_REQUIRE_VALID (inf);
> +
> +  const char *name = nullptr;
> +  try
> +    {
> +      /* This is unfortunate but the implementation of main_name can
> +	 reach into memory, among other things.  */
> +      scoped_restore_current_inferior restore_inferior;
> +      set_current_inferior (inf->inferior);
> +
> +      scoped_restore_current_program_space restore_current_progspace;
> +      set_current_program_space (inf->inferior->pspace);

I guess switch_to_inferior_no_thread would be overkill here, if all
we're doing is accessing inferior memory?  Though you do tease us with
"among other things", so hopefully nothing is using the incorrect
thread....

> +
> +      name = main_name ();
> +    }
> +  catch (const gdb_exception &except)
> +    {
> +      /* We can just ignore this.  */
> +      name = nullptr;

Is this assignment necessary, given name is initialised to nullptr?

Thanks,
Andrew

> +    }
> +
> +  if (name == nullptr)
> +    Py_RETURN_NONE;
> +
> +  return host_string_to_python_string (name).release ();
> +}
>  
>  static void
>  infpy_dealloc (PyObject *obj)
> @@ -844,6 +999,8 @@ GDBPY_INITIALIZE_FILE (gdbpy_initialize_inferior);
>  
>  static gdb_PyGetSetDef inferior_object_getset[] =
>  {
> +  { "arguments", infpy_get_args, infpy_set_args,
> +    "Arguments to this program.", nullptr },
>    { "num", infpy_get_num, NULL, "ID of inferior, as assigned by GDB.", NULL },
>    { "connection", infpy_get_connection, NULL,
>      "The gdb.TargetConnection for this inferior.", NULL },
> @@ -854,6 +1011,8 @@ static gdb_PyGetSetDef inferior_object_getset[] =
>    { "was_attached", infpy_get_was_attached, NULL,
>      "True if the inferior was created using 'attach'.", NULL },
>    { "progspace", infpy_get_progspace, NULL, "Program space of this inferior" },
> +  { "main_name", infpy_get_main_name, nullptr,
> +    "Name of 'main' function, if known.", nullptr },
>    { NULL }
>  };
>  
> @@ -889,6 +1048,15 @@ Return thread object corresponding to thread handle." },
>    { "architecture", (PyCFunction) infpy_architecture, METH_NOARGS,
>      "architecture () -> gdb.Architecture\n\
>  Return architecture of this inferior." },
> +  { "clear_env", (PyCFunction) infpy_clear_env, METH_NOARGS,
> +    "clear_env () -> None\n\
> +Clear environment of this inferior." },
> +  { "set_env", (PyCFunction) infpy_set_env, METH_VARARGS | METH_KEYWORDS,
> +    "set_env (name, value) -> None\n\
> +Set an environment variable of this inferior." },
> +  { "unset_env", infpy_unset_env, METH_VARARGS,
> +    "unset_env (name) -> None\n\
> +Unset an environment of this inferior." },
>    { NULL }
>  };
>  
> diff --git a/gdb/testsuite/gdb.ada/py_range.exp b/gdb/testsuite/gdb.ada/py_range.exp
> index 2972db21827..833493bad95 100644
> --- a/gdb/testsuite/gdb.ada/py_range.exp
> +++ b/gdb/testsuite/gdb.ada/py_range.exp
> @@ -42,3 +42,7 @@ gdb_test "python print(gdb.parse_and_eval('si').type)" \
>      "foo\\.small_integer" "print type"
>  gdb_test "python print(gdb.parse_and_eval('si').type.name)" \
>      "foo\\.small_integer" "print type name"
> +
> +gdb_test "python print(gdb.selected_inferior().main_name)" \
> +    "_ada_foo" \
> +    "print main name"
> diff --git a/gdb/testsuite/gdb.python/py-inferior.exp b/gdb/testsuite/gdb.python/py-inferior.exp
> index 424050a6166..58deece5d5f 100644
> --- a/gdb/testsuite/gdb.python/py-inferior.exp
> +++ b/gdb/testsuite/gdb.python/py-inferior.exp
> @@ -342,3 +342,39 @@ with_test_prefix "architecture" {
>  	"True" \
>  	"inferior architecture matches frame architecture"
>  }
> +
> +gdb_test "python print(gdb.selected_inferior().main_name)" \
> +    "main" \
> +    "print main name"
> +
> +gdb_test_no_output "set args x y z"
> +gdb_test "python print(gdb.selected_inferior().arguments)" \
> +    "x y z" \
> +    "print arguments"
> +
> +gdb_test_no_output "python gdb.selected_inferior().arguments = 'a b c'" \
> +    "set arguments from string"
> +gdb_test "show args" \
> +    [string_to_regexp "Argument list to give program being debugged when it is started is \"a b c\"."] \
> +    "show args from string"
> +
> +gdb_test_no_output "python gdb.selected_inferior().arguments = \['a', 'b c'\]" \
> +    "set arguments from list"
> +gdb_test "show args" \
> +    [string_to_regexp "Argument list to give program being debugged when it is started is \"a b\\ c\"."] \
> +    "show args from list"
> +
> +gdb_test_no_output "python gdb.selected_inferior().clear_env()" \
> +    "clear environment"
> +gdb_test_no_output "show environment"
> +
> +gdb_test_no_output "python gdb.selected_inferior().set_env('DEI', 'value')" \
> +    "set environment variable"
> +gdb_test "show environment" \
> +    "DEI=value" \
> +    "examine environment variable"
> +
> +gdb_test_no_output "python gdb.selected_inferior().unset_env('DEI')" \
> +    "unset environment variable"
> +gdb_test_no_output "show environment" \
> +    "environment is empty again"
>
> -- 
> 2.40.0

Re: [PATCH 2/2] Add "args" and "env" parameters to DAP launch request

[Andrew Burgess]

Tom Tromey via Gdb-patches <gdb-patches@sourceware.org> writes:

> This patch augments the DAP launch request with some optional new
> parameters that let the client control the command-line arguments and
> the environment of the inferior.

I haven't followed the DAP additions much, but the code changes in here
look reasonable to me.

Reviewed-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew




> ---
>  gdb/doc/gdb.texinfo                | 13 +++++-
>  gdb/python/lib/gdb/dap/launch.py   | 20 ++++++++-
>  gdb/testsuite/gdb.dap/args-env.c   | 28 ++++++++++++
>  gdb/testsuite/gdb.dap/args-env.exp | 90 ++++++++++++++++++++++++++++++++++++++
>  gdb/testsuite/lib/dap-support.exp  | 39 +++++++++++++----
>  5 files changed, 178 insertions(+), 12 deletions(-)
>
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index 8c4177c1901..343612060c6 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -38998,10 +38998,21 @@ Generally, @value{GDBN} implements the Debugger Adapter Protocol as
>  written.  However, in some cases, extensions are either needed or even
>  expected.
>  
> -@value{GDBN} defines a parameter that can be passed to the
> +@value{GDBN} defines some parameters that can be passed to the
>  @code{launch} request:
>  
>  @table @code
> +@item args
> +If provided, this should be an array of strings.  These strings are
> +provided as command-line arguments to the inferior, as if by
> +@code{set args}.  @xref{Arguments}.
> +
> +@item env
> +If provided, this should be an object where each value is a string.
> +The environment of the inferior will be set to exactly as passed in,
> +as if by a sequence of invocations of @code{set environment} and
> +@code{unset environment}.  @xref{Environment}.
> +
>  @item program
>  If provided, this is a string that specifies the program to use.  This
>  corresponds to the @code{file} command.  @xref{Files}.
> diff --git a/gdb/python/lib/gdb/dap/launch.py b/gdb/python/lib/gdb/dap/launch.py
> index b4102cc28cc..21499a339e1 100644
> --- a/gdb/python/lib/gdb/dap/launch.py
> +++ b/gdb/python/lib/gdb/dap/launch.py
> @@ -13,20 +13,36 @@
>  # You should have received a copy of the GNU General Public License
>  # along with this program.  If not, see <http://www.gnu.org/licenses/>.
>  
> +import gdb
>  from .events import ExecutionInvoker
>  from .server import request, capability
> -from .startup import send_gdb
> +from .startup import send_gdb, in_gdb_thread
>  
>  
>  _program = None
>  
>  
> +@in_gdb_thread
> +def _set_args_env(args, env):
> +    inf = gdb.selected_inferior()
> +    inf.arguments = args
> +    if env is not None:
> +        inf.clear_env()
> +        for name, value in env.items():
> +            inf.set_env(name, value)
> +
> +
> +# Any parameters here are necessarily extensions -- DAP requires this
> +# from implementations.  Any additions or changes here should be
> +# documented in the gdb manual.
>  @request("launch")
> -def launch(*, program=None, **args):
> +def launch(*, program=None, args=[], env=None, **extra):
>      if program is not None:
>          global _program
>          _program = program
>          send_gdb(f"file {_program}")
> +    if len(args) > 0 or env is not None:
> +        send_gdb(lambda: _set_args_env(args, env))
>  
>  
>  @capability("supportsConfigurationDoneRequest")
> diff --git a/gdb/testsuite/gdb.dap/args-env.c b/gdb/testsuite/gdb.dap/args-env.c
> new file mode 100644
> index 00000000000..bc7f1d4b38e
> --- /dev/null
> +++ b/gdb/testsuite/gdb.dap/args-env.c
> @@ -0,0 +1,28 @@
> +/* 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/>.  */
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +
> +int
> +main (int argc, char *argv[])
> +{
> +  const char *value = getenv ("DEI");
> +  const char *no_value = getenv ("NOSUCHVARIABLE");
> +
> +  return 0; /* BREAK */
> +}
> diff --git a/gdb/testsuite/gdb.dap/args-env.exp b/gdb/testsuite/gdb.dap/args-env.exp
> new file mode 100644
> index 00000000000..96fbb28d9ce
> --- /dev/null
> +++ b/gdb/testsuite/gdb.dap/args-env.exp
> @@ -0,0 +1,90 @@
> +# 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 environment variables and command line arguments via DAP.
> +
> +require allow_dap_tests !use_gdb_stub
> +
> +load_lib dap-support.exp
> +
> +standard_testfile
> +
> +if {[build_executable ${testfile}.exp $testfile] == -1} {
> +    return
> +}
> +
> +if {[dap_launch $testfile {a "b c"} {{DEI something}}] == ""} {
> +    return
> +}
> +
> +set line [gdb_get_line_number "BREAK"]
> +set obj [dap_check_request_and_response "set breakpoint by line number" \
> +	     setBreakpoints \
> +	     [format {o source [o path [%s]] breakpoints [a [o line [i %d]]]} \
> +		  [list s $srcfile] $line]]
> +set line_bpno [dap_get_breakpoint_number $obj]
> +
> +dap_check_request_and_response "start inferior" configurationDone
> +dap_wait_for_event_and_check "inferior started" thread "body reason" started
> +
> +dap_wait_for_event_and_check "stopped at line breakpoint" stopped \
> +    "body reason" breakpoint \
> +    "body hitBreakpointIds" $line_bpno
> +
> +set bt [lindex [dap_check_request_and_response "backtrace" stackTrace \
> +		    {o threadId [i 1]}] \
> +	    0]
> +set frame_id [dict get [lindex [dict get $bt body stackFrames] 0] id]
> +
> +set obj [dap_check_request_and_response \
> +	     "evaluate argc in function" \
> +	     evaluate [format {o expression [s argc] frameId [i %s]} \
> +			   $frame_id]]
> +dap_match_values "argc in function" [lindex $obj 0] \
> +    "body result" 3
> +
> +set obj [dap_check_request_and_response \
> +	     "evaluate first argument in function" \
> +	     evaluate [format {o expression [s {argv[1]}] frameId [i %s]} \
> +			   $frame_id]]
> +set val [dict get [lindex $obj 0] body result]
> +# This ends up with some extra quoting.
> +gdb_assert {[string first "\\\"a\\\"" $val] != -1} \
> +    "first argument in function"
> +
> +set obj [dap_check_request_and_response \
> +	     "evaluate second argument in function" \
> +	     evaluate [format {o expression [s {argv[2]}] frameId [i %s]} \
> +			   $frame_id]]
> +set val [dict get [lindex $obj 0] body result]
> +# This ends up with some extra quoting.
> +gdb_assert {[string first "\\\"b c\\\"" $val] != -1} \
> +    "second argument in function"
> +
> +set obj [dap_check_request_and_response "evaluate value in function" \
> +	     evaluate [format {o expression [s value] frameId [i %s]} \
> +			   $frame_id]]
> +set val [dict get [lindex $obj 0] body result]
> +# This ends up with some extra quoting.
> +gdb_assert {[string first "\\\"something\\\"" $val] != -1} \
> +    "value in function"
> +
> +set obj [dap_check_request_and_response "evaluate no_value in function" \
> +	     evaluate [format {o expression [s no_value] frameId [i %s]} \
> +			   $frame_id]]
> +dap_match_values "no_value in function" [lindex $obj 0] \
> +    "body result" 0
> +
> +dap_shutdown
> diff --git a/gdb/testsuite/lib/dap-support.exp b/gdb/testsuite/lib/dap-support.exp
> index 6bb9b6e6377..ead295bdbfe 100644
> --- a/gdb/testsuite/lib/dap-support.exp
> +++ b/gdb/testsuite/lib/dap-support.exp
> @@ -236,17 +236,38 @@ proc _dap_initialize {name} {
>  # Start gdb, send a DAP initialize request, and then a launch request
>  # specifying FILE as the program to use for the inferior.  Returns the
>  # empty string on failure, or the response object from the launch
> -# request.  After this is called, gdb will be ready to accept
> -# breakpoint requests.  NAME is used as the test name.  It has a
> -# reasonable default but can be overridden in case a test needs to
> -# launch gdb more than once.
> -proc dap_launch {file {name startup}} {
> -    if {[_dap_initialize "$name - initialize"] == ""} {
> +# request.  If specified, ARGS is a list of command-line arguments,
> +# and ENV is a list of pairs of the form {VAR VALUE} that is used to
> +# populate the inferior's environment.  After this is called, gdb will
> +# be ready to accept breakpoint requests.
> +proc dap_launch {file {args {}} {env {}}} {
> +    if {[_dap_initialize "startup - initialize"] == ""} {
>  	return ""
>      }
> -    return [dap_check_request_and_response "$name - launch" launch \
> -		[format {o program [%s]} \
> -		     [list s [standard_output_file $file]]]]
> +    set params "o program"
> +    append params " [format {[%s]} [list s [standard_output_file $file]]]"
> +
> +    if {[llength $args] > 0} {
> +	append params " args"
> +	set arglist ""
> +	foreach arg $args {
> +	    append arglist " \[s [list $arg]\]"
> +	}
> +	append params " \[a $arglist\]"
> +    }
> +
> +    if {[llength $env] > 0} {
> +	append params " env"
> +	set envlist ""
> +	foreach pair $env {
> +	    lassign $pair var value
> +	    append envlist " $var"
> +	    append envlist " [format {[%s]} [list s $value]]"
> +	}
> +	append params " \[o $envlist\]"
> +    }
> +
> +    return [dap_check_request_and_response "startup - launch" launch $params]
>  }
>  
>  # Cleanly shut down gdb.  NAME is used as the test name.
>
> -- 
> 2.40.0

Re: [PATCH 2/2] Add "args" and "env" parameters to DAP launch request

[Tom Tromey]

>> +@item env
>> +If provided, this should be an object where each value is a string.

Eli> This doesn't say what should those string values be.  Shouldn't they
Eli> be of the format "@var{variable}=@var{value}"?

In JSON an object is a dictionary; so the keys are strings and the
values are as well.  That is it looks like {"variable": "value"}

Tom

Re: [PATCH 2/2] Add "args" and "env" parameters to DAP launch request

[Eli Zaretskii]

> From: Tom Tromey <tromey@adacore.com>
> Cc: Tom Tromey <tromey@adacore.com>,  gdb-patches@sourceware.org
> Date: Tue, 16 May 2023 13:06:09 -0600
> 
> >> +@item env
> >> +If provided, this should be an object where each value is a string.
> 
> Eli> This doesn't say what should those string values be.  Shouldn't they
> Eli> be of the format "@var{variable}=@var{value}"?
> 
> In JSON an object is a dictionary; so the keys are strings and the
> values are as well.  That is it looks like {"variable": "value"}

That's okay, but I think we should say something about the structure
of those strings, and {"variable": "value"} goes a long way towards
that.

Re: [PATCH 2/2] Add "args" and "env" parameters to DAP launch request

[Eli Zaretskii]

> Date: Tue, 16 May 2023 22:10:54 +0300
> Cc: gdb-patches@sourceware.org
> X-Spam-Status: No, score=1.7 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH,
>  DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF,
>  RCVD_IN_BARRACUDACENTRAL, SPF_HELO_PASS, SPF_PASS, TXREP,
>  T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no version=3.4.6
> From: Eli Zaretskii via Gdb-patches <gdb-patches@sourceware.org>
> 
> > From: Tom Tromey <tromey@adacore.com>
> > Cc: Tom Tromey <tromey@adacore.com>,  gdb-patches@sourceware.org
> > Date: Tue, 16 May 2023 13:06:09 -0600
> > 
> > >> +@item env
> > >> +If provided, this should be an object where each value is a string.
> > 
> > Eli> This doesn't say what should those string values be.  Shouldn't they
> > Eli> be of the format "@var{variable}=@var{value}"?
> > 
> > In JSON an object is a dictionary; so the keys are strings and the
> > values are as well.  That is it looks like {"variable": "value"}
> 
> That's okay, but I think we should say something about the structure
> of those strings, and {"variable": "value"} goes a long way towards
           ^^^^^^^
That should have been "objects", sorry.

> that.

Re: [PATCH 1/2] Add attributes and methods to gdb.Inferior

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:

>> +  const char *name;
>> +  if (!PyArg_ParseTuple (args, "s", &name))

Andrew> I'd really prefer gdb_PyArg_ParseTupleAndKeywords be used here too.  I
Andrew> know there's only a single argument, but I think it would be nice if our
Andrew> API was consistent in always accepting keywords.

Ok, I did this.

gdb doesn't generally do this for single-argument functions, but it's
also relatively harmless to do so ("relatively" since it does mean the
name is ABI).

>> +  if (value == nullptr)
>> +    {
>> +      PyErr_SetString (PyExc_TypeError, _("Cannot delete 'arguments' attribute."));

Andrew> Line length?

Fixed.

>> +      return -1;
>> +    }
>> +
>> +  if (PyUnicode_Check (value))

Andrew> I prefer gdbpy_is_string, which is used more.  Though
Andrew> PyUnicode_Check is present in a few places.

I made this change, but it seems to me that gdbpy_is_string was probably
there for Python 2/3 compatibility.  Now that it's just a simple wrapper
for PyUnicode_Check, it seems like it would be more "Pythonically
idiomatic" to just remove the wrapper.

>> +      /* This is unfortunate but the implementation of main_name can
>> +	 reach into memory, among other things.  */
>> +      scoped_restore_current_inferior restore_inferior;
>> +      set_current_inferior (inf->inferior);
>> +
>> +      scoped_restore_current_program_space restore_current_progspace;
>> +      set_current_program_space (inf->inferior->pspace);

Andrew> I guess switch_to_inferior_no_thread would be overkill here, if all
Andrew> we're doing is accessing inferior memory?  Though you do tease us with
Andrew> "among other things", so hopefully nothing is using the incorrect
Andrew> thread....

I removed the "among other things" bit.  I think the thread shouldn't
really matter.

The only offender here is Ada and there's a bug open about how it would
be better to get this info from the executable instead.

>> +
>> +      name = main_name ();
>> +    }
>> +  catch (const gdb_exception &except)
>> +    {
>> +      /* We can just ignore this.  */
>> +      name = nullptr;

Andrew> Is this assignment necessary, given name is initialised to nullptr?

Apparently not, I removed it in v2.

Tom

Re: [PATCH 1/2] Add attributes and methods to gdb.Inferior

[Tom Tromey]

>>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:

>> +The environment that will be passed to the inferior can be changed
>> +from Python.  These methods only take effect when the inferior is

Eli> "These methods" seems to allude to something that was described
Eli> before, but there's nothing.  So I would suggest either

Eli>   The methods described below only take effect when...

Eli> or

Eli>   The environment that will be passed to the inferior can be changed
Eli>   from Python by using the following methods.  These methods only take
Eli>   effect when...

I made this change in v2.

Eli> Reviewed-By: Eli Zaretskii <eliz@gnu.org>

Tom