Re: [PATCH] [AArch64] Make gdbserver register set selection dynamic

[Luis Machado <luis.machado@linaro.org>]

On 11/3/21 5:50 AM, Alan Hayward wrote:
> 
> 
>> On 1 Nov 2021, at 13:09, Luis Machado <luis.machado@linaro.org> wrote:
>>
>> The current register set selection mechanism for AArch64 is static, based
>> on a pre-populated array of register sets.
>>
>> This means that we might potentially probe register sets that are not
>> available. This is OK if the kernel errors out during ptrace, but probing the
>> tag_ctl register, for example, does not result in a ptrace error if the kernel
>> supports the tagged address ABI but not MTE (PR 28355).
>>
>> Making the register set selection dynamic, based on feature checks, solves
>> this and simplifies the code a bit. It allows us to list all of the register
>> sets only once, and pick and choose based on HWCAP/HWCAP2 or other properties.
>>
> 
> This is all much better than the existing code.
> The two sets was getting ugly now the number of optional features are building up.
> 
> On first reading of the code, using size==0 to disable regsets feels like a hack.
> Alternatively you could add a bool enabled flag. Having that you can now put the
> sizes in the static declaration and simplify aarch64_adjust_register_sets. But, that’d
> mean updating every target file, and it just adds to the memory used. So, it’s probably
> better as you’ve done it.

I agree the bool is a more elegant way of doing it. An improvement for 
the next release most likely. I didn't want to rework other targets 
because I also want to push this to GDB 11.

