Re: [PATCH v3 12/24] AArch64: Implement memory tagging target methods for AArch64

[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:
> 
> - Added type parameter to the target method implementations.
> 
> --
> 
> The patch implements the memory tagging target hooks for AArch64, so we
> can handle MTE.
> 
> gdb/ChangeLog:
> 
> YYYY-MM-DD  Luis Machado  <luis.machado@linaro.org>
> 
> 	* Makefile.in (ALL_64_TARGET_OBS): Add arch/aarch64-mte-linux.o.
> 	(HFILES_NO_SRCDIR): Add arch/aarch64-mte-linux.h and
> 	nat/aarch64-mte-linux-ptrace.h.
> 	* aarch64-linux-nat.c: Include nat/aarch64-mte-linux-ptrace.h.
> 	(aarch64_linux_nat_target) <supports_memory_tagging>: New method
> 	override.
> 	<fetch_memtags>: New method override.
> 	<store_memtags>: New method override.
> 	(aarch64_linux_nat_target::supports_memory_tagging): New method.
> 	(aarch64_linux_nat_target::fetch_memtags): New method.
> 	(aarch64_linux_nat_target::store_memtags): New method.
> 	* arch/aarch64-mte-linux.c: New file.
> 	* arch/aarch64-mte-linux.h: Include gdbsupport/common-defs.h.
> 	(MTE_GRANULE_SIZE): Define.
> 	(get_tag_granules): New prototype.
> 	* configure.nat (NATDEPFILES): Add nat/aarch64-mte-linux-ptrace.o.
> 	* configure.tgt (aarch64*-*-linux*): Add arch/aarch64-mte-linux.o.
> 	* nat/aarch64-mte-linux-ptrace.c: New file.
> 	* nat/aarch64-mte-linux-ptrace.h: New file.
> ---
>  gdb/Makefile.in                    |   1 +
>  gdb/aarch64-linux-nat.c            |  50 ++++++++
>  gdb/arch/aarch64-mte-linux.c       |  34 +++++
>  gdb/arch/aarch64-mte-linux.h       |  10 ++
>  gdb/configure.nat                  |   3 +-
>  gdb/configure.tgt                  |   1 +
>  gdb/nat/aarch64-mte-linux-ptrace.c | 200 +++++++++++++++++++++++++++++
>  gdb/nat/aarch64-mte-linux-ptrace.h |  17 +++
>  8 files changed, 315 insertions(+), 1 deletion(-)
>  create mode 100644 gdb/arch/aarch64-mte-linux.c
>  create mode 100644 gdb/nat/aarch64-mte-linux-ptrace.c
> 
> diff --git a/gdb/Makefile.in b/gdb/Makefile.in
> index 4f08beea44..452e21b2b2 100644
> --- a/gdb/Makefile.in
> +++ b/gdb/Makefile.in
> @@ -693,6 +693,7 @@ ALL_64_TARGET_OBS = \
>  	amd64-windows-tdep.o \
>  	arch/aarch64.o \
>  	arch/aarch64-insn.o \
> +	arch/aarch64-mte-linux.o \
>  	arch/amd64.o \
>  	ia64-linux-tdep.o \
>  	ia64-tdep.o \
> diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
> index dea34da669..4edf5a0454 100644
> --- a/gdb/aarch64-linux-nat.c
> +++ b/gdb/aarch64-linux-nat.c
> @@ -52,6 +52,8 @@
>  
>  #include "arch/aarch64-mte-linux.h"
>  
> +#include "nat/aarch64-mte-linux-ptrace.h"
> +
>  #ifndef TRAP_HWBKPT
>  #define TRAP_HWBKPT 0x0004
>  #endif
> @@ -102,6 +104,16 @@ class aarch64_linux_nat_target final : public linux_nat_target
>      override;
>  
>    struct gdbarch *thread_architecture (ptid_t) override;
> +
> +  bool supports_memory_tagging () override;
> +
> +  /* Read memory allocation tags from memory via PTRACE.  */
> +  int fetch_memtags (CORE_ADDR address, size_t len,
> +		     gdb::byte_vector &tags, int type) override;
> +
> +  /* Write allocation tags to memory via PTRACE.  */
> +  int store_memtags (CORE_ADDR address, size_t len,
> +		     const gdb::byte_vector &tags, int type) override;
>  };
>  
>  static aarch64_linux_nat_target the_aarch64_linux_nat_target;
> @@ -1050,6 +1062,44 @@ aarch64_linux_nat_target::thread_architecture (ptid_t ptid)
>    return gdbarch_find_by_info (info);
>  }
>  
> +/* Implement the "supports_memory_tagging" target_ops method.  */
> +
> +bool
> +aarch64_linux_nat_target::supports_memory_tagging ()
> +{
> +  return (linux_get_hwcap2 (this) & HWCAP2_MTE) != 0;
> +}
> +
> +/* Implement the "fetch_memtags" target_ops method.  */
> +
> +int
> +aarch64_linux_nat_target::fetch_memtags (CORE_ADDR address, size_t len,
> +					 gdb::byte_vector &tags, int type)
> +{
> +  int tid = inferior_ptid.lwp ();

I believe this should use get_ptrace_pid (same in the store version).

> +
> +  /* Allocation tags?  */
> +  if (type == 1)
> +    return aarch64_mte_fetch_memtags (tid, address, len, tags);
> +
> +  return 1;

What's the logic of this "return 1"?  Does this mean "success", and
is this what we want to do when it's an unhandled type?  Oh, now that I
read the doc for aarch64_mte_fetch_memtags, I see that 1 means failure.
See comment below.

> diff --git a/gdb/arch/aarch64-mte-linux.h b/gdb/arch/aarch64-mte-linux.h
> index 4124e80543..e555f0af19 100644
> --- a/gdb/arch/aarch64-mte-linux.h
> +++ b/gdb/arch/aarch64-mte-linux.h
> @@ -20,6 +20,8 @@
>  #ifndef ARCH_AARCH64_LINUX_H
>  #define ARCH_AARCH64_LINUX_H
>  
> +#include "gdbsupport/common-defs.h"
> +
>  /* Feature check for Memory Tagging Extension.  */
>  #ifndef HWCAP2_MTE
>  #define HWCAP2_MTE  (1 << 18)
> @@ -28,4 +30,12 @@
>  /* The MTE regset consists of a single 64-bit register.  */
>  #define AARCH64_LINUX_SIZEOF_MTE 8
>  
> +/* We have one tag per 16 bytes of memory.  */
> +#define MTE_GRANULE_SIZE 16

I'd suggest prefixing this, AARCH64_MTE_GRANULE_SIZE.

> +
> +/* 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);

And aarch64_mte_get_tag_granules.

> +/* Helper function to display various possible errors when reading
> +   MTE tags.  */
> +
> +static void
> +aarch64_mte_linux_peek_error (int error)
> +{
> +  switch (error)
> +    {
> +    case EIO:
> +      perror_with_name (_("PEEKMTETAGS not supported"));
> +      break;
> +    case EFAULT:
> +      perror_with_name (_("Couldn't fetch allocation tags"));
> +      break;
> +    case EOPNOTSUPP:
> +      perror_with_name (_("PROT_ME not enabled for requested address"));
> +    default:
> +      perror_with_name (_("Unknown MTE error"));
> +      break;
> +    }
> +}
> +
> +/* Helper function to display various possible errors when writing
> +   MTE tags.  */
> +
> +static void
> +aarch64_mte_linux_poke_error (int error)
> +{
> +  switch (error)
> +    {
> +    case EIO:
> +      perror_with_name (_("POKEMTETAGS not supported"));
> +      break;
> +    case EFAULT:
> +      perror_with_name (_("Couldn't store allocation tags"));
> +      break;
> +    case EOPNOTSUPP:
> +      perror_with_name (_("PROT_ME not enabled for requested address"));
> +    default:
> +      perror_with_name (_("Unknown MTE error"));
> +      break;
> +    }
> +}

These functions should probably be marked with ATTRIBUTE_NORETURN.

> +
> +/* Helper to prepare a vector of tags to be passed on to the kernel.  The
> +   main purpose of this function is to optimize the number of calls to
> +   ptrace if we're writing too many tags at once, like a pattern fill
> +   request.
> +
> +   Return a vector of tags of up to MAX_SIZE size, containing the tags that
> +   must be passed on to the kernel, extracted from TAGS, starting at POS.
> +   GRANULES is the number of tag granules to be modified.  */
> +
> +static gdb::byte_vector
> +prepare_tag_vector (size_t granules, const gdb::byte_vector &tags, size_t pos,
> +		    size_t max_size)
> +{
> +  gdb::byte_vector t;
> +
> +  if (granules == 0)
> +    {
> +      t.clear ();

That clear seems unnecessary.

> diff --git a/gdb/nat/aarch64-mte-linux-ptrace.h b/gdb/nat/aarch64-mte-linux-ptrace.h
> index 099b6440ca..7ba6f014f6 100644
> --- a/gdb/nat/aarch64-mte-linux-ptrace.h
> +++ b/gdb/nat/aarch64-mte-linux-ptrace.h
> @@ -30,4 +30,21 @@
>  #define PTRACE_POKEMTETAGS	  34
>  #endif
>  
> +/* Maximum number of tags to pass at once to the kernel.  */
> +#define TAGS_MAX_SIZE 4096

I think this define should have a more scoped named, like
AARCH64_MTE_TAGS_MAX_SIZE.

> +
> +/* Read the allocation tags from memory range [ADDRESS, ADDRESS + LEN)
> +   into TAGS.
> +
> +   Return 0 if successful and non-zero otherwise.  */
> +extern int aarch64_mte_fetch_memtags (int tid, CORE_ADDR address, size_t len,
> +				      gdb::byte_vector &tags);
> +
> +/* Write the TAGS allocation tags to the memory range
> +   [ADDRESS, ADDRESS + LEN).
> +
> +   Return 0 if successful and non-zero otherwise.  */
> +extern int aarch64_mte_store_memtags (int tid, CORE_ADDR address, size_t len,
> +				      const gdb::byte_vector &tags);

IWBN to use bool for the return value, true for success and false for failure.
And as mentioned in the first patch, the target methods should also use bool
for the return value and be documented (although with bool, it's pretty obvious
that "true" means success, unlike with ints where both ways are common).

Otherwise, LGTM.

Simon