From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 32229 invoked by alias); 18 Jun 2019 20:03:04 -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 32102 invoked by uid 89); 18 Jun 2019 20:03:04 -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,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.1 spammy=1796 X-HELO: mail-wr1-f65.google.com Received: from mail-wr1-f65.google.com (HELO mail-wr1-f65.google.com) (209.85.221.65) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 18 Jun 2019 20:03:03 +0000 Received: by mail-wr1-f65.google.com with SMTP id n9so876429wru.0 for ; Tue, 18 Jun 2019 13:03:02 -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 u18sm2173231wmd.19.2019.06.18.13.03.00 (version=TLS1_3 cipher=AEAD-AES128-GCM-SHA256 bits=128/128); Tue, 18 Jun 2019 13:03:00 -0700 (PDT) Subject: Re: [PATCH v3 4/5] mi/python: Allow redefinition of python MI commands To: Jan Vrany , gdb-patches@sourceware.org References: <20190128124101.26243-1-jan.vrany@fit.cvut.cz> <20190530134850.3236-5-jan.vrany@fit.cvut.cz> From: Pedro Alves Message-ID: Date: Tue, 18 Jun 2019 20:03: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-5-jan.vrany@fit.cvut.cz> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2019-06/txt/msg00356.txt.bz2 On 5/30/19 2:48 PM, Jan Vrany wrote: > Redefining python MI commands is especially useful when developing > them. > > gdb/Changelog: > > * mi/mi-cmds.h (mi_command::can_be_redefined): New method. > * mi/mi-cmds.c: (mi_command::can_be_redefined): New method. > (insert_mi_cmd_entry): Allow redefinition of python-defined MI commands. > * python/py-micmd.h (mi_command_py::can_be_redefined): New method. > * python/py-micmd.c: (mi_command_py::can_be_redefined): New method. > --- > gdb/ChangeLog | 8 ++++++++ > gdb/mi/mi-cmds.c | 10 +++++++++- > gdb/mi/mi-cmds.h | 3 +++ > gdb/python/py-micmd.c | 24 +++++++++++++++++------- > gdb/python/py-micmd.h | 2 +- > 5 files changed, 38 insertions(+), 9 deletions(-) > > diff --git a/gdb/ChangeLog b/gdb/ChangeLog > index 241e5da68f..1d264c44f7 100644 > --- a/gdb/ChangeLog > +++ b/gdb/ChangeLog > @@ -1,3 +1,11 @@ > +2019-05-02 Jan Vrany > + > + * mi/mi-cmds.h (mi_command::can_be_redefined): New method. > + * mi/mi-cmds.c: (mi_command::can_be_redefined): New method. No ":" after the file name and before "(". > + (insert_mi_cmd_entry): Allow redefinition of python-defined MI commands. > + * python/py-micmd.h (mi_command_py::can_be_redefined): New method. > + * python/py-micmd.c: (mi_command_py::can_be_redefined): New method. Ditto. Please double check the other ChangeLog entries in the other patches. > + > 2019-05-02 Didier Nadeau > Jan Vrany > > diff --git a/gdb/mi/mi-cmds.c b/gdb/mi/mi-cmds.c > index 0aead954f9..e280e51daf 100644 > --- a/gdb/mi/mi-cmds.c > +++ b/gdb/mi/mi-cmds.c > @@ -41,7 +41,8 @@ insert_mi_cmd_entry (mi_cmd_up command) > const std::string &name = command->name (); > > if (mi_cmd_table.find (name) != mi_cmd_table.end ()) > - return false; > + if (! mi_cmd_table[name]->can_be_redefined ()) > + return false; No space after !. I'd combine the two ifs: if (mi_cmd_table.find (name) != mi_cmd_table.end () && !mi_cmd_table[name]->can_be_redefined ()) return false; better even, avoid the second lookup: auto foo = mi_cmd_table.find (name); if (foo != mi_cmd_table.end () && !foo->can_be_redefined ()) return false; > > mi_cmd_table[name] = std::move (command); > > @@ -80,6 +81,13 @@ mi_command::mi_command (const char *name, int *suppress_notification) > m_suppress_notification (suppress_notification) > {} > > +bool > +mi_command::can_be_redefined() Space before parens. > +{ > + return false; > +} > + > + > > void > mi_command::invoke (struct mi_parse *parse) > diff --git a/gdb/mi/mi-cmds.h b/gdb/mi/mi-cmds.h > index 5ca7232fca..331b1349df 100644 > --- a/gdb/mi/mi-cmds.h > +++ b/gdb/mi/mi-cmds.h > @@ -143,6 +143,9 @@ public: > /* Execute the MI command. */ > void invoke (struct mi_parse *parse); > > + /* Return TRUE if command can be redefined, FALSE otherwise. */ "if THE command", or "if THIS command". Double space after period. > + virtual bool can_be_redefined(); Space before parens. > + > protected: > gdb::optional> do_suppress_notification (); > virtual void do_invoke(struct mi_parse *parse) = 0; > diff --git a/gdb/python/py-micmd.c b/gdb/python/py-micmd.c > index e99632c97a..4f2b13f3c7 100644 > --- a/gdb/python/py-micmd.c > +++ b/gdb/python/py-micmd.c > @@ -171,6 +171,12 @@ mi_command_py::mi_command_py (const char *name, gdbpy_ref<> object) > { > } > > +bool > +mi_command_py::can_be_redefined() Space before parens. > +{ > + return true; > +} > + > void > mi_command_py::do_invoke (struct mi_parse *parse) > { > @@ -179,6 +185,7 @@ mi_command_py::do_invoke (struct mi_parse *parse) > if (parse->argv == NULL) > error (_("Problem parsing arguments: %s %s"), parse->command, parse->args); > > + std::string name (this->name ()); As principle, prefer copy-initialization when possible: std::string name = this->name (); This style avoids unwanted explicit conversions, and makes explicit conversions stand out, since they'll be written with ()s. I think this needs to be a copy because of what the "THIS must not be used" comment below says. A comment to the effect would help. Something like: /* Must save a copy of the name because the Python code may replace the very same command that is currently executing. See further below. */ std::string name = this->name (); > PyObject *obj = this->pyobj.get (); > > gdbpy_enter enter_py (get_current_arch (), current_language); > @@ -187,15 +194,14 @@ mi_command_py::do_invoke (struct mi_parse *parse) > > if (!PyObject_HasAttr (obj, invoke_cst)) > error (_("-%s: Python command object missing 'invoke' method."), > - name ().c_str ()); > - > + 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 ()); > + name.c_str ()); > } > > for (int i = 0; i < parse->argc; ++i) > @@ -205,11 +211,16 @@ mi_command_py::do_invoke (struct mi_parse *parse) > if (PyList_SetItem (argobj.get (), i, str.release ()) != 0) > { > error (_("-%s: failed to create the Python arguments list."), > - name ().c_str ()); > + name.c_str ()); > } > } > > gdb_assert (PyErr_Occurred () == NULL); > + > + /* From this point on, THIS must not be used since Python code may replace > + the the very same command that is currently executing. This in turn leads > + to desctruction of THIS making it invalid. See insert_mi_cmd_entry. */ Double "the" in "the the". Typo "desctruction". Double space after period at the end. > + > gdbpy_ref<> result ( > PyObject_CallMethodObjArgs (obj, invoke_cst, argobj.get (), NULL)); > if (PyErr_Occurred () != NULL) > @@ -218,9 +229,9 @@ mi_command_py::do_invoke (struct mi_parse *parse) > 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 ()); > + error (_("-%s: failed to execute command"), name.c_str ()); > else > - error (_("-%s: %s"), name ().c_str (), ex_msg.get ()); > + error (_("-%s: %s"), name.c_str (), ex_msg.get ()); > } > else > { > @@ -233,7 +244,6 @@ void mi_command_py::finalize () > { > this->pyobj.reset (nullptr); > } > - > /* Initialize the MI command object. */ Spurious deletion of that line. > > int > diff --git a/gdb/python/py-micmd.h b/gdb/python/py-micmd.h > index deadc0116b..0d61069ca5 100644 > --- a/gdb/python/py-micmd.h > +++ b/gdb/python/py-micmd.h > @@ -44,7 +44,7 @@ class mi_command_py : public mi_command > > mi_command_py (const char *name, gdbpy_ref<> object); > > - > + bool can_be_redefined() override; > /* This is called just before shutting down a Python interpreter > to release python object implementing the command. */ Space before parens. Add empty line between the new declaration and the unrelated comment below. Double space after period. Thanks, Pedro Alves