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

[Alan Hayward via Gdb-patches <gdb-patches@sourceware.org>]

> On 29 Oct 2020, at 14:45, Luis Machado <luis.machado@linaro.org> wrote:
> 
> On 10/29/20 11:39 AM, Luis Machado wrote:
>> On 10/29/20 11:21 AM, Alan Hayward wrote:
>>> 
>>> 
>>>> On 22 Oct 2020, at 21:00, Luis Machado <luis.machado@linaro.org> 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 8c9e6c9f6c..33a08a2288 100644
>>>> --- a/gdb/Makefile.in
>>>> +++ b/gdb/Makefile.in
>>>> @@ -692,6 +692,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)
>>> 
>>> I’m a little unsure as to where the type is coming from. Who in the call stack
>>> is explicitly passing the value 1?
>> Someone invoking the target methods directly or invoking the gdbarch hooks. For example, in gdb/printcmd.c:
>>   if (gdbarch_set_memtags (target_gdbarch (), val, 0, tags,
>>                            tag_logical) != 0)
>> Or:
>>   std::string tag = gdbarch_memtag_to_string (target_gdbarch (),
>>                                               val, tag_type);
>>> It’s different from the LOGICAL and ALLOCATION enum values used elsewhere?
>> No. Just a different type (int). The remote layer shouldn't try to interpret these tag types though. It should just forward them to the other side.
> 
> Complementing the answer... It isn't the case at the moment, but one of the layers of the target could translate the generic enums (tag_logical/tag_allocation) to something different.
> 
> That's why I don't want the remote target to be aware of what these tag types mean.


Understandable, and it makes sense. Probably not worth the translation complexity until
there are additional types.


Alan.