Re: [PATCH 08/12] Add an aarch64-tls feature which includes the tpidr register.

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

Hi John,

On 3/23/22 21:00, John Baldwin wrote:
> ---
>   gdb/aarch64-linux-nat.c          |  3 ++-
>   gdb/aarch64-linux-tdep.c         |  2 +-
>   gdb/aarch64-tdep.c               | 42 ++++++++++++++++++++++++++------
>   gdb/aarch64-tdep.h               | 10 +++++++-
>   gdb/arch/aarch64.c               |  7 +++++-
>   gdb/arch/aarch64.h               |  9 +++++--
>   gdb/features/Makefile            |  1 +
>   gdb/features/aarch64-tls.c       | 14 +++++++++++
>   gdb/features/aarch64-tls.xml     | 12 +++++++++
>   gdbserver/linux-aarch64-tdesc.cc |  2 +-
>   gdbserver/netbsd-aarch64-low.cc  |  2 +-
>   11 files changed, 88 insertions(+), 16 deletions(-)
>   create mode 100644 gdb/features/aarch64-tls.c
>   create mode 100644 gdb/features/aarch64-tls.xml
> 
> diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
> index 7bb82d17cc8..4da274c285a 100644
> --- a/gdb/aarch64-linux-nat.c
> +++ b/gdb/aarch64-linux-nat.c
> @@ -646,7 +646,8 @@ aarch64_linux_nat_target::read_description ()
>     bool pauth_p = hwcap & AARCH64_HWCAP_PACA;
>     bool mte_p = hwcap2 & HWCAP2_MTE;
>   
> -  return aarch64_read_description (aarch64_sve_get_vq (tid), pauth_p, mte_p);
> +  return aarch64_read_description (aarch64_sve_get_vq (tid), pauth_p, mte_p,
> +				   false);

This is getting a bit ugly. We should pass a single struct that contains 
all available features eventually. But it should be OK right now.

