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

[Luis Machado via Gdb-patches <gdb-patches@sourceware.org>]

On 12/25/20 9:33 PM, Simon Marchi wrote:
> 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).
> 

Indeed. Fixed now.

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

All of these were supposed to return error codes. But right now we don't 
have a list of possible codes, therefore it is basically 0/1 for 
success/failure. I've switched to using bool.

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

Done now.

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

Done.

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

Attribute added.

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

You're right. Removed now.

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

That makes sense too.

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

Thanks for the review and suggestions so far. They were very useful.