From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id E15XFHWyRmBgcgAAWB0awg (envelope-from ) for ; Mon, 08 Mar 2021 18:25:41 -0500 Received: by simark.ca (Postfix, from userid 112) id 4BEE31EF78; Mon, 8 Mar 2021 18:25:41 -0500 (EST) 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 96C4A1E789 for ; Mon, 8 Mar 2021 18:25:39 -0500 (EST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 25CD9385802A; Mon, 8 Mar 2021 23:25:39 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 25CD9385802A DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1615245939; bh=Nil7mrLcWOa/GbahZJGtxhIylpWW0OW2/4LCh/44EJI=; 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=B7zPLQYMW72lHmM7QhNTRmx15BaB91q6j1VPjR5ACwt9e3CSx3mIEiyxMT+nT3nve bVYLUGNKK/HnnxO6rJD0UZrpAMMaIITx+D/P9GTLt8B+mvpYdbJBguMTxHMrJuI+SB 97oH9lQBRJEI0Wgx/ztWUGuY/IK1F17LpBC2zjXU= Received: from smtp.polymtl.ca (smtp.polymtl.ca [132.207.4.11]) by sourceware.org (Postfix) with ESMTPS id 9AFA7385802A for ; Mon, 8 Mar 2021 23:25:36 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 9AFA7385802A 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 128NPV0T016376 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 8 Mar 2021 18:25:35 -0500 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 128NPV0T016376 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 07C331E789; Mon, 8 Mar 2021 18:25:31 -0500 (EST) Subject: Re: [PATCH v2 4/5] gdb: generate the prefix name for prefix commands on demand To: Marco Barisione , gdb-patches@sourceware.org References: <20210108100706.96190-1-mbarisione@undo.io> <20210125112649.56362-1-mbarisione@undo.io> <20210125112649.56362-5-mbarisione@undo.io> Message-ID: <1b0b042b-f8f4-c350-dcbf-1f617670f24f@polymtl.ca> Date: Mon, 8 Mar 2021 18:25:30 -0500 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-5-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 Mon, 8 Mar 2021 23:25:31 +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: > 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