Re: [PATCH v2 5/5] gdb: Add support for renaming commands

[Simon Marchi via Gdb-patches <gdb-patches@sourceware.org>]

On 2021-01-25 6:26 a.m., Marco Barisione via Gdb-patches wrote:
> This patch adds:
> * A "rename" command.
> * A "-rename-existing-to" option for the "define" command.
> * A "rename_existing_to" optional argument to gdb.Command.__init__
>   which matches the behaviour of "define -rename-existing-to".
> 
> The goal of this is to allow users to build on top of existing commands
> without losing their original implementation.
> Something similar could be achieved through hooks but they are limited:
> * Hooks cannot prevent the command from being executed without printing
>   any error.
> * Hooks don't get passed the arguments passed to the command.
> * Post hooks can't know if the command failed.
> * Hooks cannot be defined in Python.

The command code really gives me headaches.  I read the code but I won't
pretend like I thoroughly check it.

Here are some minor things I noted while reading it.

> @@ -160,53 +166,191 @@ set_cmd_completer_handle_brkchars (struct cmd_list_element *cmd,
>    cmd->completer_handle_brkchars = func;
>  }
>  
> -/* Like ADD_CMD, but the command function fields are not modified.  */
> +/* Update the prefix-related fields associated with command C, which must
> +   be part of the comand list *LIST, and, if not null, adds PREFIXLIST

comand -> command

> @@ -283,14 +435,19 @@ add_alias_cmd (const char *name, cmd_list_element *old,
>  {
>    if (old == 0)
>      {
> +      struct cmd_list_element **prefixlist;
> +      struct cmd_list_element *aliases;
>        struct cmd_list_element *prehook, *prehookee, *posthook, *posthookee;
> -      struct cmd_list_element *aliases = delete_cmd (name, list,
> -						     &prehook, &prehookee,
> -						     &posthook, &posthookee);
> -
> +      struct cmd_list_element *old_cmd
> +	= disconnect_cmd (name, list,
> +			  &prefixlist,
> +			  &aliases,
> +			  &prehook, &prehookee,
> +			  &posthook, &posthookee);
>        /* If this happens, it means a programmer error somewhere.  */
> -      gdb_assert (!aliases && !prehook && !prehookee
> -		  && !posthook && ! posthookee);
> +      gdb_assert (!old_cmd);

Compare pointers explicitly with nullptr:

  gdb_assert (old_cmd != nullptr);

> +      /* As OLD_CMD is null, NAME didn't exist, so the prefix list,
> +       * aliases and hooks are all null as well.  */

Remove the asterisk on the second line:

      /* As OLD_CMD is null, NAME didn't exist, so the prefix list,
         aliases and hooks are all null as well.  */

> diff --git a/gdb/command.h b/gdb/command.h
> index ca791cff809..b20737a6aee 100644
> --- a/gdb/command.h
> +++ b/gdb/command.h
> @@ -175,24 +175,37 @@ extern bool valid_cmd_char_p (int c);
>     It should start with ? for a command that is an abbreviation
>     or with * for a command that most users don't need to know about.
>  
> +   If RENAME_EXISTING_TO is not null, then the existing command called
> +   NAME in *LIST will be renamed to RENAME_EXISTING_TO in
> +   *RENAME_EXISTING_TO_LIST.  If a command called RENAME_EXISTING_TO
> +   already exists in *RENAME_EXISTING_TO_LIST, it gets deleted first.
> +   Ownership of RENAME_EXISTING_TO is not taken by this function (i.e.
> +   the caller is responsible to free it if needed).
> +
>     If NAME already existed in *LIST, all its hooks and aliases are moved
>     to the new command.
>  
>     Return a pointer to the added command (not necessarily the head of
>     *LIST).  */
>  
> -extern struct cmd_list_element *add_cmd (const char *name,
> -					 enum command_class theclass,
> -					 cmd_const_cfunc_ftype *fun,
> -					 const char *doc,
> -					 struct cmd_list_element **list);
> +extern struct cmd_list_element *add_cmd
> +		(const char *name,
> +		 enum command_class theclass,
> +		 cmd_const_cfunc_ftype *fun,
> +		 const char *doc,
> +		 struct cmd_list_element **list,
> +		 const char *rename_existing_to=nullptr,
> +		 struct cmd_list_element **rename_existing_to_list=nullptr);

Spaces around `=`, apply everywhere where it's relevant.

When wrapping like this, where you need to start the parameter list on
another line, use a single indent (two spaces).  Like this:

extern struct cmd_list_element *add_cmd
  (const char *name,
   enum command_class theclass,
   cmd_const_cfunc_ftype *fun,
   const char *doc,
   struct cmd_list_element **list,
   const char *rename_existing_to=nullptr,
   struct cmd_list_element **rename_existing_to_list=nullptr);

Optionally, you can also put multiple parameters on the same line to use
less lines.  Since we use C++, I like to get rid of `struct` or `enum`
keywords when possible, that's less verbose.

extern cmd_list_element *add_cmd
  (const char *name, command_class theclass, cmd_const_cfunc_ftype *fun,
   const char *doc, cmd_list_element **list,
   const char *rename_existing_to = nullptr,
   cmd_list_element **rename_existing_to_list = nullptr);


> diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
> index 35568594f58..22155f595c6 100644
> --- a/gdb/doc/python.texi
> +++ b/gdb/doc/python.texi
> @@ -3657,7 +3657,7 @@ You can implement new @value{GDBN} CLI commands in Python.  A CLI
>  command is implemented using an instance of the @code{gdb.Command}
>  class, most commonly using a subclass.
>  
> -@defun Command.__init__ (name, @var{command_class} @r{[}, @var{completer_class} @r{[}, @var{prefix}@r{]]})
> +@defun Command.__init__ (name, @var{command_class} @r{[}, @var{completer_class} @r{[}, @var{prefix} @r{[}, @var{rename_existing_to}@r{]]]})

I think the way this renders to is misleading:

    Command.__init__ (name, command_class [, completer_class [, prefix [, rename_existing_to]]])

The brackets are nested, so it makes it sound like rename_existing_to
can only be used when prefix is given.  And they both can only be used
when completer_class is given.

And since command_class is now sometimes optional, it should be marked
as such.

    Command.__init__ (name[, command_class] [, completer_class] [, prefix] [, rename_existing_to])

> @@ -3931,6 +3945,24 @@ registration of the command with @value{GDBN}.  Depending on how the
>  Python code is read into @value{GDBN}, you may need to import the
>  @code{gdb} module explicitly.
>  
> +The following code snippet shows how a CLI command building on top of an
> +existing command can be implemented:
> +
> +@smallexample
> +class MyQuit (gdb.Command):
> +  """Print a message before executing the "quit" command."""
> +
> +  def __init__ (self):
> +    super (MyQuit, self).__init__ ("quit", gdb.COMMAND_RUNNING,
> +                                  rename_existing_to="original-quit")

Do you use COMMAND_RUNNING on purpose here, to show that it's possible
to change the class of a command?

> @@ -450,40 +457,130 @@ cmdpy_init (PyObject *self, PyObject *args, PyObject *kw)
>        return -1;
>      }
>  
> -  if (!gdb_PyArg_ParseTupleAndKeywords (args, kw, "si|iO",
> -					keywords, &name, &cmdtype,
> -					&completetype, &is_prefix_obj))
> +  /* The second argument (command_class) is marked as optional because
> +     it should not be specified if RENAME_EXISTING_TO is specified.
> +     Otherwise it's required.  */
> +  if (!gdb_PyArg_ParseTupleAndKeywords (args, kw, "s|O!O!Os",
> +					keywords,
> +					&name,
> +					&PyInt_Type, &cmdtype_obj,
> +					&PyInt_Type, &completetype_obj,
> +					&is_prefix_obj,
> +					&rename_existing_to))
>      return -1;
>  
> -  if (cmdtype != no_class && cmdtype != class_run
> -      && cmdtype != class_vars && cmdtype != class_stack
> -      && cmdtype != class_files && cmdtype != class_support
> -      && cmdtype != class_info && cmdtype != class_breakpoint
> -      && cmdtype != class_trace && cmdtype != class_obscure
> -      && cmdtype != class_maintenance && cmdtype != class_user
> -      && cmdtype != class_tui)
> -    {
> -      PyErr_Format (PyExc_RuntimeError, _("Invalid command class argument."));
> -      return -1;
> -    }
> -
> -  if (completetype < -1 || completetype >= (int) N_COMPLETERS)
> -    {
> -      PyErr_Format (PyExc_RuntimeError,
> -		    _("Invalid completion type argument."));
> -      return -1;
> -    }
> -
>    cmd_name = gdbpy_parse_command_name (name, &cmd_list, &cmdlist);
>    if (! cmd_name)
>      return -1;
>  
> +  /* Deal with rename_existing_to before other arguments as it affects how
> +     some of them are treated.  */
> +  if (rename_existing_to != NULL)
> +    {
> +      renamed_cmd = lookup_cmd_exact (cmd_name, *cmd_list);
> +      if (renamed_cmd == nullptr)
> +	{
> +	  PyErr_Format (PyExc_RuntimeError,
> +			_("Command \"%s\" does not exist, so it cannot "
> +			  "be renamed to \"%s\"."),
> +			name, rename_existing_to);
> +	  xfree (cmd_name);
> +	  return -1;
> +	}
> +
> +      rename_existing_to_cmd_name =
> +	gdbpy_parse_command_name (rename_existing_to,
> +				  &rename_existing_to_cmd_list,
> +				  &cmdlist);
> +      if (! rename_existing_to_cmd_name)

  if (rename_existing_to_cmd_name == nullptr)

If the surrounding code is incorrect, you can fix it up too, if you want
to keep it consistent.

> +	{
> +	  xfree (cmd_name);
> +	  return -1;

Instead of xfree'ing cmd_name by hand, I'll make a patch that makes
gdbpy_parse_command_name return a gdb::unique_xmalloc_ptr.  You'll have
to update this code, but it will just make it simpler.

Here it is, I would appreciate if you could give it a look:

  https://sourceware.org/pipermail/gdb-patches/2021-March/177162.html

Thanks,

Simon