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

[Simon Marchi via Gdb-patches <gdb-patches@sourceware.org>]

> +/* 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?

>  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:

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

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

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
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.

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?

Simon