>   }
>   
>   /* Convert a native/host siginfo object, into/from the siginfo in the
> diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
> index cb132d5a540..f5aac7bc0b4 100644
> --- a/gdb/aarch64-linux-tdep.c
> +++ b/gdb/aarch64-linux-tdep.c
> @@ -763,7 +763,7 @@ aarch64_linux_core_read_description (struct gdbarch *gdbarch,
>     bool pauth_p = hwcap & AARCH64_HWCAP_PACA;
>     bool mte_p = hwcap2 & HWCAP2_MTE;
>     return aarch64_read_description (aarch64_linux_core_read_vq (gdbarch, abfd),
> -				   pauth_p, mte_p);
> +				   pauth_p, mte_p, false);
>   }
>   
>   /* Implementation of `gdbarch_stap_is_single_operand', as defined in
> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
> index b714f6194b6..38c5c9e0e00 100644
> --- a/gdb/aarch64-tdep.c
> +++ b/gdb/aarch64-tdep.c
> @@ -58,7 +58,7 @@
>   #define HA_MAX_NUM_FLDS		4
>   
>   /* All possible aarch64 target descriptors.  */
> -static target_desc *tdesc_aarch64_list[AARCH64_MAX_SVE_VQ + 1][2/*pauth*/][2 /* mte */];
> +static target_desc *tdesc_aarch64_list[AARCH64_MAX_SVE_VQ + 1][2/*pauth*/][2 /* mte */][2 /* tls */];
>   
>   /* The standard register names, and all the valid aliases for them.  */
>   static const struct
> @@ -178,6 +178,12 @@ static const char *const aarch64_mte_register_names[] =
>     "tag_ctl"
>   };
>   
> +static const char *const aarch64_tls_register_names[] =
> +{
> +  /* Thread/Process ID Register.  */
> +  "tpidr"
> +};
> +
>   /* AArch64 prologue cache structure.  */
>   struct aarch64_prologue_cache
>   {
> @@ -3327,21 +3333,23 @@ aarch64_displaced_step_hw_singlestep (struct gdbarch *gdbarch)
>      If VQ is zero then it is assumed SVE is not supported.
>      (It is not possible to set VQ to zero on an SVE system).
>   
> -   MTE_P indicates the presence of the Memory Tagging Extension feature. */
> +   MTE_P indicates the presence of the Memory Tagging Extension feature.
> +
> +   TLS_P indicates the presence of the Thread Local Storage feature.  */
>   
>   const target_desc *
> -aarch64_read_description (uint64_t vq, bool pauth_p, bool mte_p)
> +aarch64_read_description (uint64_t vq, bool pauth_p, bool mte_p, bool tls_p)
>   {
>     if (vq > AARCH64_MAX_SVE_VQ)
>       error (_("VQ is %" PRIu64 ", maximum supported value is %d"), vq,
>   	   AARCH64_MAX_SVE_VQ);
>   
> -  struct target_desc *tdesc = tdesc_aarch64_list[vq][pauth_p][mte_p];
> +  struct target_desc *tdesc = tdesc_aarch64_list[vq][pauth_p][mte_p][tls_p];
>   
>     if (tdesc == NULL)
>       {
> -      tdesc = aarch64_create_target_description (vq, pauth_p, mte_p);
> -      tdesc_aarch64_list[vq][pauth_p][mte_p] = tdesc;
> +      tdesc = aarch64_create_target_description (vq, pauth_p, mte_p, tls_p);
> +      tdesc_aarch64_list[vq][pauth_p][mte_p][tls_p] = tdesc;
>       }
>   
>     return tdesc;
> @@ -3430,7 +3438,7 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>     bool valid_p = true;
>     int i, num_regs = 0, num_pseudo_regs = 0;
>     int first_pauth_regnum = -1, pauth_ra_state_offset = -1;
> -  int first_mte_regnum = -1;
> +  int first_mte_regnum = -1, first_tls_regnum = -1;
>   
>     /* Use the vector length passed via the target info.  Here -1 is used for no
>        SVE, and 0 is unset.  If unset then use the vector length from the existing
> @@ -3462,7 +3470,7 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>        value.  */
>     const struct target_desc *tdesc = info.target_desc;
>     if (!tdesc_has_registers (tdesc) || vq != aarch64_get_tdesc_vq (tdesc))
> -    tdesc = aarch64_read_description (vq, false, false);
> +    tdesc = aarch64_read_description (vq, false, false, false);
>     gdb_assert (tdesc);
>   
>     feature_core = tdesc_find_feature (tdesc,"org.gnu.gdb.aarch64.core");
> @@ -3471,6 +3479,8 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>     feature_pauth = tdesc_find_feature (tdesc, "org.gnu.gdb.aarch64.pauth");
>     const struct tdesc_feature *feature_mte
>       = tdesc_find_feature (tdesc, "org.gnu.gdb.aarch64.mte");
> +  const struct tdesc_feature *feature_tls
> +    = tdesc_find_feature (tdesc, "org.gnu.gdb.aarch64.tls");
>   
>     if (feature_core == nullptr)
>       return nullptr;
> @@ -3555,6 +3565,21 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>         num_regs += i;
>       }
>   
> +  /* Add the TLS register.  */
> +  if (feature_tls != nullptr)
> +    {
> +      first_tls_regnum = num_regs;
> +
> +      /* Validate the descriptor provides the mandatory MTE registers and
> +	 allocate their numbers.  */

It should say TLS instead of MTE. And also adjust it to mention it is a 
single register (at least for now?).

> +      for (i = 0; i < ARRAY_SIZE (aarch64_tls_register_names); i++)
> +	valid_p &= tdesc_numbered_register (feature_tls, tdesc_data.get (),
> +					    first_tls_regnum + i,
> +					    aarch64_tls_register_names[i]);
> +
> +      num_regs += i;
> +    }
> +
>     if (!valid_p)
>       return nullptr;

We should probably error/warn loudly about the fact this feature lacks a 
mandatory register.

>   
> @@ -3573,6 +3598,7 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>     tdep->pauth_ra_state_regnum = (feature_pauth == NULL) ? -1
>   				: pauth_ra_state_offset + num_regs;
>     tdep->mte_reg_base = first_mte_regnum;
> +  tdep->tls_reg_base = first_tls_regnum;
>   
>     set_gdbarch_push_dummy_call (gdbarch, aarch64_push_dummy_call);
>     set_gdbarch_frame_align (gdbarch, aarch64_frame_align);
> diff --git a/gdb/aarch64-tdep.h b/gdb/aarch64-tdep.h
> index 60a9d5a29c2..092586d4ee3 100644
> --- a/gdb/aarch64-tdep.h
> +++ b/gdb/aarch64-tdep.h
> @@ -111,10 +111,18 @@ struct aarch64_gdbarch_tdep : gdbarch_tdep
>     {
>       return mte_reg_base != -1;
>     }
> +
> +  /* TLS register.  This is -1 if the TLS register is not available.  */
> +  int tls_reg_base = 0;
> +
> +  bool has_tls() const
> +  {
> +    return tls_reg_base != -1;
> +  }
>   };
>   
>   const target_desc *aarch64_read_description (uint64_t vq, bool pauth_p,
> -					     bool mte_p);
> +					     bool mte_p, bool tls_p);
>   
>   extern int aarch64_process_record (struct gdbarch *gdbarch,
>   			       struct regcache *regcache, CORE_ADDR addr);
> diff --git a/gdb/arch/aarch64.c b/gdb/arch/aarch64.c
> index 485d667ccde..733a3fd6d2a 100644
> --- a/gdb/arch/aarch64.c
> +++ b/gdb/arch/aarch64.c
> @@ -24,11 +24,13 @@
>   #include "../features/aarch64-sve.c"
>   #include "../features/aarch64-pauth.c"
>   #include "../features/aarch64-mte.c"
> +#include "../features/aarch64-tls.c"
>   
>   /* See arch/aarch64.h.  */
>   
>   target_desc *
> -aarch64_create_target_description (uint64_t vq, bool pauth_p, bool mte_p)
> +aarch64_create_target_description (uint64_t vq, bool pauth_p, bool mte_p,
> +				   bool tls_p)
>   {
>     target_desc_up tdesc = allocate_target_description ();
>   
> @@ -52,5 +54,8 @@ aarch64_create_target_description (uint64_t vq, bool pauth_p, bool mte_p)
>     if (mte_p)
>       regnum = create_feature_aarch64_mte (tdesc.get (), regnum);
>   
> +  if (tls_p)
> +    regnum = create_feature_aarch64_tls (tdesc.get (), regnum);
> +
>     return tdesc.release ();
>   }
> diff --git a/gdb/arch/aarch64.h b/gdb/arch/aarch64.h
> index e416e346e9a..79821666ce6 100644
> --- a/gdb/arch/aarch64.h
> +++ b/gdb/arch/aarch64.h
> @@ -29,6 +29,7 @@ struct aarch64_features
>     bool sve = false;
>     bool pauth = false;
>     bool mte = false;
> +  bool tls = false;
>   };

