Re: [PATCHv2] gdb/python/mi: create MI commands using python

[Simon Marchi <simon.marchi@polymtl.ca>]

> +@defun MICommand.invoke (arguments)
> +This method is called by @value{GDBN} when the new MI command is
> +invoked.
> +
> +@var{arguments} is a list of strings.  Note, that @code{--thread}
> +and @code{--frame} arguments are handled by @value{GDBN} itself therefore
> +they do not show up in @code{arguments}.
> +
> +If this method throws a @code{gdb.GdbError} exception, it is turned
> +into a @sc{GDB/MI} @code{^error} response.  If this method returns

Hmm, not only gdb.GdbError, but all exceptions will result in a ^error,
I think?

> +@code{None}, then the @sc{GDB/MI} command will return a @code{^done}
> +response with no additional values.
> +
> +Otherwise, the return value must be a dictionary, which is converted
> +to a @sc{GDB/MI} @var{RESULT-RECORD} (@pxref{GDB/MI Output Syntax}).
> +The keys of this dictionary must be strings, and are used as
> +@emph{VARIABLE} names in the @emph{RESULT-RECORD}, these strings must
> +comply with the naming rules detailed below.  The values of this
> +dictionary are recursively handled as follows:
> +
> +@itemize
> +@item If the value is Python sequence or iterator, it is converted to
> +@sc{GDB/MI} @emph{LIST} with elements converted recursively.
> +
> +@item If the value is Python dictionary, it is converted to
> +@sc{GDB/MI} @emph{TUPLE}.  Keys in that dictionary must be strings,
> +which comply with the @emph{VARIABLE} naming rules detailed below.
> +Values are converted recursively.
> +
> +@item Otherwise, value is first converted to a Python string using
> +@code{str ()} and then converted to @sc{GDB/MI} @emph{CONST}.
> +@end itemize
> +
> +The strings used for @emph{VARIABLE} names in the @sc{GDB/MI} output
> +must follow C variable naming rules; the string must be at least one
> +character long, the first character must be in the set
> +@code{[a-zA-Z]}, while every subsequent character must be in the set
> +@code{[a-zA-Z0-9_]}.

Well, in C, identifiers can start with an underscore.  Not that I need
to use it.

> @@ -113,12 +109,12 @@ struct mi_command_cli : public mi_command
>     not have been added to mi_cmd_table.  Otherwise, return true, and
>     COMMAND was added to mi_cmd_table.  */

Change this to /* See ... */.  Also, this contains a bit more details
than what was put in the .h, notably the reason why the function can
fail.  It would be nice to preserve that.

