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

Luis Machado via Gdb-patches
2020-12-28T19:50:32Z

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.