Re: [PATCH v3 4/5] mi/python: Allow redefinition of python MI commands

[Pedro Alves <palves@redhat.com>]

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  <jan.vrany@fit.cvut.cz>
> +
> +	* 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  <didier.nadeau@gmail.com>
>              Jan Vrany  <jan.vrany@fit.cvut.cz>
>  
> 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<scoped_restore_tmpl<int>> 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<char> 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