From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 116261 invoked by alias); 17 May 2019 04:29:30 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 115709 invoked by uid 89); 17 May 2019 04:29:30 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-18.1 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_SHORT,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.1 spammy=am, a.m, UD:a.m, coupled X-HELO: simark.ca Received: from simark.ca (HELO simark.ca) (158.69.221.121) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 17 May 2019 04:29:26 +0000 Received: from [10.0.0.11] (unknown [192.222.164.54]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 8E2381E0A9; Fri, 17 May 2019 00:29:23 -0400 (EDT) Subject: Re: [PATCH v2 3/5] Create MI commands using python. To: Jan Vrany , gdb-patches@sourceware.org Cc: Didier Nadeau References: <20190418152337.6376-1-jan.vrany@fit.cvut.cz> <20190514112418.24091-4-jan.vrany@fit.cvut.cz> From: Simon Marchi Message-ID: <128d0fc3-4364-0cab-f03d-45ed7d3eda32@simark.ca> Date: Fri, 17 May 2019 04:29:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.6.1 MIME-Version: 1.0 In-Reply-To: <20190514112418.24091-4-jan.vrany@fit.cvut.cz> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2019-05/txt/msg00407.txt.bz2 On 2019-05-14 7:24 a.m., Jan Vrany wrote: > From: Didier Nadeau > > 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 > + Jan Vrany > + > + * 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 > Jan Vrany > > 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 mi_cmd_table; > +std::map 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 > + > enum print_values { > PRINT_NO_VALUES, > PRINT_ALL_VALUES, > @@ -190,9 +192,16 @@ typedef std::unique_ptr mi_cmd_up; > > extern mi_command *mi_cmd_lookup (const char *command); > > +extern std::map 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 . */ > + > +/* 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 > + > +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 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 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> 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 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 . */ > + > +#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 > #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 (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