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

[John Baldwin <jhb@FreeBSD.org>]

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.

-- 
John Baldwin