Re: [PATCH v5 15/25] AArch64: Implement the memory tagging gdbarch hooks

[Simon Marchi <simon.marchi@polymtl.ca>]

On 2021-01-27 3:21 p.m., Luis Machado via Gdb-patches wrote:
> Updates on v4:
> 
> - Update function names to be more scoped.
> - Use of gdb::optional values.
> - Fixed up gdbarch hooks.
> 
> Updates on v2:
> 
> - Update target methods to contain a tag type parameter.
> 
> --
> 
> This patch implements the memory tagging gdbarch hooks for AArch64, for
> the MTE feature.
> 
> gdb/ChangeLog:
> 
> YYYY-MM-DD  Luis Machado  <luis.machado@linaro.org>
> 
> 	* aarch64-linux-tdep.c: Include target.h, arch-utils.h, value.h.
> 	(aarch64_mte_get_atag, aarch64_linux_tagged_address_p)
> 	(aarch64_linux_memtag_mismatch_p, aarch64_linux_set_memtags)
> 	(aarch64_linux_get_memtag, aarch64_linux_memtag_to_string): New
> 	functions.
> 	(aarch64_linux_init_abi): Initialize MTE-related gdbarch hooks.
> 	* arch/aarch64-mte-linux.c (aarch64_mte_make_ltag_bits)
> 	(aarch64_mte_make_ltag, aarch64_linux_set_ltag)
> 	(aarch64_linux_get_ltag): New functions.
> 	* arch/aarch64-mte-linux.h (AARCH64_MTE_LOGICAL_TAG_START_BIT)
> 	(AARCH64_MTE_LOGICAL_MAX_VALUE): Define.
> 	(aarch64_mte_make_ltag_bits, aarch64_mte_make_ltag)
> 	(aarch64_mte_set_ltag, aarch64_mte_get_ltag): New prototypes.
> ---
>  gdb/aarch64-linux-tdep.c     | 210 +++++++++++++++++++++++++++++++++++
>  gdb/arch/aarch64-mte-linux.c |  41 ++++++-
>  gdb/arch/aarch64-mte-linux.h |  19 ++++
>  3 files changed, 269 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
> index f113d1ea30..2fe88cf016 100644
> --- a/gdb/aarch64-linux-tdep.c
> +++ b/gdb/aarch64-linux-tdep.c
> @@ -30,6 +30,7 @@
>  #include "symtab.h"
>  #include "tramp-frame.h"
>  #include "trad-frame.h"
> +#include "target.h"
>  #include "target/target.h"
>  
>  #include "regcache.h"
> @@ -46,6 +47,9 @@
>  
>  #include "arch/aarch64-mte-linux.h"
>  
> +#include "arch-utils.h"
> +#include "value.h"
> +
>  /* Signal frame handling.
>  
>        +------------+  ^
> @@ -1513,6 +1517,187 @@ aarch64_linux_gcc_target_options (struct gdbarch *gdbarch)
>    return {};
>  }
>  
> +/* Helper to get the allocation tag from a 64-bit ADDRESS.
> +
> +   Return the allocation tag if successful and nullopt otherwise.  */
> +
> +static gdb::optional<CORE_ADDR>
> +aarch64_mte_get_atag (CORE_ADDR address)
> +{
> +  gdb::byte_vector tags;
> +
> +  /* Attempt to fetch the allocation tag.  */
> +  if (!target_fetch_memtags (address, 0, tags, tag_allocation))
> +    return {};
> +
> +  /* Only one tag should've been returned.  Make sure we got exactly that.  */
> +  gdb_assert (tags.size () == 1);

The tags are returned by the target, possibly the remote target.  That
means that you could make GDB assert by returning it a memtag packet
with the wrong number of bytes.  And we don't want it to be possible to
make GDB assert when feeding it bad input.

I'd probably use error() here just for this reason.

> +/* Implement the set_memtags gdbarch method.  */
> +
> +static bool
> +aarch64_linux_set_memtags (struct gdbarch *gdbarch, struct value *address,
> +			   size_t length, const gdb::byte_vector &tags,
> +			   enum memtag_type tag_type)
> +{
> +  if (tags.empty ())
> +    return true;

Can this can really happen?

> +
> +  gdb_assert (address != nullptr);
> +
> +  CORE_ADDR addr = value_as_address (address);
> +
> +  /* Set the logical tag or the allocation tag.  */
> +  if (tag_type == tag_logical)
> +    {
> +      /* When setting logical tags, we don't care about the length, since
> +	 we are only setting a single logical tag.  */
> +      addr = aarch64_mte_set_ltag (addr, tags[0]);
> +
> +      /* Update the value's content with the tag.  */
> +      enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> +      gdb_byte *srcbuf = value_contents_raw (address);
> +      store_unsigned_integer (srcbuf, sizeof (addr), byte_order, addr);
> +    }
> +  else
> +    {
> +      /* Make sure we are dealing with a tagged address to begin with.  */
> +      if (!aarch64_linux_tagged_address_p (gdbarch, address))
> +	return false;
> +
> +      /* With G being the number of tag granules and N the number of tags
> +	 passed in, we can have the following cases:
> +
> +	 1 - G == N: Store all the N tags to memory.
> +
> +	 2 - G < N : Warn about having more tags than granules, but write G
> +		     tags.
> +
> +	 3 - G > N : This is a "fill tags" operation.  We should use the tags
> +		     as a pattern to fill the granules repeatedly until we have
> +		     written G tags to memory.
> +      */
> +
> +      size_t g = aarch64_mte_get_tag_granules (addr, length,
> +					       AARCH64_MTE_GRANULE_SIZE);
> +      size_t n = tags.size ();
> +
> +      if (g < n)
> +	warning (_("Got more tags than memory granules.  Tags will be "
> +		   "truncated."));
> +      else if (g > n)
> +	warning (_("Using tag pattern to fill memory range."));
> +
> +      if (!target_store_memtags (addr, length, tags, tag_allocation))
> +	return false;
> +    }
> +  return true;

I just realized that the gdbarch methods are overloaded, as they are
used to both set/get the logical tag of the pointer and set/get the
allocation tags of the allocated memory.  I think it would be easier to
understand to have four methods, get/set the local tags and get/set the
allocation tags.

The documentation in gdbarch.sh for get_memtag also only talks about the
logical tag (extracting the tag from the address).

Since this is internal stuff, I suggest we leave this patch as-is to
avoid some more churn, but please consider addressing it as a follow-up
patch to the series, if you agree.

> +}
> +
> +/* Implement the get_memtag gdbarch method.  */
> +
> +static struct value *
> +aarch64_linux_get_memtag (struct gdbarch *gdbarch, struct value *address,
> +			  enum memtag_type tag_type)
> +{
> +  gdb_assert (address != nullptr);
> +
> +  CORE_ADDR addr = value_as_address (address);
> +  CORE_ADDR tag = 0;
> +
> +  /* Get the logical tag or the allocation tag.  */
> +  if (tag_type == tag_logical)
> +    tag = aarch64_mte_get_ltag (addr);
> +  else
> +    {
> +      /* Make sure we are dealing with a tagged address to begin with.  */
> +      if (!aarch64_linux_tagged_address_p (gdbarch, address))
> +	return nullptr;
> +
> +      gdb::optional<CORE_ADDR> atag = aarch64_mte_get_atag (addr);
> +
> +      if (!atag.has_value ())
> +	return nullptr;
> +
> +      tag = *atag;
> +    }
> +
> +  /* Convert the tag to a value.  */
> +  return value_from_ulongest (builtin_type (gdbarch)->builtin_unsigned_int,
> +			      tag);
> +}
> +
> +/* Implement the memtag_to_string gdbarch method.  */
> +
> +static std::string
> +aarch64_linux_memtag_to_string (struct gdbarch *gdbarch,
> +				struct value *address,
> +				enum memtag_type tag_type)
> +{
> +  gdb_assert (address != nullptr);
> +
> +  struct value *v_tag = aarch64_linux_get_memtag (gdbarch, address, tag_type);
> +
> +  if (v_tag == nullptr && tag_allocation)
> +    error (_("Error getting tag from target"));
> +
> +  CORE_ADDR tag = value_as_address (v_tag);
> +
> +  return string_printf ("0x%s", phex_nz (tag, sizeof (tag)));
> +}

I'll see in the following patches how gdbarch_linux_memtag_to_string is
used, but at first glance I think it would make sense for this method to
have the prototype:

  std::string gdbarch_memtag_to_string (struct gdbarch *gdbarch, value *tag);

Meaning that you would pass it a tag you already fetched (using
gdbarch_get_memtag).  This way if you already have a tag as a local
variable and you want to format it as a string, it doesn't require going
to the target.  I don't think you would need the tag type parameter in
this case, as we wouldn't care where the tag comes from, we'd just use
the passed-in value.

Also something that can be done as a follow-up patch.

> +
>  static void
>  aarch64_linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
>  {
> @@ -1570,6 +1755,31 @@ aarch64_linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
>       data associated with the address.  */
>    set_gdbarch_significant_addr_bit (gdbarch, 56);
>  
> +  /* MTE-specific settings and hooks.  */
> +  if (tdep->has_mte ())
> +    {
> +      /* Register a hook for checking if an address is tagged or not.  */
> +      set_gdbarch_tagged_address_p (gdbarch, aarch64_linux_tagged_address_p);
> +
> +      /* Register a hook for checking if there is a memory tag match.  */
> +      set_gdbarch_memtag_matches_p (gdbarch,
> +				    aarch64_linux_memtag_matches_p);
> +
> +      /* Register a hook for setting the logical/allocation tags for
> +	 a range of addresses.  */
> +      set_gdbarch_set_memtags (gdbarch, aarch64_linux_set_memtags);
> +
> +      /* Register a hook for extracting the logical/allocation tag from an
> +	 address.  */
> +      set_gdbarch_get_memtag (gdbarch, aarch64_linux_get_memtag);
> +
> +      /* Set the allocation tag granule size to 16 bytes.  */
> +      set_gdbarch_memtag_granule_size (gdbarch, AARCH64_MTE_GRANULE_SIZE);
> +
> +      /* Register a hook for converting a memory tag to a string.  */
> +      set_gdbarch_memtag_to_string (gdbarch, aarch64_linux_memtag_to_string);
> +    }
> +
>    /* Initialize the aarch64_linux_record_tdep.  */
>    /* These values are the size of the type that will be used in a system
>       call.  They are obtained from Linux Kernel source.  */
> diff --git a/gdb/arch/aarch64-mte-linux.c b/gdb/arch/aarch64-mte-linux.c
> index ede5f5f2b9..4461864a32 100644
> --- a/gdb/arch/aarch64-mte-linux.c
> +++ b/gdb/arch/aarch64-mte-linux.c
> @@ -22,7 +22,8 @@
>  /* See arch/aarch64-mte-linux.h */
>  
>  size_t
> -aarch64_mte_get_tag_granules (CORE_ADDR addr, size_t len, size_t granule_size)
> +aarch64_mte_get_tag_granules (CORE_ADDR addr, size_t len,
> +				    size_t granule_size)

Unintended change.

>  {
>    /* Start address */
>    CORE_ADDR s_addr = align_down (addr, granule_size);
> @@ -32,3 +33,41 @@ aarch64_mte_get_tag_granules (CORE_ADDR addr, size_t len, size_t granule_size)
>    /* We always have at least 1 granule.  */
>    return 1 + (e_addr - s_addr) / granule_size;
>  }
> +
> +/* See arch/aarch64-mte-linux.h */
> +
> +CORE_ADDR
> +aarch64_mte_make_ltag_bits (CORE_ADDR value)
> +{
> +  return value & AARCH64_MTE_LOGICAL_MAX_VALUE;
> +}
> +
> +/* See arch/aarch64-mte-linux.h */
> +
> +CORE_ADDR
> +aarch64_mte_make_ltag (CORE_ADDR value)
> +{
> +  return aarch64_mte_make_ltag_bits (value)
> +	 << AARCH64_MTE_LOGICAL_TAG_START_BIT;

We would usually use parenthesis when wrapping this:

  return (aarch64_mte_make_ltag_bits (value)
	  << AARCH64_MTE_LOGICAL_TAG_START_BIT);

The argument being that Emacs aligns it correctly if you do that (not
that I care :)).

The patch LGTM with the nits fixed.

Simon