[v2,3/5] Create MI commands using python.

Message ID 20190514112418.24091-4-jan.vrany@fit.cvut.cz
State New
Headers show
Series
  • Untitled series #13313
Related show

Commit Message

Jan Vrany May 14, 2019, 11:24 a.m.
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

    * 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.
---
 gdb/ChangeLog                |  23 +++
 gdb/Makefile.in              |   1 +
 gdb/mi/mi-cmds.c             |   7 +-
 gdb/mi/mi-cmds.h             |   9 ++
 gdb/python/py-micmd.c        | 300 +++++++++++++++++++++++++++++++++++
 gdb/python/py-micmd.h        |  51 ++++++
 gdb/python/python-internal.h |   2 +
 gdb/python/python.c          |  13 +-
 8 files changed, 401 insertions(+), 5 deletions(-)
 create mode 100644 gdb/python/py-micmd.c
 create mode 100644 gdb/python/py-micmd.h

-- 
2.20.1

Comments

Simon Marchi May 17, 2019, 4:29 a.m. | #1
On 2019-05-14 7:24 a.m., 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

> 

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

> ---

>  gdb/ChangeLog                |  23 +++

>  gdb/Makefile.in              |   1 +

>  gdb/mi/mi-cmds.c             |   7 +-

>  gdb/mi/mi-cmds.h             |   9 ++

>  gdb/python/py-micmd.c        | 300 +++++++++++++++++++++++++++++++++++

>  gdb/python/py-micmd.h        |  51 ++++++

>  gdb/python/python-internal.h |   2 +

>  gdb/python/python.c          |  13 +-

>  8 files changed, 401 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 03eb42555e..4cbe97002b 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 44f3813fdc..7e1d2c3f2d 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 fc432a16b9..a2757bae20 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>

> +

>  enum print_values {

>     PRINT_NO_VALUES,

>     PRINT_ALL_VALUES,

> @@ -190,9 +192,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);

> +

>  #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..c8d429bb4b

> --- /dev/null

> +++ b/gdb/python/py-micmd.c

> @@ -0,0 +1,300 @@

> +/* 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/>.  */

> +

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

> +

> +extern PyTypeObject

> +  micmdpy_object_type CPYCHECKER_TYPE_OBJECT_FOR_TYPEDEF ("micmdpy_object");

> +

> +/* 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, and can be contained within Python

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

> +   and/or dictionaries.  */

> +

> +static void

> +parse_mi_result (PyObject *result, const char *field_name)

> +{

> +  struct ui_out *uiout = current_uiout;

> +

> +  if (PyString_Check (result))

> +    {

> +      goto generic;


Hmm is this goto really necessary?  Can't you just remove this if, and execution will
go directly in the else?  Or is a string considered a sequence by PySequence_Check?

In any case, there's gotta be a better way to organize this that doesn't require the goto.

> +    }

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

> +	{

> +	  gdb::unique_xmalloc_ptr<char> key_string (gdbpy_obj_to_string (key));


When parsing a dict, I think it would be good to enforce that keys are strings, I
don't really see the use case to allow arbitrary objects there.  In the MI, keys
will be strings, and I don't really see the use case of letting the user do

return {
  MyObject(): "foo"
}

where the str(MyObject()) will be used as the key.  By validating that keys are
strings, I think we can help the user catch errors earlier in development.

> +	  parse_mi_result (value, key_string.get ());

> +	}

> +    }

> +  else if (PySequence_Check (result))

> +    {

> +      PyObject *item;

> +      Py_ssize_t i = 0;


Declare these variables in the most specific scope possible (the for loop header for i,
in the for loop for item).

> +      ui_out_emit_list list_emitter (uiout, field_name);

> +      for (i = 0; i < PySequence_Size (result); ++i)

> +	{

> +	  item = PySequence_GetItem (result, i);


PySequence_GetItem returns a new reference, which must be dropped.

Also, you could probably use PySequence_ITEM which will be a bit faster.  It should be
safe, since we know result is a PySequence and we don't use negative indices.

> +	  parse_mi_result (item, 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);

> +	}


Remove extra curly braces.

> +    }

> +  else

