From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id EPUoEkw3WmCgQwAAWB0awg (envelope-from ) for ; Tue, 23 Mar 2021 14:45:32 -0400 Received: by simark.ca (Postfix, from userid 112) id 3694E1EF78; Tue, 23 Mar 2021 14:45:32 -0400 (EDT) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-1.1 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,MAILING_LIST_MULTI,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.2 Received: from sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id E83CC1E939 for ; Tue, 23 Mar 2021 14:45:30 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id A2BEC3857829; Tue, 23 Mar 2021 18:45:30 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org A2BEC3857829 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1616525130; bh=qN4pIPynxZ+M/fYh+i+qMW3bGFBEjeH1vcpFBZs8ZmY=; h=Subject:To:References:Date:In-Reply-To:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=GbAELyZ6kAjWGBsqq8HxRQous5n9d7KBZh6Nbnn0MdbsU+Y2Rn+KPAdzEpSeo8nhX wZsHE9FPa6BvA6XVKzyn1kdgs1UgnppDnYvbMbeBfIqE2ppyHlexINBvMLMoy8DmLp nvhBjAMjjchmEmdbg1XHwItz9hZ0FHSctjCv+UNw= Received: from smtp.polymtl.ca (smtp.polymtl.ca [132.207.4.11]) by sourceware.org (Postfix) with ESMTPS id B50663857829 for ; Tue, 23 Mar 2021 18:45:27 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org B50663857829 Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id 12NIjMbY015083 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 23 Mar 2021 14:45:26 -0400 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 12NIjMbY015083 Received: from [10.0.0.11] (192-222-157-6.qc.cable.ebox.net [192.222.157.6]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id DAB0F1E939; Tue, 23 Mar 2021 14:45:21 -0400 (EDT) Subject: Re: [PATCH v2 5/5] gdb: Add support for renaming commands To: Marco Barisione , gdb-patches@sourceware.org References: <20210108100706.96190-1-mbarisione@undo.io> <20210125112649.56362-1-mbarisione@undo.io> <20210125112649.56362-6-mbarisione@undo.io> Message-ID: Date: Tue, 23 Mar 2021 14:45:21 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.7.1 MIME-Version: 1.0 In-Reply-To: <20210125112649.56362-6-mbarisione@undo.io> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Tue, 23 Mar 2021 18:45:22 +0000 X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Simon Marchi via Gdb-patches Reply-To: Simon Marchi Errors-To: gdb-patches-bounces@sourceware.org Sender: "Gdb-patches" 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