The tls field doesn't seem to be set/used anywhere. I think it should be 
set when we find the tls feature, and used during gdbserver linux/bsd 
target description selection.

Also, the patch adjusts the gdb/aarch64-linux-nat.c file, but doesn't 
touch similar code in gdbserver's gdbserver/linux-aarch64-low.cc. We 
also support NT_ARM_TLS there, but we don't export this particular 
register through a feature.

I think it makes sense to do so, though I understand that is outside the 
boundary of bsd work. Let me know if you'd like me to enable that and 
test before the patch goes in.

>   
>   /* Create the aarch64 target description.  A non zero VQ value indicates both
> @@ -36,10 +37,12 @@ struct aarch64_features
>      an SVE Z register.  HAS_PAUTH_P indicates the presence of the PAUTH
>      feature.
>   
> -   MTE_P indicates the presence of the Memory Tagging Extension feature.  */
> +   MTE_P indicates the presence of the Memory Tagging Extension feature.
> +
> +   TLS_P indicates the presence of the Thread Local Storage feature.  */
>   
>   target_desc *aarch64_create_target_description (uint64_t vq, bool has_pauth_p,
> -						bool mte_p);
> +						bool mte_p, bool tls_p);
>   
>   /* Register numbers of various important registers.
>      Note that on SVE, the Z registers reuse the V register numbers and the V
> @@ -84,6 +87,8 @@ enum aarch64_regnum
>   #define AARCH64_PAUTH_CMASK_REGNUM(pauth_reg_base) (pauth_reg_base + 1)
>   #define AARCH64_PAUTH_REGS_SIZE (16)
>   
> +#define	AARCH64_TPIDR_REGNUM(tls_reg_base) (tls_reg_base)
> +

The purpose of similar macros is to index a register given a base 
number. Given the TLS set is just a single register, I don't think this 
macro should exist. We should use the value recorded in the tdep struct 
instead.

Also, I couldn't find any uses of AARCH64_TPIDR_REGNUM. Maybe I missed a 
patch?

>   #define AARCH64_X_REGS_NUM 31
>   #define AARCH64_V_REGS_NUM 32
>   #define AARCH64_SVE_Z_REGS_NUM AARCH64_V_REGS_NUM
> diff --git a/gdb/features/Makefile b/gdb/features/Makefile
> index 4b09819389a..946ec983df5 100644
> --- a/gdb/features/Makefile
> +++ b/gdb/features/Makefile
> @@ -198,6 +198,7 @@ FEATURE_XMLFILES = aarch64-core.xml \
>   	aarch64-fpu.xml \
>   	aarch64-pauth.xml \
>   	aarch64-mte.xml \
> +	aarch64-tls.xml \
>   	arc/v1-core.xml \
>   	arc/v1-aux.xml \
>   	arc/v2-core.xml \
> diff --git a/gdb/features/aarch64-tls.c b/gdb/features/aarch64-tls.c
> new file mode 100644
> index 00000000000..30d730dffba
> --- /dev/null
> +++ b/gdb/features/aarch64-tls.c
> @@ -0,0 +1,14 @@
> +/* THIS FILE IS GENERATED.  -*- buffer-read-only: t -*- vi:set ro:
> +  Original: aarch64-tls.xml */
> +
> +#include "gdbsupport/tdesc.h"
> +
> +static int
> +create_feature_aarch64_tls (struct target_desc *result, long regnum)
> +{
> +  struct tdesc_feature *feature;
> +
> +  feature = tdesc_create_feature (result, "org.gnu.gdb.aarch64.tls");
> +  tdesc_create_reg (feature, "tpidr", regnum++, 1, NULL, 64, "data_ptr");
> +  return regnum;
> +}
> diff --git a/gdb/features/aarch64-tls.xml b/gdb/features/aarch64-tls.xml
> new file mode 100644
> index 00000000000..48e0e9a6030
> --- /dev/null
> +++ b/gdb/features/aarch64-tls.xml
> @@ -0,0 +1,12 @@
> +<?xml version="1.0"?>
> +<!-- Copyright (C) 2009-2022 Free Software Foundation, Inc.
> +     Contributed by ARM Ltd.

