Re: [PATCH v3 3/5] Create MI commands using python.

[Pedro Alves <palves at redhat dot com>]

On 5/30/19 2:48 PM, Jan Vrany wrote:
> From: Didier Nadeau <didier.nadeau@gmail.com>
> 
> This commit allows an user to create custom MI commands using Python
> similarly to what is possible for Python CLI commands.
> 
> A new subclass of mi_command is defined for Python MI commands,
> mi_command_py. A new file, py-micmd.c contains the logic for Python
> MI commands.
> 
> gdb/ChangeLog
> 

>     * python/py-micmd.h: New file
>     (micmdpy_object): New struct.
>     (micmdpy_object): New typedef.

This typedef shouldn't be necessary?

>     (mi_command_py): New class.
>     * python/python-internal.h
>     (gdbpy_initialize_micommands): New declaration.
>     * python/python.c
>     (_initialize_python): New call to gdbpy_initialize_micommands.
>     (finalize_python): Finalize Python MI commands.
> ---
>  gdb/ChangeLog                |  23 +++
>  gdb/Makefile.in              |   1 +
>  gdb/mi/mi-cmds.c             |   7 +-
>  gdb/mi/mi-cmds.h             |   9 ++
>  gdb/python/py-micmd.c        | 297 +++++++++++++++++++++++++++++++++++
>  gdb/python/py-micmd.h        |  60 +++++++
>  gdb/python/python-internal.h |   2 +
>  gdb/python/python.c          |  13 +-
>  8 files changed, 407 insertions(+), 5 deletions(-)
>  create mode 100644 gdb/python/py-micmd.c
>  create mode 100644 gdb/python/py-micmd.h
> 
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 994ad947bd..241e5da68f 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,26 @@
> +2019-05-02  Didier Nadeau  <didier.nadeau@gmail.com>
> +            Jan Vrany  <jan.vrany@fit.cvut.cz>
> +
> +	* Makefile.in (SUBDIR_PYTHON_SRCS): Add py-micmd.c.
> +	* mi/mi-cmds.c (insert_mi_cmd_entry): Remove static.
> +	(mi_cmd_table): Remove static.
> +	* mi/mi-cmds.h (insert_mi_cmd_entry): New declaration.
> +	(mi_cmd_table): New declaration.
> +	* python/py-micmd.c: New file
> +	(parse_mi_result): New function.
> +	(micmdpy_init): New function.
> +	(gdbpy_initialize_micommands): New function.
> +	(mi_command_py): New class.
> +	* python/py-micmd.h: New file
> +	(micmdpy_object): New struct.
> +	(micmdpy_object): New typedef.
> +	(mi_command_py): New class.
> +	* python/python-internal.h
> +	(gdbpy_initialize_micommands): New declaration.
> +	* python/python.c
> +	(_initialize_python): New call to gdbpy_initialize_micommands.
> +	(finalize_python): Finalize Python MI commands.
> +
>  2019-05-02  Didier Nadeau  <didier.nadeau@gmail.com>
>  	    Jan Vrany <jan.vrany@fit.cvut.cz>
>  
> diff --git a/gdb/Makefile.in b/gdb/Makefile.in
> index 0f49578360..28866962bc 100644
> --- a/gdb/Makefile.in
> +++ b/gdb/Makefile.in
> @@ -382,6 +382,7 @@ SUBDIR_PYTHON_SRCS = \
>  	python/py-instruction.c \
>  	python/py-lazy-string.c \
>  	python/py-linetable.c \
> +	python/py-micmd.c \
>  	python/py-newobjfileevent.c \
>  	python/py-objfile.c \
>  	python/py-param.c \
> diff --git a/gdb/mi/mi-cmds.c b/gdb/mi/mi-cmds.c
> index 2d8863c5f9..0aead954f9 100644
> --- a/gdb/mi/mi-cmds.c
> +++ b/gdb/mi/mi-cmds.c
> @@ -28,12 +28,11 @@
>  
>  /* MI command table (built at run time). */
>  
> -static std::map<std::string, mi_cmd_up> mi_cmd_table;
> +std::map<std::string, mi_cmd_up> mi_cmd_table;
>  
> -/* Insert a new mi-command into the command table.  Return true if
> -   insertion was successful.  */
> +/* See mi-cmds.h.  */
>  
> -static bool
> +bool
>  insert_mi_cmd_entry (mi_cmd_up command)
>  {
>    gdb_assert (command != NULL);
> diff --git a/gdb/mi/mi-cmds.h b/gdb/mi/mi-cmds.h
> index f1b4728bde..5ca7232fca 100644
> --- a/gdb/mi/mi-cmds.h
> +++ b/gdb/mi/mi-cmds.h
> @@ -22,6 +22,8 @@
>  #ifndef MI_MI_CMDS_H
>  #define MI_MI_CMDS_H
>  
> +#include <map>

Include <string> as well.

> +
>  enum print_values {
>     PRINT_NO_VALUES,
>     PRINT_ALL_VALUES,
> @@ -191,9 +193,16 @@ typedef std::unique_ptr<mi_command> mi_cmd_up;
>  
>  extern mi_command *mi_cmd_lookup (const char *command);
>  
> +extern std::map<std::string, mi_cmd_up> mi_cmd_table;
> +
>  /* Debug flag */
>  extern int mi_debug_p;
>  
>  extern void mi_execute_command (const char *cmd, int from_tty);
>  
> +/* Insert a new mi-command into the command table.  Return true if
> +   insertion was successful.  */
> +
> +extern bool insert_mi_cmd_entry (mi_cmd_up command);

Use "mi_cmd_up &&command" ?

> +
>  #endif /* MI_MI_CMDS_H */
> diff --git a/gdb/python/py-micmd.c b/gdb/python/py-micmd.c
> new file mode 100644
> index 0000000000..e99632c97a
> --- /dev/null
> +++ b/gdb/python/py-micmd.c
> @@ -0,0 +1,297 @@
> +/* MI Command Set for GDB, the GNU debugger.
> +
> +   Copyright (C) 2019 Free Software Foundation, Inc.

If any of this code was posted in previous years, please write
201x-2019 for the year range (with x replaced with the proper
digit, of course).  Likewise all other files in the patch.

> +
> +   This file is part of GDB.
> +
> +   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/>.  */
> +
> +/* gdb MI commands implemented in Python  */
> +
> +#include "defs.h"
> +#include "python-internal.h"
> +#include "python/py-micmd.h"
> +#include "arch-utils.h"
> +#include "charset.h"
> +#include "language.h"
> +
> +#include <string>
> +
> +static PyObject *invoke_cst;

Document.

> +
> +extern PyTypeObject
> +  micmdpy_object_type CPYCHECKER_TYPE_OBJECT_FOR_TYPEDEF ("micmdpy_object");

Is there a reason this is "extern"?  I don't see this used anywhere
outside py-micmd.c.


> +
> +/* If the command invoked returns a list, this function parses it and create an
> +   appropriate MI out output.
> +
> +   The returned values must be Python string,

"values ... string" doesn't parse correctly for me.

Did you mean

   "The returned value must be a Python string

?

>  and can be contained within Python
> +   lists and dictionaries. 

I think I'm a bit more confused now.  :-)

Can you clarify this please?

> +   lists and dictionaries. It is possible to have a multiple levels of lists

Double space after periods.

"a multiple levels" => "multiple levels" 


> +   and/or dictionaries.  */
> +
> +static void
> +parse_mi_result (PyObject *result, const char *field_name)
> +{
> +  struct ui_out *uiout = current_uiout;
> +
> +  if (PyDict_Check (result))
> +    {
> +      PyObject *key, *value;
> +      Py_ssize_t pos = 0;
> +      ui_out_emit_tuple tuple_emitter (uiout, field_name);
> +      while (PyDict_Next (result, &pos, &key, &value))
> +	{
> +	  if (!PyString_Check (key))
> +	    {
> +	      gdbpy_ref<> key_repr (PyObject_Repr (key));
> +	      if (PyErr_Occurred () != NULL)
> +                {
> +                  gdbpy_err_fetch ex;
> +                  gdb::unique_xmalloc_ptr<char> ex_msg (ex.to_string ());
> +
> +                  if (ex_msg == NULL || *ex_msg == '\0')
> +                    error (_("Non-string object used as key."));
> +                  else
> +                    error (_("Non-string object used as key: %s."),
> +                           ex_msg.get ());
> +                }
> +              else
> +	        {
> +	          auto key_repr_string
> +	                 = python_string_to_target_string (key_repr.get ());
> +	          error (_("Non-string object used as key: %s."),
> +                         key_repr_string.get ());
> +	        }
> +	    }

There's aspace vs tabs mixup above, and in other parts
of the file too.  Please fix that throughout.

> +
> +	  auto key_string = python_string_to_target_string (key);
> +	  parse_mi_result (value, key_string.get ());
> +	}
> +    }
> +  else if (PySequence_Check (result) && !PyString_Check (result))
> +    {
> +      ui_out_emit_list list_emitter (uiout, field_name);
> +      for (Py_ssize_t i = 0; i < PySequence_Size (result); ++i)
> +	{
> +          gdbpy_ref<> item (PySequence_ITEM (result, i));
> +	  parse_mi_result (item.get (), NULL);
> +	}
> +    }
> +  else if (PyIter_Check (result))
> +    {
> +      gdbpy_ref<> item;
> +      ui_out_emit_list list_emitter (uiout, field_name);
> +      while (item.reset (PyIter_Next (result)), item != nullptr)
> +	parse_mi_result (item.get (), NULL);
> +    }
> +  else
> +    {
> +      gdb::unique_xmalloc_ptr<char> string (gdbpy_obj_to_string (result));
> +      uiout->field_string (field_name, string.get ());
> +    }
> +}
> +
> +/* Object initializer; sets up gdb-side structures for MI command.

"for an MI command", I suppose?

> +
> +   Use: __init__(NAME).

Did you mean "Usage:"?  Or that this function implements
__init__?

> +
> +   NAME is the name of the MI command to register.  It must start with a dash
> +   as traditional MI commands do.  */
> +
> +static int
> +micmdpy_init (PyObject *self, PyObject *args, PyObject *kw)
> +{
> +  const char *name;
> +  gdbpy_ref<> self_ref = gdbpy_ref<>::new_reference (self);
> +
> +  if (!PyArg_ParseTuple (args, "s", &name))
> +    return -1;
> +
> +  /* Validate command name */

Period and double space.

> +  const int name_len = strlen (name);
> +  if (name_len == 0)
> +    {
> +      error (_("MI command name is empty."));
> +      return -1;

the return line is unreacheable, since error throws.
Remove it, and then remove the unnecessary braces.

> +    }
> +  else if ((name_len < 2) || (name[0] != '-') || !isalnum (name[1]))

Please remove reduntant parens:

    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;

Likewise remove the return line.

> +    }
> +  else

Since the conditions above if reached never return you
could drop the "else"s, which removes this indentation level.
Also, I'd use "== 1" instead of "< 2", since you've already
checked 0.

Like:

  if (name_len == 0)
    error (_("MI command name is empty."));

  if (name_len == 1 || name[0] != '-' || !isalnum name[1])
    error (_("MI command name does not start with '-'"
             " followed by at least one letter or digit."));

  for (int i = 2; i < name_len; i++)
    {
  ...

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

Ditto re. the return after error.

> +	  }
> +      }
> +
> +  if (!PyObject_HasAttr (self_ref.get (), invoke_cst))
> +      error (_("-%s: Python command object missing 'invoke' method."), name);

Indentation of "error" doesn't look right.

> +
> +  try
> +    {
> +      mi_cmd_up micommand = mi_cmd_up(new mi_command_py (name + 1, self_ref));
> +
> +      bool result = insert_mi_cmd_entry (std::move (micommand));
> +
> +      if (!result)
> +	{
> +	  error (_("Unable to insert command."
> +                   "The name might already be in use."));

Indentation isn't right here.

> +	  return -1;

Remove, unreachable.

> +	}
> +    }
> +  catch (const gdb_exception &except)
> +    {
> +      GDB_PY_SET_HANDLE_EXCEPTION (except);
> +    }
> +
> +  return 0;
> +}
> +
> +mi_command_py::mi_command_py (const char *name, gdbpy_ref<> object)
> +  : mi_command (name, NULL), pyobj (object)
> +{
> +}
> +
> +void
> +mi_command_py::do_invoke (struct mi_parse *parse)
> +{
> +  mi_parse_argv (parse->args, parse);
> +
> +  if (parse->argv == NULL)
> +    error (_("Problem parsing arguments: %s %s"), parse->command, parse->args);
> +
> +  PyObject *obj = this->pyobj.get ();
> +
> +  gdbpy_enter enter_py (get_current_arch (), current_language);
> +
> +  gdb_assert (obj != nullptr);
> +
> +  if (!PyObject_HasAttr (obj, invoke_cst))
> +      error (_("-%s: Python command object missing 'invoke' method."),
> +	     name ().c_str ());
> +
> +
> +  gdbpy_ref<> argobj (PyList_New (parse->argc));
> +  if (argobj == nullptr)
> +    {
> +      gdbpy_print_stack ();
> +      error (_("-%s: failed to create the Python arguments list."),
> +	     name ().c_str ());
> +    }
> +
> +  for (int i = 0; i < parse->argc; ++i)
> +    {
> +      gdbpy_ref<> str (PyUnicode_Decode (parse->argv[i], strlen (parse->argv[i]),
> +					 host_charset (), NULL));
> +      if (PyList_SetItem (argobj.get (), i, str.release ()) != 0)
> +	{
> +	  error (_("-%s: failed to create the Python arguments list."),
> +		 name ().c_str ());
> +	}
> +    }
> +
> +  gdb_assert (PyErr_Occurred () == NULL);
> +  gdbpy_ref<> result (
> +    PyObject_CallMethodObjArgs (obj, invoke_cst, argobj.get (), NULL));
> +  if (PyErr_Occurred () != NULL)
> +    {
> +      gdbpy_err_fetch ex;
> +      gdb::unique_xmalloc_ptr<char> ex_msg (ex.to_string ());
> +
> +      if (ex_msg == NULL || *ex_msg == '\0')
> +	error (_("-%s: failed to execute command"), name ().c_str ());
> +      else
> +	error (_("-%s: %s"), name ().c_str (), ex_msg.get ());
> +    }
> +  else
> +    {
> +      if (Py_None != result)
> +	parse_mi_result (result.get (), "result");
> +    }
> +}
> +
> +void mi_command_py::finalize ()

