Re: [PATCH v2 4/5] gdb: generate the prefix name for prefix commands on demand

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

On 2021-01-25 6:26 a.m., Marco Barisione via Gdb-patches wrote:
> Previously, the prefixname field of struct cmd_list_element was manually
> set for prefix commands.  This seems verbose and error prone as it
> required every single call to functions adding prefix commands to
> specify the prefix name while the same information can be easily
> generated.
> 
> Historically, this was not possible as the prefix field was null for
> many commands, but this was fixed in commit
> 3f4d92ebdf7f848b5ccc9e8d8e8514c64fde1183 by Philippe Waroquiers, so
> we can rely on the prefix field being set when generating the prefix
> name.

Sounds like a good idea to me.  The patch is ok to push with these nits
fixed.

> 
> This commit also fixes a free after in this scenario:

"a free" -> "a use after free"?

> * A command gets created via Python (using the gdb.Command class).
>   The prefix name member is dynamically allocated.
> * An alias to the new command is created. The alias's prefixname is set
>   to point to the prefixname for the original command with a direct
>   assignment.
> * A new command with the same name as the Python command is created.
> * The object for the original Python command gets freed and its
>   prefixname gets freed as well.
> * The alias is update to point to the new command, but its prefixname
>   is not updated so it keeps pointing to the freed one.
> 
> gdb/ChangeLog:
> 
> 	* command.h (add_prefix_cmd): Remove the prefixname argument as
> 	it can now be generated automatically.
> 	(add_basic_prefix_cmd): Ditto.
> 	(add_show_prefix_cmd): Ditto.
> 	(add_prefix_cmd_suppress_notification): Ditto.
> 	(add_abbrev_prefix_cmd): Ditto.
> 	* cli/cli-decode.h (struct cmd_list_element): Replace the
> 	prefixname member variable with a method which generates the
> 	prefix name at runtime.
> 	* python/py-cmd.c (cmdpy_destroyer): Remove code to free the
> 	prefixname member as it's now a method.
> 	(cmdpy_function): Determine if the command is a prefix by
> 	looking at prefixlist, not prefixname.
> 	(cmdpy_init): Remove code to generate the prefix name.
> 	* cli/cli-decode.c (add_alias_cmd): Do not set prefixname on
> 	aliases.
> 	(do_prefix_cmd): Do not pass the prefixname argument to
> 	help_list.
> 	(add_prefix_cmd): Remove the prefixname argument.
> 	(add_basic_prefix_cmd): Ditto.
> 	(add_show_prefix_cmd): Ditto.
> 	(add_abbrev_prefix_cmd): Ditto.
> 	(fput_command_name_styled): Use the prefixname method.
> 	(apropos_cmd): Ditto.
> 	(help_cmd): Ditto.
> 	(lookup_cmd): Ditto.
> 	(deprecated_cmd_warning): Ditto.
> 	* cli/cli-script.c (do_define_command): Do not set the prefix
> 	name.
> 	(define_prefix_command): Ditto.
> 	(show_user_1): Uupdate to use the prefixname method.
> 	* cli/cli-setshow.c (cmd_show_list): Ditto.
> 	* top.c (execute_command): Ditto.
> 	* unittests/command-def-selftests.c (check_doc): Ditto.
> 	(traverse_command_structure): Ditto.
> 	* ada-lang.c (_initialize_ada_language): Do not pass the prefix
> 	name as it's not needed any more.
> 	* arm-tdep.c (_initialize_arm_tdep): Ditto.
> 	* auto-load.c: Ditto.
> 	* breakpoint.c: Ditto.
> 	* bsd-kvm.c: Ditto.
> 	* btrace.c: Ditto.
> 	* cli/cli-cmds.c: Ditto.
> 	* cli/cli-dump.c (_initialize_cli_dump): Ditto.
> 	* cli/cli-logging.c (_initialize_cli_logging): Ditto.
> 	* cli/cli-style.c (cli_style_option::add_setshow_commands): Ditto.
> 	* cli/cli-style.h (class cli_style_option): Ditto.
> 	* compile/compile.c: Ditto.
> 	* cp-support.c (_initialize_cp_support): Ditto.
> 	* dcache.c: Ditto.
> 	* dwarf2/index-cache.c (_initialize_index_cache): Ditto.
> 	* dwarf2/read.c: Ditto.
> 	* f-lang.c (_initialize_f_language): Ditto.
> 	* frame.c: Ditto.
> 	* gnu-nat.c (add_thread_commands): Ditto.
> 	* go32-nat.c (DJGPP): Ditto.
> 	* guile/guile.c: Ditto.
> 	* guile/scm-cmd.c (gdbscm_register_command_x): Ditto.
> 	* i386-tdep.c: Ditto.
> 	* infcmd.c: Ditto.
> 	* language.c (_initialize_language): Ditto.
> 	* macrocmd.c (_initialize_macrocmd): Ditto.
> 	* maint-test-options.c: Ditto.
> 	* maint-test-settings.c: Ditto.
> 	* maint.c: Ditto.
> 	* memattr.c: Ditto.
> 	* mips-tdep.c (_initialize_mips_tdep): Ditto.
> 	* printcmd.c: Ditto.
> 	* probe.c: Ditto.
> 	* python/python.c: Ditto.
> 	* ravenscar-thread.c (_initialize_ravenscar): Ditto.
> 	* record-btrace.c (_initialize_record_btrace): Ditto.
> 	* record-full.c (_initialize_record_full): Ditto.
> 	* record.c: Ditto.
> 	* remote.c: Ditto.
> 	* riscv-tdep.c (_initialize_riscv_tdep): Ditto.
> 	* rs6000-tdep.c (_initialize_rs6000_tdep): Ditto.
> 	* ser-tcp.c: Ditto.
> 	* serial.c: Ditto.
> 	* sh-tdep.c (_initialize_sh_tdep): Ditto.
> 	* skip.c: Ditto.
> 	* sparc64-tdep.c (_initialize_sparc64_adi_tdep): Ditto.
> 	* stack.c: Ditto.
> 	* symfile.c: Ditto.
> 	* symtab.c: Ditto.
> 	* target-descriptions.c: Ditto.
> 	* target.c: Ditto.
> 	* thread.c: Ditto.
> 	* tracepoint.c: Ditto.
> 	* tui/tui-layout.c: Ditto.
> 	* tui/tui-win.c (tui_get_cmd_list): Ditto.
> 	(_initialize_tui_win): Ditto.
> 	* typeprint.c: Ditto.
> 	* utils.c (add_internal_problem_command): Ditto.
> 	* valprint.c: Ditto.
> 	* value.c: Ditto.
> 	* windows-tdep.c (init_w32_command_list): Ditto.

