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>]

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