Line break after void.

> +{
> +  this->pyobj.reset (nullptr);
> +}
> +
> +/* Initialize the MI command object.  */
> +
> +int
> +gdbpy_initialize_micommands ()
> +{
> +  micmdpy_object_type.tp_new = PyType_GenericNew;
> +  if (PyType_Ready (&micmdpy_object_type) < 0)
> +    return -1;
> +
> +  if (gdb_pymodule_addobject (gdb_module, "MICommand",
> +			      (PyObject *) &micmdpy_object_type)
> +      < 0)
> +    return -1;
> +
> +  invoke_cst = PyString_FromString ("invoke");
> +  if (invoke_cst == NULL)
> +    return -1;
> +
> +  return 0;
> +}
> +
> +static PyMethodDef micmdpy_object_methods[] = {{0}};
> +
> +PyTypeObject micmdpy_object_type = {

Can this be static?

> +  PyVarObject_HEAD_INIT (NULL, 0) "gdb.MICommand", /*tp_name */

Line break between the PyVarObject_HEAD_INIT statement
and "gdb.MICommand".


> +  sizeof (micmdpy_object),			   /*tp_basicsize */
> +  0,						   /*tp_itemsize */
> +  0,						   /*tp_dealloc */
> +  0,						   /*tp_print */
> +  0,						   /*tp_getattr */
> +  0,						   /*tp_setattr */
> +  0,						   /*tp_compare */
> +  0,						   /*tp_repr */
> +  0,						   /*tp_as_number */
> +  0,						   /*tp_as_sequence */
> +  0,						   /*tp_as_mapping */
> +  0,						   /*tp_hash */
> +  0,						   /*tp_call */
> +  0,						   /*tp_str */
> +  0,						   /*tp_getattro */
> +  0,						   /*tp_setattro */
> +  0,						   /*tp_as_buffer */
> +  Py_TPFLAGS_DEFAULT | Py_TPFLAGS_BASETYPE,	/*tp_flags */
> +  "GDB mi-command object",			   /* tp_doc */
> +  0,						   /* tp_traverse */
> +  0,						   /* tp_clear */
> +  0,						   /* tp_richcompare */
> +  0,						   /* tp_weaklistoffset */
> +  0,						   /* tp_iter */
> +  0,						   /* tp_iternext */
> +  micmdpy_object_methods,			   /* tp_methods */
> +  0,						   /* tp_members */
> +  0,						   /* tp_getset */
> +  0,						   /* tp_base */
> +  0,						   /* tp_dict */
> +  0,						   /* tp_descr_get */
> +  0,						   /* tp_descr_set */
> +  0,						   /* tp_dictoffset */
> +  micmdpy_init,					   /* tp_init */
> +  0,						   /* tp_alloc */
> +};
> diff --git a/gdb/python/py-micmd.h b/gdb/python/py-micmd.h
> new file mode 100644
> index 0000000000..deadc0116b
> --- /dev/null
> +++ b/gdb/python/py-micmd.h
> @@ -0,0 +1,60 @@
> +/* MI Command Set for GDB, the GNU debugger.
> +
> +   Copyright (C) 2019 Free Software Foundation, Inc.
> +
> +   This file is part of GDB.
> +
> +   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/>.  */
> +
> +#ifndef PY_MICMDS_H
> +#define PY_MICMDS_H
> +
> +#include "mi/mi-cmds.h"
> +#include "mi/mi-parse.h"
> +#include "python-internal.h"
> +#include "python/py-ref.h"
> +
> +struct micmdpy_object
> +{
> +  PyObject_HEAD
> +};
> +
> +typedef struct micmdpy_object micmdpy_object;