I think the above Copyright should say 2022 and you should drop the 
"Contributed by ARM".

> +
> +     Copying and distribution of this file, with or without modification,
> +     are permitted in any medium without royalty provided the copyright
> +     notice and this notice are preserved.  -->
> +
> +<!DOCTYPE feature SYSTEM "gdb-target.dtd">
> +<feature name="org.gnu.gdb.aarch64.tls">
> +  <reg name="tpidr" bitsize="64" type="data_ptr"/>
> +</feature>
> diff --git a/gdbserver/linux-aarch64-tdesc.cc b/gdbserver/linux-aarch64-tdesc.cc
> index e982ab85067..14d6a4f80eb 100644
> --- a/gdbserver/linux-aarch64-tdesc.cc
> +++ b/gdbserver/linux-aarch64-tdesc.cc
> @@ -42,7 +42,7 @@ aarch64_linux_read_description (uint64_t vq, bool pauth_p, bool mte_p)
>   
>     if (tdesc == NULL)
>       {
> -      tdesc = aarch64_create_target_description (vq, pauth_p, mte_p);
> +      tdesc = aarch64_create_target_description (vq, pauth_p, mte_p, false);
>   
>         static const char *expedite_regs_aarch64[] = { "x29", "sp", "pc", NULL };
>         static const char *expedite_regs_aarch64_sve[] = { "x29", "sp", "pc",
> diff --git a/gdbserver/netbsd-aarch64-low.cc b/gdbserver/netbsd-aarch64-low.cc
> index 202bf1cdac6..b371e599232 100644
> --- a/gdbserver/netbsd-aarch64-low.cc
> +++ b/gdbserver/netbsd-aarch64-low.cc
> @@ -96,7 +96,7 @@ void
>   netbsd_aarch64_target::low_arch_setup ()
>   {
>     target_desc *tdesc
> -    = aarch64_create_target_description (0, false);
> +    = aarch64_create_target_description (0, false, false, false);
>   
>     static const char *expedite_regs_aarch64[] = { "x29", "sp", "pc", NULL };
>     init_target_desc (tdesc, expedite_regs_aarch64);