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

[John Baldwin <jhb@FreeBSD.org>]

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).

-- 
John Baldwin