>  
> -static bool
> +bool
>  insert_mi_cmd_entry (mi_command_up command)
>  {
>    gdb_assert (command != nullptr);
>  
> -  const std::string &name = command->name ();
> +  const std::string name (command->name ());

Is this change needed?

>  
>    if (mi_cmd_table.find (name) != mi_cmd_table.end ())
>      return false;
> @@ -127,6 +123,20 @@ insert_mi_cmd_entry (mi_command_up command)
>    return true;
>  }
>  
> +bool
> +remove_mi_cmd_entry (mi_command *command)
> +{
> +  gdb_assert (command != nullptr);

If command can't be nullptr, it could be a reference.

> diff --git a/gdb/python/lib/gdb/__init__.py b/gdb/python/lib/gdb/__init__.py
> index 9734a0d9437..e2d2ab10467 100644
> --- a/gdb/python/lib/gdb/__init__.py
> +++ b/gdb/python/lib/gdb/__init__.py
> @@ -82,6 +82,9 @@ frame_filters = {}
>  # Initial frame unwinders.
>  frame_unwinders = []
>  
> +# Hash containing all user created MI commands, the key is the command
> +# name, and the value is the gdb.MICommand object.
> +_mi_commands = {}

I would say "Dict" instead of "Hash", since that's how they are called
in Python.

>  
>  def _execute_unwinders(pending_frame):
>      """Internal function called from GDB to execute all unwinders.
> diff --git a/gdb/python/py-micmd.c b/gdb/python/py-micmd.c
> new file mode 100644
> index 00000000000..e289785c61e
> --- /dev/null
> +++ b/gdb/python/py-micmd.c
> @@ -0,0 +1,815 @@
> +/* MI Command Set for GDB, the GNU debugger.
> +
> +   Copyright (C) 2019 Free Software Foundation, Inc.

Wrong year?

> +/* Representation of a python gdb.MICommand object.  */
> +
> +struct micmdpy_object
> +{
> +  PyObject_HEAD
> +
> +  /* The object representing this command in the mi command table.  This
> +     pointer can be nullptr if the command is not currently installed into
> +     the mi command table (see gdb.MICommand.installed property).  */
> +  struct mi_command_py *mi_command;
> +
> +  /* The string representing the name of this command, without the leading
> +     dash.  This string is never nullptr once the python object has been

In comments, python -> Python.

> +     initialised.
> +
> +     The memory for this string was allocated with malloc, and needs to be
> +     deallocated with free when the python object is deallocated.
> +
> +     When the MI_COMMAND variable is not nullptr, then the mi_command_py

variable -> field?

> +     object's name will point back to this string.  */
> +  char *mi_command_name;
> +};
> +
> +/* The mi command implemented in python.  */

In comments, mi -> MI.

> +
> +struct mi_command_py : public mi_command
> +{
> +  /* Constructs a new mi_command_py object.  NAME is command name without
> +     leading dash.  OBJECT is a reference to a Python object implementing
> +     the command.  This object should inherit from gdb.MICommand and should
> +     implement method invoke (args). */

Two spaces at the end.

Also, should -> must?

> +
> +  mi_command_py (const char *name, micmdpy_object *object)
> +    : mi_command (name, nullptr),
> +      m_pyobj (object)
> +  {
> +    pymicmd_debug_printf ("this = %p", this);
> +  }
> +
> +  ~mi_command_py ()
> +  {
> +    /* The python object representing a mi command contains a pointer back
> +       to this c++ object.  We can safely set this pointer back to nullptr
> +       now, to indicate the python object no longer references a valid c++
> +       object.
> +
> +       However, the python object also holds the storage for our name
> +       string.  We can't clear that here as our parent's destructor might
> +       still want to reference that string.  Instead we rely on the python
> +       object deallocator to free that memory, and reset the pointer.  */
> +    m_pyobj->mi_command = nullptr;
> +
> +    pymicmd_debug_printf ("this = %p", this);
> +  };
> +
> +  /* Validate that CMD_OBJ, a non-nullptr pointer, is installed into the mi
> +     command table correctly.  This function looks up the command in the mi
> +     command table and checks that the object we get back references
> +     CMD_OBJ.  This function is only intended for calling within a
> +     gdb_assert.  This function performs many assertions internally, and
> +     then always returns true.  */
> +  static bool validate_installation (micmdpy_object *cmd_obj);

If validate_installation does some gdb_asserts and then returns true, I
don't really see the point of calling it inside a gdb_assert.  You can
just call it normally:

  validate_installation (cmd);

> +/* Parse RESULT and print it in mi format to the current_uiout.
> +
> +   This function handles the top-level result initially returned from the
> +   invoke method of the Python command implementation.  At the top-level
> +   the result must be a dictionary.  The values within this dictionary can
> +   be a wider range of types.  Handling the values of the top-level
> +   dictionary is done by parse_mi_result_1, see that function for more
> +   details.
> +
> +   If anything goes wrong while parsing and printing the mi output then an
> +   error is thrown.  */
> +
> +static void
> +parse_mi_result (PyObject *result)

Naming nit: I don't think parse is the appropriate word.  We're doing
the opposite here, so maybe use "serialize"?  Parsing would be to read
the text version of an MI result, to create an object hierarchy.

> +/* Return a reference to the gdb.mi_commands dictionary.  */
> +
> +static gdbpy_ref<>
> +micmdpy_global_command_dictionary ()
> +{
> +  if (gdb_python_module == nullptr
> +      || ! PyObject_HasAttrString (gdb_python_module, "_mi_commands"))
> +    error (_("unable to find gdb.mi_commands dictionary"));
> +
> +  gdbpy_ref<> mi_cmd_dict (PyObject_GetAttrString (gdb_python_module,
> +						   "_mi_commands"));
> +  if (mi_cmd_dict == nullptr || !PyDict_Check (mi_cmd_dict.get ()))
> +    error (_("unable to fetch gdb.mi_commands dictionary"));

gdb.mi_commands -> gdb._mi_commands (twice in error messages, one in a
comment)?

I wonder if calling PyObject_HasAttrString is necessary, since you call
PyObject_GetAttrString just after.

> +/* Implement gdb.MICommand.__init__.  The init method takes the name of
> +   the mi command as the first argument, which must be a string, starting
> +   with a single dash.  */
> +
> +static int
> +micmdpy_init (PyObject *self, PyObject *args, PyObject *kwargs)
> +{
> +  PYMICMD_SCOPED_DEBUG_ENTER_EXIT;
> +
> +  micmdpy_object *cmd = (micmdpy_object *) self;
> +
> +  static const char *keywords[] = { "name", nullptr };
> +  const char *name;
> +
> +  if (!gdb_PyArg_ParseTupleAndKeywords (args, kwargs, "s", keywords,
> +					&name))
> +    return -1;
> +
> +  /* Validate command name */
> +  const int name_len = strlen (name);
> +  if (name_len == 0)
> +    {
> +      error (_("MI command name is empty."));

I think that error() calls in this function should be replaced with
setting the appropriate Python exception type.  For example, the above
should raise a ValueError.

> +      return -1;
> +    }
> +  else if ((name_len < 2) || (name[0] != '-') || !isalnum (name[1]))
> +    {
> +      error (_("MI command name does not start with '-'"
> +               " followed by at least one letter or digit."));
> +      return -1;
> +    }
> +  else
> +    {
> +      for (int i = 2; i < name_len; i++)
> +	{
> +	  if (!isalnum (name[i]) && name[i] != '-')
> +	    {
> +	      error (_("MI command name contains invalid character: %c."),
> +		     name[i]);
> +	      return -1;
> +	    }
> +	}
> +
> +      /* Skip over the leading dash.  For the rest of this function the
> +	 dash is not important.  */
> +      ++name;
> +    }
> +
> +  /* Check that there's an 'invoke' method.  */
> +  if (!PyObject_HasAttr (self, invoke_cst))
> +    error (_("-%s: Python command object missing 'invoke' method."), name);
> +
> +  /* If this object already has a name set, then this object has been
> +     initialized before.  We handle this case a little differently.  */
> +  if (cmd->mi_command_name != nullptr)

Huh, how can this happen?

> diff --git a/gdb/testsuite/gdb.python/py-mi-cmd.exp b/gdb/testsuite/gdb.python/py-mi-cmd.exp
> new file mode 100644
> index 00000000000..dd8012f1f7a
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/py-mi-cmd.exp
> @@ -0,0 +1,286 @@
> +# Copyright (C) 2019-2022 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 custom MI commands implemented in Python.
> +
> +load_lib gdb-python.exp
> +load_lib mi-support.exp
> +set MIFLAGS "-i=mi2"

Any reason to use mi2 specifically?

> diff --git a/gdb/testsuite/gdb.python/py-mi-cmd.py b/gdb/testsuite/gdb.python/py-mi-cmd.py
> new file mode 100644
> index 00000000000..031c5307589
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/py-mi-cmd.py
> @@ -0,0 +1,100 @@
> +# Copyright (C) 2019-2022 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/>.
> +
> +import gdb
> +
> +class BadKey:
> +    def __repr__(self):
> +        return "Bad Key"
> +
> +class ReallyBadKey:
> +    def __repr__(self):
> +        return BadKey()
> +
> +
> +class pycmd1(gdb.MICommand):
> +    def invoke(self, argv):
> +        if argv[0] == 'int':
> +            return { 'result': 42 }
> +        elif argv[0] == 'str':
> +            return { 'result': "Hello world!" }

Don't forget to run black.

Simon