From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 35693 invoked by alias); 18 Jun 2019 19:43:00 -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 35685 invoked by uid 89); 18 Jun 2019 19:43:00 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_SHORT,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.1 spammy=H*M:4641 X-HELO: mail-wm1-f65.google.com Received: from mail-wm1-f65.google.com (HELO mail-wm1-f65.google.com) (209.85.128.65) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 18 Jun 2019 19:42:56 +0000 Received: by mail-wm1-f65.google.com with SMTP id v19so4456538wmj.5 for ; Tue, 18 Jun 2019 12:42:56 -0700 (PDT) Return-Path: Received: from ?IPv6:2001:8a0:f913:f700:56ee:75ff:fe8d:232b? ([2001:8a0:f913:f700:56ee:75ff:fe8d:232b]) by smtp.gmail.com with ESMTPSA id x129sm3682700wmg.44.2019.06.18.12.42.53 (version=TLS1_3 cipher=AEAD-AES128-GCM-SHA256 bits=128/128); Tue, 18 Jun 2019 12:42:53 -0700 (PDT) Subject: Re: [PATCH v3 3/5] Create MI commands using python. To: Jan Vrany , gdb-patches@sourceware.org References: <20190128124101.26243-1-jan.vrany@fit.cvut.cz> <20190530134850.3236-4-jan.vrany@fit.cvut.cz> Cc: Didier Nadeau From: Pedro Alves Message-ID: <23e65e69-7c94-3681-4641-c5f32547d303@redhat.com> Date: Tue, 18 Jun 2019 19:43:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: <20190530134850.3236-4-jan.vrany@fit.cvut.cz> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2019-06/txt/msg00354.txt.bz2 On 5/30/19 2:48 PM, 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 > > * 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 > + 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 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 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 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 Include as well. > + > enum print_values { > PRINT_NO_VALUES, > PRINT_ALL_VALUES, > @@ -191,9 +193,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); 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 . */ > + > +/* 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; 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 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 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 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 . */ > + > +#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 > #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 (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