> 
> Very minor nit below, otherwise looks good to me.
> 
>> I plan to backport this fix to GDB 11 as well.
>>
>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28355
>> ---
>> gdb/arch/aarch64.h             |   9 ++
>> gdbserver/linux-aarch64-low.cc | 184 ++++++++++++++++++---------------
>> 2 files changed, 109 insertions(+), 84 deletions(-)
>>
>> diff --git a/gdb/arch/aarch64.h b/gdb/arch/aarch64.h
>> index 0eb702c5b5e..95edb664b55 100644
>> --- a/gdb/arch/aarch64.h
>> +++ b/gdb/arch/aarch64.h
>> @@ -22,6 +22,15 @@
>>
>> #include "gdbsupport/tdesc.h"
>>
>> +/* Holds information on what architectural features are available.  This is
>> +   used to select register sets.  */
>> +struct aarch64_features
>> +{
>> +  bool sve = false;
>> +  bool pauth = false;
>> +  bool mte = false;
>> +};
>> +
>> /* Create the aarch64 target description.  A non zero VQ value indicates both
>>     the presence of SVE and the Vector Quotient - the number of 128bit chunks in
>>     an SVE Z register.  HAS_PAUTH_P indicates the presence of the PAUTH
>> diff --git a/gdbserver/linux-aarch64-low.cc b/gdbserver/linux-aarch64-low.cc
>> index daccfef746e..0ad9d01101a 100644
>> --- a/gdbserver/linux-aarch64-low.cc
>> +++ b/gdbserver/linux-aarch64-low.cc
>> @@ -196,16 +196,6 @@ is_64bit_tdesc (void)
>>    return register_size (regcache->tdesc, 0) == 8;
>> }
>>
>> -/* Return true if the regcache contains the number of SVE registers.  */
>> -
>> -static bool
>> -is_sve_tdesc (void)
>> -{
>> -  struct regcache *regcache = get_thread_regcache (current_thread, 0);
>> -
>> -  return tdesc_contains_feature (regcache->tdesc, "org.gnu.gdb.aarch64.sve");
>> -}
>> -
>> static void
>> aarch64_fill_gregset (struct regcache *regcache, void *buf)
>> {
>> @@ -680,40 +670,6 @@ aarch64_target::low_new_fork (process_info *parent,
>>    *child->priv->arch_private = *parent->priv->arch_private;
>> }
>>
>> -/* Matches HWCAP_PACA in kernel header arch/arm64/include/uapi/asm/hwcap.h.  */
>> -#define AARCH64_HWCAP_PACA (1 << 30)
>> -
>> -/* Implementation of linux target ops method "low_arch_setup".  */
>> -
>> -void
>> -aarch64_target::low_arch_setup ()
>> -{
>> -  unsigned int machine;
>> -  int is_elf64;
>> -  int tid;
>> -
>> -  tid = lwpid_of (current_thread);
>> -
>> -  is_elf64 = linux_pid_exe_is_elf_64_file (tid, &machine);
>> -
>> -  if (is_elf64)
>> -    {
>> -      uint64_t vq = aarch64_sve_get_vq (tid);
>> -      unsigned long hwcap = linux_get_hwcap (8);
>> -      unsigned long hwcap2 = linux_get_hwcap2 (8);
>> -      bool pauth_p = hwcap & AARCH64_HWCAP_PACA;
>> -      /* MTE is AArch64-only.  */
>> -      bool mte_p = hwcap2 & HWCAP2_MTE;
>> -
>> -      current_process ()->tdesc
>> -	= aarch64_linux_read_description (vq, pauth_p, mte_p);
>> -    }
>> -  else
>> -    current_process ()->tdesc = aarch32_linux_read_description ();
>> -
>> -  aarch64_linux_get_debug_reg_capacity (lwpid_of (current_thread));
>> -}
>> -
>> /* Wrapper for aarch64_sve_regs_copy_to_reg_buf.  */
>>
>> static void
>> @@ -730,20 +686,35 @@ aarch64_sve_regs_copy_from_regcache (struct regcache *regcache, void *buf)
>>    return aarch64_sve_regs_copy_from_reg_buf (regcache, buf);
>> }
>>
>> +/* Array containing all the possible register sets for AArch64/Linux.  During
>> +   architecture setup, these will be checked against the HWCAP/HWCAP2 bits for
>> +   validity and enabled/disabled accordingly.
>> +
>> +   Their sizes are set to 0 here, but they will be adjusted later depending
>> +   on whether each register set is available or not.  */
>> static struct regset_info aarch64_regsets[] =
>> {
>> +  /* GPR registers.  */
>>    { PTRACE_GETREGSET, PTRACE_SETREGSET, NT_PRSTATUS,
>> -    sizeof (struct user_pt_regs), GENERAL_REGS,
>> +    0, GENERAL_REGS,
>>      aarch64_fill_gregset, aarch64_store_gregset },
>> +  /* Floating Point (FPU) registers.  */
>>    { PTRACE_GETREGSET, PTRACE_SETREGSET, NT_FPREGSET,
>> -    sizeof (struct user_fpsimd_state), FP_REGS,
>> +    0, FP_REGS,
>>      aarch64_fill_fpregset, aarch64_store_fpregset
>>    },
>> +  /* Scalable Vector Extension (SVE) registers.  */
>> +  { PTRACE_GETREGSET, PTRACE_SETREGSET, NT_ARM_SVE,
>> +    0, EXTENDED_REGS,
>> +    aarch64_sve_regs_copy_from_regcache, aarch64_sve_regs_copy_to_regcache
>> +  },
>> +  /* PAC registers.  */
>>    { PTRACE_GETREGSET, PTRACE_SETREGSET, NT_ARM_PAC_MASK,
>> -    AARCH64_PAUTH_REGS_SIZE, OPTIONAL_REGS,
>> -    NULL, aarch64_store_pauthregset },
>> +    0, OPTIONAL_REGS,
>> +    nullptr, aarch64_store_pauthregset },
>> +  /* Tagged address control / MTE registers.  */
>>    { PTRACE_GETREGSET, PTRACE_SETREGSET, NT_ARM_TAGGED_ADDR_CTRL,
>> -    AARCH64_LINUX_SIZEOF_MTE, OPTIONAL_REGS, aarch64_fill_mteregset,
>> +    0, OPTIONAL_REGS, aarch64_fill_mteregset,
>>      aarch64_store_mteregset },
> 
> Minor nit - the newlines in the PAC and MTE sets are inconsistent with the ones above.
> 

Indeed. I'll get this adjusted. Thanks!