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

[Luis Machado <luis.machado@arm.com>]

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.