From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id M9ekKa9l61/EaQAAWB0awg (envelope-from ) for ; Tue, 29 Dec 2020 12:21:51 -0500 Received: by simark.ca (Postfix, from userid 112) id 9C39D1F0AA; Tue, 29 Dec 2020 12:21:51 -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 6CBB01E965 for ; Tue, 29 Dec 2020 12:21:49 -0500 (EST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id C9E8B3851C0D; Tue, 29 Dec 2020 17:21:48 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org C9E8B3851C0D DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1609262508; bh=V646tSVHBxmUk12Qubz0FqDrBm2slKYcDEd9jvDvKdQ=; 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=MNWZ26Aq9sZjbQU2BjazeOr1gttCqGOPVOPnjSME+31WaTp3rew8cG5x92F504ToY E3akfCpI8HMERFomA6EarE1X3bxa3bu0nhByVvSOEG7phWJpJP/AjY8Yph/aNm9EEA qDt4mcBd9L3Cxd1kDJ9con7gPYRXRxq6jJdzukAM= Received: from mail-qk1-x736.google.com (mail-qk1-x736.google.com [IPv6:2607:f8b0:4864:20::736]) by sourceware.org (Postfix) with ESMTPS id 2D3513857012 for ; Tue, 29 Dec 2020 17:21:45 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 2D3513857012 Received: by mail-qk1-x736.google.com with SMTP id p14so11910283qke.6 for ; Tue, 29 Dec 2020 09:21:45 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=V646tSVHBxmUk12Qubz0FqDrBm2slKYcDEd9jvDvKdQ=; b=cOxZCeV7Ng5Rvd+0kYRSfngNSDO1ArtuewsBByynabXoFpQTR3M1aHG5NRTvCj8kAC XB7zfVnude0kM1z5M4xlHyHQF5dPhirOvtZkWwrZViyoNiCl0qyCsNORuJ/SYatKHjlt z6tMbhcg0cJ6Sy1VkGM7wm6fIjscY/evvaw9XtIMXocFhXNjlUyodNI/SB+l0mwJ9krh aDChyI941Ep/+YGZCJhSoCbktc1NWCDuo5xFWtuAiI+lmij4BFSTP1raktoAeRTk8S9i Sm84UyZIs2CGbQWZ/6G8NvGjjycTh0dvtyZett1kDGGjBi/tdQ94sk4w6IJN7qYyWrnM qI1Q== X-Gm-Message-State: AOAM530oRKkm+hrV+gBLvXWQuVd9XSN7+iLfWPB1gjwKCuwm2TJFEhzw IvBMtdUFIl2rTcmTT0e0eFZfxw== X-Google-Smtp-Source: ABdhPJwak0SL0WbcuaAJ/RWyutONXjisPX6cTqwNNKdRLI+hrJYbrmcQY4vIIxtxufk1xGAe9Bo/eQ== X-Received: by 2002:ae9:e8c5:: with SMTP id a188mr48768533qkg.479.1609262503933; Tue, 29 Dec 2020 09:21:43 -0800 (PST) Received: from ?IPv6:2804:7f0:8284:370e:891:588d:2ac2:dc7e? ([2804:7f0:8284:370e:891:588d:2ac2:dc7e]) by smtp.gmail.com with ESMTPSA id s30sm26137248qte.44.2020.12.29.09.21.42 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 29 Dec 2020 09:21:43 -0800 (PST) Subject: Re: [PATCH v3 19/24] New mtag commands To: Simon Marchi , gdb-patches@sourceware.org References: <20201109170435.15766-1-luis.machado@linaro.org> <20201109170435.15766-20-luis.machado@linaro.org> <58d052da-d1da-9e92-c9aa-0a12303a8663@polymtl.ca> Message-ID: <8516399b-0b23-c5e9-d7cc-126515b54121@linaro.org> Date: Tue, 29 Dec 2020 14:21:41 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: <58d052da-d1da-9e92-c9aa-0a12303a8663@polymtl.ca> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit 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: Luis Machado via Gdb-patches Reply-To: Luis Machado Cc: david.spickett@linaro.org Errors-To: gdb-patches-bounces@sourceware.org Sender: "Gdb-patches" On 12/27/20 12:32 AM, Simon Marchi wrote: > > > 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 suppose we could... :-) > > - 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 I don't mind the long names to be honest. What I find a bit annoying is having to work with longer names while the column limit is still 80. I've been hitting this issue a bit more with C++ and longer function names. Sometimes it is hard to break things up across multiple lines. > > And the "mtag" prefix... maybe "memory-tag", or "mem-tag" at least? It's not obvious > that the "m" stands for "memory", > In any case, I've changed the commands to spell it all completely and also switched from using "show" to using "print". >> >> 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. > Done now. >> --- >> 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? > Fixed. >> >> /* 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". > I've changed it for all commands. >> +} >> + >> +/* 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. > Leftovers from a previous strategy. I removed it now. >> + >> + /* 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"? > Yeah. I reused the code from the other function that accepts more than a couple digits. Fixed now. >> + >> + 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. > It should read "... we have the right type for the ...". Fixed now. >> + 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. No. Another leftover from earlier versions of the code. I've moved the check for whether we have tag checks on/off to the beginning of the function, so this doesn't make sense anymore. Fixed now. Given the amount of changes in this particular patch, I'm inclined to send a v4 just to make sure. > > Simon >