> +    {

> +    generic:

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

> +

> +   Use: __init__(NAME).

> +

> +   NAME is the name of the MI command to register.  It must start with a dash

> +   as a traditional MI commands do.  */


"as traditional MI commands", drop the "a"

> +

> +static int

> +micmdpy_init (PyObject *self, PyObject *args, PyObject *kw)

> +{

> +  const char *name;

> +

> +  if (!PyArg_ParseTuple (args, "s", &name))

> +    return -1;

> +

> +  /* Validate command name */

> +  const int name_len = strlen (name);

> +  if (name_len < 2)


This should probably name_len == 0, to match the error message.

> +    {

> +      PyErr_Format (PyExc_RuntimeError, _("MI command name is empty"));

> +      return -1;

> +    }

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

> +    {

> +      PyErr_Format (PyExc_RuntimeError,

> +		    _("MI command name does not start with '-'"

> +		      " followed by a letter or digit"));

> +      return -1;

> +    }

Could all these error messages be just calls to error(), handled by the catch below?

We would then raise some gdb.error exceptions instead of RuntimeError, maybe it's desirable?

> +  else

> +    for (int i = 2; i < name_len; i++)

> +      {

> +	if (!isalnum (name[i]) && name[i] != '-')

> +	  {

> +	    PyErr_Format (PyExc_RuntimeError,

> +			  _("MI command name contains invalid character: %c"),

> +			  name[i]);

> +	    return -1;

> +	  }

> +      }

> +

> +  gdbpy_ref<> self_ref = gdbpy_ref<>::new_reference (self);

> +  try

> +    {

> +      mi_command *micommand = new mi_command_py (name + 1, NULL, self_ref);

> +

> +      bool result = insert_mi_cmd_entry (std::move (mi_cmd_up (micommand)));


Make micommand a mi_cmd_up directly:

  mi_cmd_up micommand (new mi_command_py (...));

> +

> +      if (!result)

> +	{

> +	  PyErr_Format (

> +	    PyExc_RuntimeError,

> +	    _("Unable to insert command. The name might already be in use."));

> +	  return -1;

> +	}

> +    }

> +  catch (const gdb_exception &except)

> +    {

> +      GDB_PY_SET_HANDLE_EXCEPTION (except);

> +    }

> +

> +  return 0;

> +}

> +

> +mi_command_py::mi_command_py (const char *name, int *suppress_notification,

> +			      gdbpy_ref<> object)

> +  : mi_command (name, suppress_notification), pyobj (object)

> +{

> +}

> +

> +void

> +mi_command_py::invoke (struct mi_parse *parse)

> +{

> +  std::unique_ptr<scoped_restore_tmpl<int>> restore

> +    = do_suppress_notification ();

> +

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

> +  int i;

> +

> +  gdbpy_enter enter_py (get_current_arch (), current_language);

> +

> +  if (!obj)

> +    error (_("-%s: invalid invocation of Python micommand object."),

> +	   name ().c_str ());


Can this condition happen because of a mistake by the user, or it would only be because
of a logic error in GDB?  If it's the latter, it should be a gdb_assert (obj != nullptr);

> +

> +  if (!PyObject_HasAttr (obj, invoke_cst))

> +    {

> +      error (_("-%s: python command object missing 'invoke' method."),

> +	     name ().c_str ());

> +    }


I would make that check (presence of invoke method) when the command is registered.  It would
help users catch mistakes earlier.   Here, it could just be a gdb_assert to confirm it exists.

> +

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

> +    }


It would be nice to be consistent in how we write "Python" in our messages to the user.
I suggest "Python", with the capital letter, as it's how the language is named.

> +

> +  for (i = 0; i < parse->argc; ++i)


Declare i in the for loop header.

> +    {

> +      /* Since PyList_SetItem steals the reference, we don't use

> +       * gdbpy_ref<> to hold on arg string. */

> +      PyObject *str = PyUnicode_Decode (parse->argv[i], strlen (parse->argv[i]),

> +					host_charset (), NULL);

> +      if (PyList_SetItem (argobj.get (), i, str) != 0)


Actually, I would suggest using gdbpy_ref<>, but then releasing it:

  PyList_SetItem (..., str.release ());

It illustrates better the transfer of ownership, and there's no window where
the reference is unmanaged.

Just wondering, because I know that unicode handling is very different between
Python 2 and 3: did you test with both Python versions?

> +	{

> +	  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 (result != nullptr)

> +    {

> +      if (Py_None != result)

> +	parse_mi_result (result.get (), "result");

> +    }

> +  else

> +    {

> +      error (

> +	_("-%s: command invoke() method returned NULL but no python exception "

> +	  "is set"),

> +	name ().c_str ());


I am curious, did you actually hit this case, where no Python exception was
raised and invoke returned NULL?  I thought the two were pretty coupled
(if return value is NULL, it means there's an exception raise and vice versa).

> +    }

> +}

> +

> +void mi_command_py::finalize ()

> +{

> +  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 = {

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

> +  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..418de41a10

> --- /dev/null

> +++ b/gdb/python/py-micmd.h

> @@ -0,0 +1,51 @@

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

> +

> +/* MI command implemented on top of a Python command.  */


I find this description a bit misleading, what's a Python command?
The only thing "Python command" can refer to is the gdb.Command class, use
to implement CLI commands in Python, but these Python MI commands are not
related.  I would suggest instead:

/* MI command implemented in Python.  */

> +class mi_command_py : public mi_command

> +{

> +public:

> +  mi_command_py (const char *name, int *suppress_notification,

> +		 gdbpy_ref<> object);


If the suppress_notification parameter is not used for Python commands, you can remove it.

Could you document the constructor?  The OBJECT parameter in particular is a bit opaque.

> +  void invoke (struct mi_parse *parse) override;


This should be private, like in the previous patch.

> +

> +  /* This is called just before shutting down a Python interpreter

> +     to release python object implementing the command */

> +  void finalize ();

> +

> +private:

> +  gdbpy_ref<> pyobj;

> +};

> +

> +#endif

> diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h

> index 449926ca87..26606b4b36 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..ee89c603a5 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 (auto const& name_and_cmd : mi_cmd_table)


Use:

  for (const auto &name_and_cmd : mi_cmd_table)

> +    {

> +      mi_command    *cmd = name_and_cmd.second.get ();

> +      mi_command_py *cmd_py = dynamic_cast<mi_command_py*> (cmd);


Don't align variables like this, use regular formatting.

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

> 


Simon
Jan Vrany May 28, 2019, 8:35 p.m. | #2
Hi,

thanks a lot! I agree with most of comments and fixed 
them already in upcoming v3 patch. 

For the rest, sae below. 

> > > +    {

> > > +      /* Since PyList_SetItem steals the reference, we don't use

> > > +       * gdbpy_ref<> to hold on arg string. */

> > > +      PyObject *str = PyUnicode_Decode (parse->argv[i], strlen (parse->argv[i]),

> > > +                                     host_charset (), NULL);

> > > +      if (PyList_SetItem (argobj.get (), i, str) != 0)

> > 

> > Just wondering, because I know that unicode handling is very different between

> > Python 2 and 3: did you test with both Python versions?

> > 


No. I will do that before sending v3. 

> > > +  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 (result != nullptr)

> > > +    {

> > > +      if (Py_None != result)

> > > +     parse_mi_result (result.get (), "result");

> > > +    }

> > > +  else

> > > +    {

> > > +      error (

> > > +     _("-%s: command invoke() method returned NULL but no python exception "

> > > +       "is set"),

> > > +     name ().c_str ());

> > 

> > I am curious, did you actually hit this case, where no Python exception was

> > raised and invoke returned NULL?  I thought the two were pretty coupled

> > (if return value is NULL, it means there's an exception raise and vice versa).

> > 


Yes, it looks like. I guess I took this code from Didier. I removed the else
branch in v3. 

> > > +  void invoke (struct mi_parse *parse) override;

> > 

> > This should be private, like in the previous patch.

> > 


Ah, this is "bigger" problem, we should override 
do_invoke() which is declared protected in superclass. Again,fixed in v3. 

Thanks a lot!

Jan

Patch

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 03eb42555e..4cbe97002b 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 44f3813fdc..7e1d2c3f2d 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 fc432a16b9..a2757bae20 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>
+
 enum print_values {
    PRINT_NO_VALUES,
    PRINT_ALL_VALUES,
@@ -190,9 +192,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);
+
 #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..c8d429bb4b
--- /dev/null
+++ b/gdb/python/py-micmd.c
@@ -0,0 +1,300 @@ 
+/* 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/>.  */
+
+/* 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;
+
+extern PyTypeObject
+  micmdpy_object_type CPYCHECKER_TYPE_OBJECT_FOR_TYPEDEF ("micmdpy_object");
+
+/* 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, and can be contained within Python
+   lists and dictionaries. It is possible to have a multiple levels of lists
+   and/or dictionaries.  */
+
+static void
+parse_mi_result (PyObject *result, const char *field_name)
+{
+  struct ui_out *uiout = current_uiout;
+
+  if (PyString_Check (result))
+    {
+      goto generic;
+    }
+  else 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))
+	{
+	  gdb::unique_xmalloc_ptr<char> key_string (gdbpy_obj_to_string (key));
+	  parse_mi_result (value, key_string.get ());
+	}
+    }
+  else if (PySequence_Check (result))
+    {
+      PyObject *item;
+      Py_ssize_t i = 0;
+      ui_out_emit_list list_emitter (uiout, field_name);
+      for (i = 0; i < PySequence_Size (result); ++i)
+	{
+	  item = PySequence_GetItem (result, i);
+	  parse_mi_result (item, 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
+    {
+    generic:
+      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.
+
+   Use: __init__(NAME).
+
+   NAME is the name of the MI command to register.  It must start with a dash
+   as a traditional MI commands do.  */
+
+static int
+micmdpy_init (PyObject *self, PyObject *args, PyObject *kw)
+{
+  const char *name;
+
+  if (!PyArg_ParseTuple (args, "s", &name))
+    return -1;
+
+  /* Validate command name */
+  const int name_len = strlen (name);
+  if (name_len < 2)
+    {
+      PyErr_Format (PyExc_RuntimeError, _("MI command name is empty"));
+      return -1;
+    }
+  else if ((name[0] != '-') || !isalnum (name[1]))
+    {
+      PyErr_Format (PyExc_RuntimeError,
+		    _("MI command name does not start with '-'"
+		      " followed by a letter or digit"));
+      return -1;
+    }
+  else
+    for (int i = 2; i < name_len; i++)
+      {
+	if (!isalnum (name[i]) && name[i] != '-')
+	  {
+	    PyErr_Format (PyExc_RuntimeError,
+			  _("MI command name contains invalid character: %c"),
+			  name[i]);
+	    return -1;
+	  }
+      }
+
+  gdbpy_ref<> self_ref = gdbpy_ref<>::new_reference (self);
+  try
+    {
+      mi_command *micommand = new mi_command_py (name + 1, NULL, self_ref);
+
+      bool result = insert_mi_cmd_entry (std::move (mi_cmd_up (micommand)));
+
+      if (!result)
+	{
+	  PyErr_Format (
+	    PyExc_RuntimeError,
+	    _("Unable to insert command. The name might already be in use."));
+	  return -1;
+	}
+    }
+  catch (const gdb_exception &except)
+    {
+      GDB_PY_SET_HANDLE_EXCEPTION (except);
+    }
+
+  return 0;
+}
+
+mi_command_py::mi_command_py (const char *name, int *suppress_notification,
+			      gdbpy_ref<> object)
+  : mi_command (name, suppress_notification), pyobj (object)
+{
+}
+
+void
+mi_command_py::invoke (struct mi_parse *parse)
+{
+  std::unique_ptr<scoped_restore_tmpl<int>> restore
+    = do_suppress_notification ();
+
+  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 ();
+  int i;
+
+  gdbpy_enter enter_py (get_current_arch (), current_language);
+
+  if (!obj)
+    error (_("-%s: invalid invocation of Python micommand object."),
+	   name ().c_str ());
+
+  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 (i = 0; i < parse->argc; ++i)
+    {
+      /* Since PyList_SetItem steals the reference, we don't use
+       * gdbpy_ref<> to hold on arg string. */
+      PyObject *str = PyUnicode_Decode (parse->argv[i], strlen (parse->argv[i]),
+					host_charset (), NULL);
+      if (PyList_SetItem (argobj.get (), i, str) != 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 (result != nullptr)
+    {
+      if (Py_None != result)
+	parse_mi_result (result.get (), "result");
+    }
+  else
+    {
+      error (
+	_("-%s: command invoke() method returned NULL but no python exception "
+	  "is set"),
+	name ().c_str ());
+    }
+}
+
+void mi_command_py::finalize ()
+{
+  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 = {
+  PyVarObject_HEAD_INIT (NULL, 0) "gdb.MICommand", /*tp_name */
+  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..418de41a10
--- /dev/null
+++ b/gdb/python/py-micmd.h
@@ -0,0 +1,51 @@ 
+/* 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;
+
+/* MI command implemented on top of a Python command.  */
+class mi_command_py : public mi_command
+{
+public:
+  mi_command_py (const char *name, int *suppress_notification,
+		 gdbpy_ref<> object);
+  void invoke (struct mi_parse *parse) override;
+
+  /* This is called just before shutting down a Python interpreter
+     to release python object implementing the command */
+  void finalize ();
+
+private:
+  gdbpy_ref<> pyobj;
+};
+
+#endif
diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
index 449926ca87..26606b4b36 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..ee89c603a5 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 (auto const& name_and_cmd : mi_cmd_table)
+    {
+      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)	\