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

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

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

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

> +
> +  /* 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 (_("<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.

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

Simon