Re: [PATCH v5 20/25] New memory-tag commands

[Luis Machado <luis.machado@linaro.org>]

On 2/5/21 1:49 AM, Simon Marchi wrote:
>> +/* Parse ARGS and extract ADDR and TAG.
>> +   ARGS should have format <expression> <tag bytes>.  */
>> +
>> +static void
>> +parse_with_logical_tag_input (const char *args, struct value **val,
>> +			      gdb::byte_vector &tags,
>> +			      value_print_options *print_opts)
>> +{
>> +  /* Fetch the address.  */
>> +  std::string address_string = extract_string_maybe_quoted (&args);
>> +
>> +  /* Parse the address into a value.  */
>> +  *val = process_print_command_args (address_string.c_str (), print_opts,
>> +				     true);
>> +
>> +  /* Fetch the tag bytes.  */
>> +  std::string tag_string = extract_string_maybe_quoted (&args);
>> +
>> +  /* Validate the input.  */
>> +  if (address_string.empty () || tag_string.empty ())
>> +    error (_("Missing arguments."));
>> +
>> +  if (tag_string.length () != 2)
>> +    error (_("Error parsing tags argument. The tag should be 2 digits."));
> 
> Can't an hypothetical architecture use more than one byte for a tag?
> 

It could. But it is unlikely, given architectures use spare bits of the 
virtual address.

We could solve this by having another gdbarch constant that tells us the 
size of a tag in a particular architecture. Then we check for size * 2 
characters. Or check for more than a couple characters, and let the 
architecture device if the tag value is sane or not.

What do you think?

>>   void _initialize_printcmd ();
>>   void
>>   _initialize_printcmd ()
>> @@ -2982,4 +3263,63 @@ Construct a GDB command and then evaluate it.\n\
>>   Usage: eval \"format string\", ARG1, ARG2, ARG3, ..., ARGN\n\
>>   Convert the arguments to a string as \"printf\" would, but then\n\
>>   treat this string as a command line, and evaluate it."));
>> +
>> +  /* Memory tagging commands.  */
>> +  add_prefix_cmd ("memory-tag", class_vars, memory_tag_command, _("\
>> +Generic command for printing and manipulating memory tag properties."),
>> +		  &memory_tag_list, "memory-tag ", 0, &cmdlist);
>> +  add_cmd ("print-logical-tag", class_vars,
>> +	   memory_tag_print_logical_tag_command,
>> +	   ("Print the logical tag for an address.\n\
>> +Usage: memory-tag print-logical-tag <address>.\n\
>> +<address> is an expression that evaluates to a pointer or memory address.\n\
>> +GDB will print the logical tag associated with <address>.  The tag\n\
> 
> Instead of saying "GDB will print the...", use the infinite "Print the
> ...".  I would also swap the two sentences to say what the command says
> first, before describing the argument.  Something like:

I don't have a preference, but the 'x' command, for example, does the 
opposite. I used it as reference.

--

   c = add_com ("x", class_vars, x_command, _("\
Examine memory: x/FMT ADDRESS.\n\
ADDRESS is an expression for the memory address to examine.\n\
FMT is a repeat count followed by a format letter and a size letter.\n\
Format letters are o(octal), x(hex), d(decimal), u(unsigned decimal),\n\
   t(binary), f(float), a(address), i(instruction), c(char), s(string)\n\
   and z(hex, zero padded on the left).\n\
Size letters are b(byte), h(halfword), w(word), g(giant, 8 bytes).\n\
The specified number of objects of the specified size are printed\n\
according to the format.  If a negative number is specified, memory is\n\
examined backward from the address.\n\n\
Defaults for format and size letters are those previously used.\n\
Default count is 1.  Default address is following last thing printed\n\
with this command or \"print\"."));

--

Besides...

> 
> Print the logical tag associated with a pointer.  POINTER is an
> expression that evaluates to a pointer.

...this is almost exactly the first line of the command documentation.

Looking at 'x', it feels to me that the second portion of the 
documentation describes what GDB will acomplish in a more verbose 
manner, since the command's output is already described in the first line.

Is there a standard for these command documentation bits somewhere?

> 
> Also, use capital letters to refer to arguments, in the help and in the
> error messages too.  See this series for reference:

I'll get these fixed.

> 
> https://sourceware.org/pipermail/gdb-patches/2018-September/152361.html
> 
>> +interpretation is architecture-specific."),
>> +	   &memory_tag_list);
>> +  add_cmd ("print-allocation-tag", class_vars,
>> +	   memory_tag_print_allocation_tag_command,
>> +	   _("Print the allocation tag for an address.\n\
>> +Usage: memory-tag print-allocation-tag <address>.\n\
>> +<address> is an expression that evaluates to a pointer or memory address.\n\
>> +GDB will print the allocation tag associated with <address>.  The tag\n\
>> +interpretation is architecture-specific."),
>> +	   &memory_tag_list);
>> +  add_cmd ("with-logical-tag", class_vars, memory_tag_with_logical_tag_command,
>> +	   _("Set the logical tag for an address.\n\
>> +Usage: memory-tag with-logical-tag <address> <tag>\n\
>> +<address> is an expression that evaluates to a pointer or memory address.\n\
>> +<tag> is a sequence of hex bytes that will be interpreted by the\n\
>> +architecture as a single memory tag.\n\
>> +GDB will set the logical tag for <address> to <tag>, and will print the\n\
>> +resulting address with the updated tag."),
>> +	   &memory_tag_list);
>> +  add_cmd ("set-allocation-tag", class_vars,
>> +	   memory_tag_set_allocation_tag_command,
>> +	   _("Set the allocation tag for an address.\n\
>> +Usage: memory-tag set-allocation-tag <address> <length> <tag_bytes>\n\
>> +<address> is an expression that evaluates to a pointer or memory address\n\
>> +<length> is the number of bytes that will get added to <address> to calculate\n\
>> +the memory range.\n\
>> +<tag bytes> is a sequence of hex bytes that will be interpreted by the\n\
>> +architecture as one or more memory tags.\n\
>> +Sets the tags of the memory range [<address>, <address> + <length>)\n\
>> +to the specified tag bytes.\n\
>> +\n\
>> +If the number of tags is greater than or equal to the number of tag granules\n\
>> +in the [<address>, <address> + <length) range, only the tags up to the\n\
>> +number of tag granules will be stored.\n\
>> +\n\
>> +If the number of tags is less than the number of tag granules, then the\n\
>> +command is a fill operation.  The tag bytes are interpreted as a pattern\n\
>> +that will get repeated until the number of tag granules in the memory range\n\
>> +[<address>, <address> + <length>] is stored to."),
>> +	   &memory_tag_list);
>> +  add_cmd ("check", class_vars, memory_tag_check_command,
>> +	   _("Validate the logical tag against the allocation tag.\n\
>> +Usage: memory-tag check <address>\n\
>> +<address> is an expression that evaluates to a pointer or memory address\n\
>> +GDB will fetch the logical and allocation tags for <address> and will\n\
>> +compare them for equality.  If the tags do not match, GDB will show\n\
>> +additional information about the mismatch."),
>> +	   &memory_tag_list);
>>   }
> 
> I don't know if this has been discussed before, but something confuses
> me about saying "evaluates to a pointer or memory address".  The way I

It wasn't, and I appreciate you putting the time to go through these 
patches and making these observations. Thanks a lot.

> understand it is that logical tags are encoded into pointers, whereas
> allocation tags are associated to memory addresses.  So I think it would
> make more sense to use "pointer" for things related to logical tags and
> "address" for things related to allocation tags.

That's the correct understanding. Though in GDB, sometimes things get a 
bit fuzzy, and I may have exposed some of GDB's inner interpretation here.

For example, when you pass in a constant to a command. Is that a pointer 
or an address? Maybe an address that gets converted to a typed pointer?

For simplicity, I think we can go for pointers when talking about memory 
tags and memory when talking about allocation tags.

> 
> With "memory-tag check", you would pass a pointer, and it verifies
> whether the pointer's logical tag matches the pointed to memory address'
> allocation tag, so that one involves both.
> 
> Otherwise, if the distinction is not so important or just confusing,
> maybe just use "address" and drop "pointer", to make the text lighter?

I'll revise the documentation and address this. My goal is to have 
something short and meaningful.