Re: [PATCH 04/12] Add an arm-tls feature which includes the tpidruro register from CP15.

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

On 4/19/22 17:18, John Baldwin wrote:
> On 4/14/22 3:23 AM, Luis Machado wrote:
>> On 4/13/22 00:36, John Baldwin wrote:
>>> On 4/4/22 1:01 AM, Luis Machado wrote:
>>>> Hi,
>>>>
>>>> On 3/23/22 21:00, John Baldwin wrote:
>>>>> ---
>>>>>     gdb/arch/aarch32.c           |  2 ++
>>>>>     gdb/arch/arm.c               |  6 +++++-
>>>>>     gdb/arch/arm.h               |  7 ++++---
>>>>>     gdb/arm-fbsd-tdep.c          |  4 ++--
>>>>>     gdb/arm-linux-nat.c          |  6 +++---
>>>>>     gdb/arm-linux-tdep.c         |  4 ++--
>>>>>     gdb/arm-netbsd-nat.c         |  4 ++--
>>>>>     gdb/arm-tdep.c               | 20 +++++++++++++++-----
>>>>>     gdb/arm-tdep.h               |  2 +-
>>>>>     gdb/features/Makefile        |  1 +
>>>>>     gdb/features/arm/arm-tls.c   | 14 ++++++++++++++
>>>>>     gdb/features/arm/arm-tls.xml | 11 +++++++++++
>>>>>     12 files changed, 62 insertions(+), 19 deletions(-)
>>>>>     create mode 100644 gdb/features/arm/arm-tls.c
>>>>>     create mode 100644 gdb/features/arm/arm-tls.xml
>>>>>
>>>>> diff --git a/gdb/arch/aarch32.c b/gdb/arch/aarch32.c
>>>>> index 0c544d381f1..4d6ffb44a15 100644
>>>>> --- a/gdb/arch/aarch32.c
>>>>> +++ b/gdb/arch/aarch32.c
>>>>> @@ -19,6 +19,7 @@
>>>>>     #include "aarch32.h"
>>>>>     #include "../features/arm/arm-core.c"
>>>>> +#include "../features/arm/arm-tls.c"
>>>>>     #include "../features/arm/arm-vfpv3.c"
>>>>>     /* See aarch32.h.  */
>>>>> @@ -38,6 +39,7 @@ aarch32_create_target_description ()
>>>>>       /* Create a vfpv3 feature, then a blank NEON feature.  */
>>>>>       regnum = create_feature_arm_arm_vfpv3 (tdesc.get (), regnum);
>>>>>       tdesc_create_feature (tdesc.get (), "org.gnu.gdb.arm.neon");
>>>>> +  regnum = create_feature_arm_arm_tls (tdesc.get (), regnum);
>>>>>       return tdesc.release ();
>>>>>     }
>>>>> diff --git a/gdb/arch/arm.c b/gdb/arch/arm.c
>>>>> index 126e46a950a..15b600e22f4 100644
>>>>> --- a/gdb/arch/arm.c
>>>>> +++ b/gdb/arch/arm.c
>>>>> @@ -22,6 +22,7 @@
>>>>>     #include "arm.h"
>>>>>     #include "../features/arm/arm-core.c"
>>>>> +#include "../features/arm/arm-tls.c"
>>>>>     #include "../features/arm/arm-vfpv2.c"
>>>>>     #include "../features/arm/arm-vfpv3.c"
>>>>>     #include "../features/arm/xscale-iwmmxt.c"
>>>>> @@ -373,7 +374,7 @@ shifted_reg_val (struct regcache *regcache,
>>>>> unsigned long inst,
>>>>>     /* See arch/arm.h.  */
>>>>>     target_desc *
>>>>> -arm_create_target_description (arm_fp_type fp_type)
>>>>> +arm_create_target_description (arm_fp_type fp_type, bool tls)
>>>>>     {
>>>>>       target_desc_up tdesc = allocate_target_description ();
>>>>> @@ -409,6 +410,9 @@ arm_create_target_description (arm_fp_type 
>>>>> fp_type)
>>>>>           error (_("Invalid Arm FP type: %d"), fp_type);
>>>>>         }
>>>>> +  if (tls)
>>>>> +    regnum = create_feature_arm_arm_tls (tdesc.get (), regnum);
>>>>> +
>>>>>       return tdesc.release ();
>>>>>     }
>>>>> diff --git a/gdb/arch/arm.h b/gdb/arch/arm.h
>>>>> index f75470e7572..32f29b20d33 100644
>>>>> --- a/gdb/arch/arm.h
>>>>> +++ b/gdb/arch/arm.h
>>>>> @@ -49,6 +49,7 @@ enum gdb_regnum {
>>>>>       ARM_D0_REGNUM,        /* VFP double-precision registers.  */
>>>>>       ARM_D31_REGNUM = ARM_D0_REGNUM + 31,
>>>>>       ARM_FPSCR_REGNUM,
>>>>> +  ARM_TPIDRURO_REGNUM,
>>>>>       /* Other useful registers.  */
>>>>>       ARM_FP_REGNUM = 11,        /* Frame register in ARM code, if
>>>>> used.  */
>>>>> @@ -65,8 +66,8 @@ enum arm_register_counts {
>>>>>       ARM_NUM_ARG_REGS = 4,
>>>>>       /* Number of floating point argument registers.  */
>>>>>       ARM_NUM_FP_ARG_REGS = 4,
>>>>> -  /* Number of registers (old, defined as ARM_FPSCR_REGNUM + 1.  */
>>>>> -  ARM_NUM_REGS = ARM_FPSCR_REGNUM + 1
>>>>> +  /* Number of registers (old, defined as ARM_TPIDRURO_REGNUM + 
>>>>> 1.  */
>>>>> +  ARM_NUM_REGS = ARM_TPIDRURO_REGNUM + 1
>>>>>     };
>>>>
>>>> I'm attempting to move away from these hardcoded register numbers. If
>>>> there are optional features, that means ARM_NUM_REGS won't reflect the
>>>> reality anymore, and there may be holes in the register numbering.
>>>>
>>>> For example, a bare metal target may not have ARM_TPIDRURO_REGNUM. The
>>>> correct way is to account for it dynamically, similar to what we do 
>>>> with
>>>> MVE (and to what we do for pauth, MTE and your TLS handling).
>>>
>>> Note that these constants aren't required for the remote protocol
>>> however as
>>> GDB's remote target figures out its own mapping between remote register
>>> numbers and the internal numbers used in target descriptions.  Having 
>>> fixed
>>> values means one can use constant register_map_entry structures that can
>>> be reused (e.g. I often reuse the structures from <foo>-fbsd-tdep.c 
>>> files
>>> in the <foo>-fbsd-nat.c file to handle a register set in a native 
>>> target).
>>>
>>> Other arches work fine with holes in the register number space (e.g.
>>> x86 uses fixed constants for various optional register sets like the
>>> different sets of vector registers).
>>>
>>
>> Indeed. It is true that these holes have no negative impact other than
>> "maint print registers" showing empty entries and the number of
>> registers being slightly misleading.
>>
>> The register_map_entry structures work nicely, but they don't provide a
>> good way to track pseudo registers alongside the real feature registers.
>> Having more clear boundaries between each feature and the registers and
>> pseudo-registers in them looks cleaner to me.
>>
>> Some time ago I disentangled the handling of pseudo registers for 32-bit
>> arm, as it started to get a bit chaotic.
>>
>> Most of the feature handling tends to happen in generic arm-tdep, so
>> that only needs to change once.
>>
>> Unfortunately the linux and fbsd layers for 32-bit arm work in slightly
>> different ways. Ideally they would share more code and we'd unify some
>> register handling. But we're not there yet.
> 
> So I'm not quite sure how to parse your reply in terms of would you rather
> me make the TLS register numbers dynamic on both arm and aarch64 with a
> field in the tdep structure holding the number, or fixed values as the V2
> patch series currently does?  Also, a question I have in the V2 series is
> if NT_ARM_TLS is old enough to be present on all supported Linux aarch64
> kernels or if it needs some runtime checks to decide if the register is
> present or not for a Linux kernel.
> 
I'd rather have the dynamic register numbering in the generic files for 
now. In the future I'd like us to converge on a unified form of handling 
these as to avoid having the linux and fbsd files using different 
mechanisms for no good reason.

As for NT_ARM_TLS, it's been in the Linux kernel since 2012:

commit 478fcb2cdb2351dcfc3fb23f42d76f4436ee4149
Author: Will Deacon <will.deacon@arm.com>
Date:   Mon Mar 5 11:49:33 2012 +0000