Re: [PATCH v3 19/24] New mtag commands

[Luis Machado via Gdb-patches <gdb-patches@sourceware.org>]

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 <expression>: Shows the logical tag for a particular address.
>>
>> - mtag withltag <expression> <tag>: Prints the address tagged with the logical
>>    tag <tag>.
>>
>> - mtag showatag <expression>: Shows the allocation tag for a particular address.
>>
>> - mtag setatag <expression> <length> <tags>: Sets one or more allocation tags to
>>    the specified tags.
>>
>> - mtag check <expression>: Check if the logical tag in <address> 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  <luis.machado@linaro.org>
>>
>> 	* 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  <luis.machado@linaro.org>
>>
>> 	* 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 <expression> <tag bytes>.  */
>> +
>> +static void
>> +parse_withltag_input (const char *args, struct value **val,
>> +		      gdb::byte_vector &tags, value_print_options *print_opts)
>> +{
>> +  /* Given <expression> can be reasonably complex, we parse things backwards
>> +     so we can isolate the <tag bytes> 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 (_("<address> <tag>"));
>> +
>> +  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 <address> <length> <tags>.  */
>> +
>> +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 (_("<starting address> <length> <tag bytes>"));
>> +
>> +  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
>