Re: [PATCH v3 10/24] AArch64: Add MTE register set support for GDB and gdbserver

Luis Machado via Gdb-patches
2020-12-28T18:41:25Z

On 12/25/20 9:17 PM, Simon Marchi wrote:
>
>
> On 2020-11-09 12:04 p.m., Luis Machado via Gdb-patches wrote:
>> AArch64 MTE support in the Linux kernel exposes a new register
>> through ptrace. This patch adds the required code to support it.
>>
>> include/ChangeLog:
>>
>> YYYY-MM-DD Luis Machado <luis.machado@linaro.org>
>>
>> * elf/common.h (NT_ARM_TAGGED_ADDR_CTRL): Define.
>>
>> gdb/ChangeLog:
>>
>> YYYY-MM-DD Luis Machado <luis.machado@linaro.org>
>>
>> * aarch64-linux-nat.c (fetch_mteregs_from_thread): New function.
>> (store_mteregs_to_thread): New function.
>> (aarch64_linux_nat_target::fetch_registers): Update to call
>> fetch_mteregs_from_thread.
>> (aarch64_linux_nat_target::store_registers): Update to call
>> store_mteregs_to_thread.
>> * aarch64-tdep.c (aarch64_mte_register_names): New struct.
>> (aarch64_cannot_store_register): Handle MTE registers.
>> (aarch64_gdbarch_init): Initialize and setup MTE registers.
>> * aarch64-tdep.h (gdbarch_tdep) <mte_reg_base>: New field.
>> <has_mte>: New method.
>> * arch/aarch64-linux.h (AARCH64_LINUX_SIZEOF_MTE): Define.
>>
>> gdbserver/ChangeLog:
>>
>> YYYY-MM-DD Luis Machado <luis.machado@linaro.org>
>>
>> * linux-aarch64-low.cc (aarch64_fill_mteregset): New function.
>> (aarch64_store_mteregset): New function.
>> (aarch64_regsets): Add MTE register set entry.
>> (aarch64_sve_regsets): Add MTE register set entry.
>> ---
>> gdb/aarch64-linux-nat.c | 70 ++++++++++++++++++++++++++++++++++
>> gdb/aarch64-tdep.c | 24 ++++++++++++
>> gdb/aarch64-tdep.h | 9 +++++
>> gdb/arch/aarch64-mte-linux.h | 3 ++
>> gdbserver/linux-aarch64-low.cc | 29 ++++++++++++++
>> include/elf/common.h | 3 ++
>> 6 files changed, 138 insertions(+)
>>
>> diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
>> index 1392ec440c..dea34da669 100644
>> --- a/gdb/aarch64-linux-nat.c
>> +++ b/gdb/aarch64-linux-nat.c
>> @@ -461,6 +461,60 @@ fetch_pauth_masks_from_thread (struct regcache *regcache)
>> &pauth_regset[1]);
>> }
>>
>> +/* Fill GDB's register array with the MTE register values from
>> + the current thread. */
>> +
>> +static void
>> +fetch_mteregs_from_thread (struct regcache *regcache)
>> +{
>> + struct gdbarch_tdep *tdep = gdbarch_tdep (regcache->arch ());
>> + int regno = tdep->mte_reg_base;
>> +
>> + gdb_assert (regno != -1);
>> +
>> + uint64_t tag_ctl = 0;
>> + struct iovec iovec;
>> +
>> + iovec.iov_base = &tag_ctl;
>> + iovec.iov_len = sizeof (tag_ctl);
>> +
>> + int tid = regcache->ptid ().lwp ();
>
> Should this use get_ptrace_pid? Same in the store version.
>

I wasn't aware of that function. Thanks for the info.

>> + if (ptrace (PTRACE_GETREGSET, tid, NT_ARM_TAGGED_ADDR_CTRL,
>> + &iovec) != 0)
>
> The indentation is wrong on this last line (and it would all fit on one line).
>

Fixed in both the fetch and store functions to be on the same line.

>> diff --git a/include/elf/common.h b/include/elf/common.h
>> index fc672de9f2..334e9e301f 100644
>> --- a/include/elf/common.h
>> +++ b/include/elf/common.h
>> @@ -660,6 +660,9 @@
>> /* note name must be "LINUX". */
>> #define NT_ARM_PAC_MASK 0x406 /* AArch pointer authentication code masks */
>> /* note name must be "LINUX". */
>> +#define NT_ARM_TAGGED_ADDR_CTRL 0x409 /* AArch64 tagged address control
>> + (prctl()) */
>> + /* note name must be "LINUX". */
>> #define NT_ARC_V2 0x600 /* ARC HS accumulator/extra registers. */
>> /* note name must be "LINUX". */
>> #define NT_SIGINFO 0x53494749 /* Fields of siginfo_t. */
>
> What this changed approved by binutils?

Not yet. There was some back-and-forth on the name of this register set
with the kernel folks. I'll post it to the binutils list after fixing up
the current version.

>
>
> Otherwise, this LGTM.
>
> Simon
>