IMO it's not really useful to list all the spots that were just
trivially adjusted.  It's ok to write things like:

  * command.h (add_prefix_cmd): Remove prefixname parameter.  Update all
  callers.

... and then skip listing all callers.  That will also help preserve
your sanity.

> diff --git a/gdb/cli/cli-decode.h b/gdb/cli/cli-decode.h
> index e6907a95a84..ddcb2ea9578 100644
> --- a/gdb/cli/cli-decode.h
> +++ b/gdb/cli/cli-decode.h
> @@ -71,6 +71,26 @@ struct cmd_list_element
>  
>      DISABLE_COPY_AND_ASSIGN (cmd_list_element);
>  
> +    /* For prefix commands, retun a string containing prefix commands to

retun -> return

> +       get here: this one plus any others needed to get to it.  Ends in a
> +       space.  It is used before the word "command" in describing the
> +       commands reached through this prefix.
> +
> +       For non-prefix commands, an empty string is returned.  */
> +    std::string prefixname ()
> +      {
> +	if (prefixlist == nullptr)
> +	  /* Not a prefix command.  */
> +	  return "";
> +
> +	std::string prefixname;
> +	if (prefix != nullptr)
> +	  prefixname = prefix->prefixname ();
> +	prefixname += name;
> +	prefixname += " ";
> +	return prefixname;
> +      }

De-indent the whole block by one (see the methods just above).

Simon