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

  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(+)
  

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

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
  

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

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