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

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

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
>