This typedef shouldn't be needed.

> +
> +/* MI command implemented in Python.  */
> +
> +class mi_command_py : public mi_command
> +{
> +  public:

Indentation of "public" etc. should be under "class".
I think you fixed other files in previous iterations but
this was left behind.

> +    /* 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). */
> +
> +    mi_command_py (const char *name, gdbpy_ref<> object);
> +
> +
> +    /* This is called just before shutting down a Python interpreter
> +       to release python object implementing the command. */

..."to release THE python"...

Double space after period.


> +
> +    void finalize ();
> +
> +  protected:
> +   virtual void do_invoke(struct mi_parse *parse) override;

Missing space before parens.  Drop the "virtual".

> +
> +  private:
> +    gdbpy_ref<> pyobj;

Should be called "m_pyobj".  And should have a describing comment.

> +};
> +
> +#endif
> diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
> index 69ff1fe30d..f28027741e 100644
> --- a/gdb/python/python-internal.h
> +++ b/gdb/python/python-internal.h
> @@ -539,6 +539,8 @@ int gdbpy_initialize_xmethods (void)
>    CPYCHECKER_NEGATIVE_RESULT_SETS_EXCEPTION;
>  int gdbpy_initialize_unwind (void)
>    CPYCHECKER_NEGATIVE_RESULT_SETS_EXCEPTION;
> +int gdbpy_initialize_micommands (void)
> +  CPYCHECKER_NEGATIVE_RESULT_SETS_EXCEPTION;
>  
>  /* A wrapper for PyErr_Fetch that handles reference counting for the
>     caller.  */
> diff --git a/gdb/python/python.c b/gdb/python/python.c
> index 4dad8ec10d..901c0a4edc 100644
> --- a/gdb/python/python.c
> +++ b/gdb/python/python.c
> @@ -36,6 +36,8 @@
>  #include <ctype.h>
>  #include "location.h"
>  #include "ser-event.h"
> +#include "mi/mi-cmds.h"
> +#include "py-micmd.h"
>  
>  /* Declared constants and enum for python stack printing.  */
>  static const char python_excp_none[] = "none";
> @@ -1564,6 +1566,14 @@ finalize_python (void *ignore)
>    python_gdbarch = target_gdbarch ();
>    python_language = current_language;
>  
> +  for (const auto& name_and_cmd : mi_cmd_table)

The "&" should be leaning on the right side, like
we format "*":

  const auto &name_and_cmd


> +    {
> +      mi_command *cmd = name_and_cmd.second.get ();
> +      mi_command_py *cmd_py = dynamic_cast<mi_command_py*> (cmd);
> +      if (cmd_py != nullptr)
> +        cmd_py->finalize ();
> +    }
> +
>    Py_Finalize ();
>  
>    restore_active_ext_lang (previous_active);
> @@ -1698,7 +1708,8 @@ do_start_initialization ()
>        || gdbpy_initialize_event () < 0
>        || gdbpy_initialize_arch () < 0
>        || gdbpy_initialize_xmethods () < 0
> -      || gdbpy_initialize_unwind () < 0)
> +      || gdbpy_initialize_unwind () < 0
> +      || gdbpy_initialize_micommands () < 0)
>      return false;
>  
>  #define GDB_PY_DEFINE_EVENT_TYPE(name, py_name, doc, base)	\
> 

Thanks,
Pedro Alves