Re: [PATCH v3 14/24] AArch64: Implement the memory tagging gdbarch hooks

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

On 2020-11-09 12:04 p.m., Luis Machado via Gdb-patches wrote:
> 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_linux_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 (make_ltag_bits, make_ltag)
> 	(aarch64_linux_set_ltag, aarch64_linux_get_ltag): New functions.
> 	* arch/aarch64-mte-linux.h (MTE_LOGICAL_TAG_START_BIT)
> 	(MTE_LOGICAL_MAX_VALUE): Define.
> 	(make_ltag_bits, make_ltag, aarch64_linux_set_ltag)
> 	(aarch64_linux_get_ltag): New prototype.
> ---
>  gdb/aarch64-linux-tdep.c     | 212 +++++++++++++++++++++++++++++++++++
>  gdb/arch/aarch64-mte-linux.c |  36 ++++++
>  gdb/arch/aarch64-mte-linux.h |  19 ++++
>  3 files changed, 267 insertions(+)
> 
> diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
> index 5f6fb42792..00c4f45035 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.
>  
>        +------------+  ^
> @@ -1437,6 +1441,189 @@ aarch64_linux_gcc_target_options (struct gdbarch *gdbarch)
>    return {};
>  }
>  
> +/* Helper to get the allocation tag from a 64-bit ADDRESS.
> +
> +   Return 0 for success and non-zero otherwise.  */
> +
> +static int
> +aarch64_linux_get_atag (CORE_ADDR address, CORE_ADDR *tag)
> +{

I'd suggest making the return value a bool.  Or, even better IMO
would be to make the return value gdb::optional<CORE_ADDR>, an
empty optional meaning failure.

> +  gdb::byte_vector tags;
> +
> +  /* Attempt to fetch the allocation tag.  */
> +  if (target_fetch_memtags (address, 0, tags, tag_allocation) != 0)
> +    return 1;
> +
> +  /* Only one tag should've been returned.  Make sure we got exactly that.  */
> +  gdb_assert (tags.size () == 1);
> +
> +  /* Although our tags are 4 bits in size, they are stored in a
> +     byte.  */
> +  *tag = tags[0];
> +
> +  return 0;
> +}
> +
> +/* Implement the tagged_address_p gdbarch method.  */
> +
> +static bool
> +aarch64_linux_tagged_address_p (struct gdbarch *gdbarch, struct value *address)
> +{
> +  gdb_assert (address != nullptr);
> +
> +  CORE_ADDR addr = value_as_address (address);
> +
> +  /* Remove the top byte for the memory range check.  */
> +  addr = address_significant (gdbarch, addr);
> +
> +  /* Check if the page that contains ADDRESS is mapped with PROT_MTE.  */
> +  if (!linux_address_in_memtag_page (addr))
> +    return false;
> +
> +  /* We have a valid tag in the top byte of the 64-bit address.  */
> +  return true;
> +}
> +
> +/* Implement the memtag_mismatch_p gdbarch method.  */
> +
> +static bool
> +aarch64_linux_memtag_mismatch_p (struct gdbarch *gdbarch,
> +				 struct value *address)
> +{
> +  gdb_assert (address != nullptr);
> +
> +  /* Make sure we are dealing with a tagged address to begin with.  */
> +  if (!aarch64_linux_tagged_address_p (gdbarch, address))
> +    return false;
> +
> +  CORE_ADDR addr = value_as_address (address);
> +
> +  /* Fetch the allocation tag for ADDRESS.  */
> +  CORE_ADDR atag = 0;
> +
> +  if (aarch64_linux_get_atag (addr, &atag) != 0)
> +    return false;
> +
> +  /* Fetch the logical tag for ADDRESS.  */
> +  gdb_byte ltag = aarch64_linux_get_ltag (addr);
> +
> +  /* Are the tags the same?  */
> +  if (ltag == atag)
> +    return false;
> +
> +  return true;

That could very well be

  return ltag != atag;

I'm wondering about the potential performance impacts of all this.  When 
memory tagging is in use, how often are the memtag functions expected to be
called?  By the looks of it, it seems like checking for a mismatch does
2 open and parse of /proc/pid/smaps.  Is that right, and is this function
something that is going to be called often, say, when printing values?

> +}
> +
> +/* Implement the set_memtags gdbarch method.  */
> +
> +static int
> +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 0;
> +
> +  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_linux_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 1;
> +
> +      /* 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 = get_tag_granules (addr, length, MTE_GRANULE_SIZE);
> +      size_t n = tags.size ();
> +
> +      if (g < n)
> +	{
> +	  warning (_("Got more tags than memory granules.  Tags will be "
> +		     "truncated."));
> +	}

Remove curly braces.

> diff --git a/gdb/arch/aarch64-mte-linux.h b/gdb/arch/aarch64-mte-linux.h
> index e555f0af19..5c5783f28b 100644
> --- a/gdb/arch/aarch64-mte-linux.h
> +++ b/gdb/arch/aarch64-mte-linux.h
> @@ -32,10 +32,29 @@
>  
>  /* We have one tag per 16 bytes of memory.  */
>  #define MTE_GRANULE_SIZE 16
> +#define MTE_LOGICAL_TAG_START_BIT   56
> +#define MTE_LOGICAL_MAX_VALUE	    0xf
>  
>  /* Return the number of tag granules in the memory range
>     [ADDR, ADDR + LEN) given GRANULE_SIZE.  */
>  extern size_t get_tag_granules (CORE_ADDR addr, size_t len,
>  				size_t granule_size);
>  
> +/* Return the 4-bit tag made from VALUE.  */
> +extern CORE_ADDR make_ltag_bits (CORE_ADDR value);

As mentioned previously, I'd suggest giving a name that's better scoped,
since these symbols are in a header file / exported.  Or if they are just
used in aarch64-mte-linux.c, move them there (the defines as well, if
possible).

Simon