From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id iwHgC20A6F9gDgAAWB0awg (envelope-from ) for ; Sat, 26 Dec 2020 22:33:01 -0500 Received: by simark.ca (Postfix, from userid 112) id 1E1921F0AA; Sat, 26 Dec 2020 22:33:01 -0500 (EST) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=0.2 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,MAILING_LIST_MULTI,RDNS_NONE,URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.2 Received: from sourceware.org (unknown [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 678781E552 for ; Sat, 26 Dec 2020 22:33:00 -0500 (EST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 84AAC3857C60; Sun, 27 Dec 2020 03:32:59 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 84AAC3857C60 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1609039979; bh=i4GSvXPWbUMERTtqHmGav1R1//iALSPSu6Gwr+lv1Ws=; h=Subject:To:References:Date:In-Reply-To:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=SmeIb6+XOOaN34WGUM7bcVmD5+W/bm3qK1Tf8/yk79ubZa7Gs22CJIRo4jRO5T0cw MXCXI5lENhRpZhyiy20ZyKzgllmOOifWz7C/CMk4X3uGrbfDf34htgwVTWSTkA+Lww MXsAYUPWPBU8FcWmS6cuuvajj39bVEWjR1ch7+cA= Received: from smtp.polymtl.ca (smtp.polymtl.ca [132.207.4.11]) by sourceware.org (Postfix) with ESMTPS id 98E3E3857C60 for ; Sun, 27 Dec 2020 03:32:55 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 98E3E3857C60 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 0BR3WnU5024741 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Sat, 26 Dec 2020 22:32:53 -0500 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 0BR3WnU5024741 Received: from [10.0.0.213] (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) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 9A26F1E552; Sat, 26 Dec 2020 22:32:48 -0500 (EST) Subject: Re: [PATCH v3 19/24] New mtag commands To: Luis Machado , gdb-patches@sourceware.org References: <20201109170435.15766-1-luis.machado@linaro.org> <20201109170435.15766-20-luis.machado@linaro.org> Message-ID: <58d052da-d1da-9e92-c9aa-0a12303a8663@polymtl.ca> Date: Sat, 26 Dec 2020 22:32:48 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0 MIME-Version: 1.0 In-Reply-To: <20201109170435.15766-20-luis.machado@linaro.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Sun, 27 Dec 2020 03:32:49 +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 Cc: david.spickett@linaro.org Errors-To: gdb-patches-bounces@sourceware.org Sender: "Gdb-patches" On 2020-11-09 12:04 p.m., Luis Machado via Gdb-patches wrote: > Add new commands under the "mtag" prefix to allow users to inspect, modify and > check memory tags in different ways. > > The available subcommands are the following: > > - mtag showltag : Shows the logical tag for a particular address. > > - mtag withltag : Prints the address tagged with the logical > tag . > > - mtag showatag : Shows the allocation tag for a particular address. > > - mtag setatag : Sets one or more allocation tags to > the specified tags. > > - mtag check : Check if the logical tag in
matches its > allocation tag. The only thing that bugs me here is the command names. I'm sure we could bikeshed for ever about this, but here are my comments: - I don't really like the "everything glued together" style, that makes it harder to read the name. For instance, I don't like the existing "set remoteaddresssize" command, that's just ugly. We usually separate words with dashes. - I don't really like the use of abbreviations (especially one-letter abbreviations) in commands, as it's not good for discoverability. I would prefer if we used full names, for clarity, and if you really want a short version, add an alias. My opinion is that with tab-completion, long (but clear) command names are not really an issue. So: - mtag show-logical-tag - mtag with-logical-tag - mtag show-allocation-tag - mtag set-allocation-tag And the "mtag" prefix... maybe "memory-tag", or "mem-tag" at least? It's not obvious that the "m" stands for "memory", > > These commands make use of the memory tagging gdbarch methods, and are still > available, but disabled, when memory tagging is not supported by the > architecture. > > I've pondered about a way to make these commands invisible when memory tagging > is not available, but given the check is at runtime (and support may come and go > based on a process' configuration), that is a bit too late in the process to > either not include the commands or get rid of them. I think it's fine if the commands are always there, because it just says "this architecture does not support memory tagging" if you try to use them with an architecture that does not support memory tagging. FYI, I get this when trying to build this patch: CXX printcmd.o cc1plus: warning: command-line option ‘-Wmissing-prototypes’ is valid for C/ObjC but not for C++ /home/simark/src/binutils-gdb/gdb/printcmd.c: In function ‘void parse_withltag_input(const char*, value**, gdb::byte_vector&, value_print_options*)’: /home/simark/src/binutils-gdb/gdb/printcmd.c:2918:10: error: ‘hex2bin’ was not declared in this scope 2918 | tags = hex2bin (s_tag.c_str ()); | ^~~~~~~ /home/simark/src/binutils-gdb/gdb/printcmd.c: In function ‘void parse_setatag_input(const char*, value**, size_t*, gdb::byte_vector&)’: /home/simark/src/binutils-gdb/gdb/printcmd.c:2997:10: error: ‘hex2bin’ was not declared in this scope 2997 | tags = hex2bin (s_tags.c_str ()); | ^~~~~~~ > > Ideas are welcome. > > gdb/ChangeLog: > > YYYY-MM-DD Luis Machado > > * printcmd.c: Include gdbsupport/rsp-low.h. > (mtaglist): New static global. > (process_print_command_args): Factored out of > print_command_1. > (print_command_1): Use process_print_command_args. > (show_addr_not_tagged, show_memtag_unsupported, mtag_command) > (mtag_showtag_command, mtag_showltag_command, mtag_showatag_command) > (parse_withltag_input, mtag_withltag_command, parse_setatag_input) > (mtag_setatag_command, mtag_check_command): New functions. > (_initialize_printcmd): Add "mtag" prefix and subcommands. > > gdbsupport/ChangeLog: > > YYYY-MM-DD Luis Machado > > * rsp-low.cc (fromhex): Change error message text to not be > RSP-specific. Then, this function should probably be moved out of rsp-low.cc, perhaps to common-utils.cc. > --- > gdb/printcmd.c | 359 ++++++++++++++++++++++++++++++++++++++++-- > gdbsupport/rsp-low.cc | 2 +- > 2 files changed, 348 insertions(+), 13 deletions(-) > > diff --git a/gdb/printcmd.c b/gdb/printcmd.c > index 28451612ab..6949678235 100644 > --- a/gdb/printcmd.c > +++ b/gdb/printcmd.c > @@ -53,6 +53,11 @@ > #include "source.h" > #include "gdbsupport/byte-vector.h" > #include "gdbsupport/gdb_optional.h" > +#include "gdbsupport/rsp-low.h" > + > +/* Chain containing all defined mtag subcommands. */ > + > +struct cmd_list_element *mtaglist; static? > > /* Last specified output format. */ > > @@ -1219,31 +1224,38 @@ print_value (value *val, const value_print_options &opts) > annotate_value_history_end (); > } > > -/* Implementation of the "print" and "call" commands. */ > +/* Helper for parsing arguments for print_command_1. */ > > -static void > -print_command_1 (const char *args, int voidprint) > +static struct value * > +process_print_command_args (const char *args, value_print_options *print_opts) > { > - struct value *val; > - value_print_options print_opts; > - > - get_user_print_options (&print_opts); > + get_user_print_options (print_opts); > /* Override global settings with explicit options, if any. */ > - auto group = make_value_print_options_def_group (&print_opts); > + auto group = make_value_print_options_def_group (print_opts); > gdb::option::process_options > (&args, gdb::option::PROCESS_OPTIONS_REQUIRE_DELIMITER, group); > > - print_command_parse_format (&args, "print", &print_opts); > + print_command_parse_format (&args, "print", print_opts); > > const char *exp = args; > > if (exp != nullptr && *exp) > { > expression_up expr = parse_expression (exp); > - val = evaluate_expression (expr.get ()); > + return evaluate_expression (expr.get ()); > } > - else > - val = access_value_history (0); > + > + return access_value_history (0); > +} > + > +/* Implementation of the "print" and "call" commands. */ > + > +static void > +print_command_1 (const char *args, int voidprint) > +{ > + value_print_options print_opts; > + > + struct value *val = process_print_command_args (args, &print_opts); > > if (voidprint || (val && value_type (val) && > value_type (val)->code () != TYPE_CODE_VOID)) > @@ -2711,6 +2723,273 @@ eval_command (const char *arg, int from_tty) > execute_command (expanded.c_str (), from_tty); > } > > +/* Convenience function for error checking in mtag commands. */ > + > +static void > +show_addr_not_tagged (CORE_ADDR address) > +{ > + error (_("Address %s not in a region mapped with a memory tagging flag."), > + paddress (target_gdbarch (), address)); > +} > + > +/* Convenience function for error checking in mtag commands. */ > + > +static void > +show_memtag_unsupported (void) > +{ > + error (_("Memory tagging not supported or disabled by the current" > + " architecture.")); > +} > + > +/* Implement the "mtag" prefix command. */ > + > +static void > +mtag_command (const char *arg, int from_tty) > +{ > + help_list (mtaglist, "mtag ", all_commands, gdb_stdout); > +} > + > +/* Helper for showltag and showatag. */ > + > +static void > +mtag_showtag_command (const char *args, enum memtag_type tag_type) > +{ > + if (args == nullptr) > + error_no_arg (_("address or pointer")); > + > + /* Parse args into a value. If the value is a pointer or an address, > + then fetch the logical or allocation tag. */ > + value_print_options print_opts; > + > + struct value *val = process_print_command_args (args, &print_opts); > + > + /* If the address is not in a region memory mapped with a memory tagging > + flag, it is no use trying to access/manipulate its allocation tag. > + > + It is OK to manipulate the logical tag though. */ > + if (tag_type == tag_allocation > + && !gdbarch_tagged_address_p (target_gdbarch (), val)) > + show_addr_not_tagged (value_as_address (val)); > + > + std::string tag = gdbarch_memtag_to_string (target_gdbarch (), > + val, tag_type); > + if (tag.empty ()) > + printf_filtered (_("%s tag unavailable.\n"), > + tag_type == tag_logical? "Logical" : "Allocation"); > + > + struct value *v_tag = process_print_command_args (tag.c_str (), > + &print_opts); > + print_opts.output_format = 'x'; > + print_value (v_tag, print_opts); When I read the patch description, I almost mentioned that it might make sense to name the commands "print-xyz" instead of "show-xyz". "print" is used to print values, while "show" is used to show parameter values. The new commands are more print-like IMO. And the fact that they share print's syntax, I think it would make even more sense. To be clear, I'm proposing that we name the sub-commands "print-logical-tag" and "print-allocation-tag". > +} > + > +/* Implement the "mtag showltag" command. */ > + > +static void > +mtag_showltag_command (const char *args, int from_tty) > +{ > + if (!memtag || !target_supports_memory_tagging ()) > + show_memtag_unsupported (); > + > + mtag_showtag_command (args, tag_logical); > +} > + > +/* Implement the "mtag showatag" command. */ > + > +static void > +mtag_showatag_command (const char *args, int from_tty) > +{ > + if (!memtag || !target_supports_memory_tagging ()) > + show_memtag_unsupported (); > + > + mtag_showtag_command (args, tag_allocation); > +} > + > +/* Parse ARGS and extract ADDR and TAG. > + ARGS should have format . */ > + > +static void > +parse_withltag_input (const char *args, struct value **val, > + gdb::byte_vector &tags, value_print_options *print_opts) > +{ > + /* Given can be reasonably complex, we parse things backwards > + so we can isolate the portion. */ This comment doesn't seem to agree with the implementation, or I'm missing something. I don't see any things being parsed backwards. > + > + /* Fetch the address. */ > + std::string s_address = extract_string_maybe_quoted (&args); > + > + /* Parse the address into a value. */ > + *val = process_print_command_args (s_address.c_str (), print_opts); > + > + /* Fetch the tag bytes. */ > + std::string s_tag = extract_string_maybe_quoted (&args); > + > + /* Validate the input. */ > + if (s_address.empty () || s_tag.empty ()) > + error (_("Missing arguments.")); > + > + if (s_tag.length () % 2) > + error (_("Error parsing tags argument. The tag should be 2 digits.")); I think the error message is a bit confusing. At least, it doesn't match the check. Should it be "the tag(s) should be an even number of digits"? > + > + tags = hex2bin (s_tag.c_str ()); > +} > + > +/* Implement the "mtag withltag" command. */ > + > +static void > +mtag_withltag_command (const char *args, int from_tty) > +{ > + if (!memtag || !target_supports_memory_tagging ()) > + show_memtag_unsupported (); > + > + if (args == nullptr) > + error_no_arg (_("
")); > + > + gdb::byte_vector tags; > + struct value *val; > + value_print_options print_opts; > + > + /* Parse the input. */ > + parse_withltag_input (args, &val, tags, &print_opts); > + > + /* Setting the logical tag is just a local operation that does not touch > + any memory from the target. Given an input value, we modify the value > + to include the appropriate tag. > + > + For this reason we need to cast the argument value to a > + (void *) pointer. This is so we have the right the for the gdbarch This sentence looks broken. > + hook to manipulate the value and insert the tag. > + > + Otherwise, this would fail if, for example, GDB parsed the argument value > + into an int-sized value and the pointer value has a type of greater > + length. */ > + > + /* Cast to (void *). */ > + val = value_cast (builtin_type (target_gdbarch ())->builtin_data_ptr, > + val); > + > + if (gdbarch_set_memtags (target_gdbarch (), val, 0, tags, > + tag_logical) != 0) > + printf_filtered (_("Could not update the logical tag data.\n")); > + else > + { > + /* Always print it in hex format. */ > + print_opts.output_format = 'x'; > + print_value (val, print_opts); > + } > +} > + > +/* Parse ARGS and extract ADDR, LENGTH and TAGS. */ > + > +static void > +parse_setatag_input (const char *args, struct value **val, size_t *length, > + gdb::byte_vector &tags) > +{ > + /* Fetch the address. */ > + std::string s_address = extract_string_maybe_quoted (&args); > + > + /* Parse the address into a value. */ > + value_print_options print_opts; > + *val = process_print_command_args (s_address.c_str (), &print_opts); > + > + /* Fetch the length. */ > + std::string s_length = extract_string_maybe_quoted (&args); > + > + /* Fetch the tag bytes. */ > + std::string s_tags = extract_string_maybe_quoted (&args); > + > + /* Validate the input. */ > + if (s_address.empty () || s_length.empty () || s_tags.empty ()) > + error (_("Missing arguments.")); > + > + errno = 0; > + *length = strtoulst (s_length.c_str (), NULL, 10); > + if (errno != 0) > + error (_("Error parsing length argument.")); > + > + if (s_tags.length () % 2) > + error (_("Error parsing tags argument. Tags should be 2 digits per byte.")); > + > + tags = hex2bin (s_tags.c_str ()); > + > + /* If the address is not in a region memory mapped with a memory tagging > + flag, it is no use trying to access/manipulate its allocation tag. */ > + if (!gdbarch_tagged_address_p (target_gdbarch (), *val)) > + show_addr_not_tagged (value_as_address (*val)); > +} > + > +/* Implement the "mtag setatag" command. > + ARGS should be in the format
. */ > + > +static void > +mtag_setatag_command (const char *args, int from_tty) > +{ > + if (!memtag || !target_supports_memory_tagging ()) > + show_memtag_unsupported (); > + > + if (args == nullptr) > + error_no_arg (_(" ")); > + > + gdb::byte_vector tags; > + size_t length = 0; > + struct value *val; > + > + /* Parse the input. */ > + parse_setatag_input (args, &val, &length, tags); > + > + if (gdbarch_set_memtags (target_gdbarch (), val, length, tags, > + tag_allocation) != 0) > + printf_filtered (_("Could not update the allocation tag(s).\n")); > + else > + printf_filtered (_("Allocation tag(s) updated successfully.\n")); > +} > + > +/* Implement the "mtag check" command. */ > + > +static void > +mtag_check_command (const char *args, int from_tty) > +{ > + if (!memtag || !target_supports_memory_tagging ()) > + show_memtag_unsupported (); > + > + if (args == nullptr) > + error (_("Argument required (address or pointer)")); > + > + /* Parse the expression into a value. If the value is an address or > + pointer, then check its logical tag against the allocation tag. */ > + value_print_options print_opts; > + > + struct value *val = process_print_command_args (args, &print_opts); > + > + /* If the address is not in a region memory mapped with a memory tagging > + flag, it is no use trying to access/manipulate its allocation tag. */ > + if (!gdbarch_tagged_address_p (target_gdbarch (), val)) > + show_addr_not_tagged (value_as_address (val)); > + > + CORE_ADDR addr = value_as_address (val); > + > + /* If memory tagging validation is on, check if the tag is valid. */ This comment doesn't seem in-line with the implementation, there is no check here whether the mem tag validation is on or off. Simon