Re: [PATCH] [AArch64] Add TPIDR2 register support for Linux

[Simon Marchi <simark@simark.ca>]

> diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
> index eda79ec6d35..03eec37a6c4 100644
> --- a/gdb/aarch64-linux-nat.c
> +++ b/gdb/aarch64-linux-nat.c
> @@ -442,19 +442,19 @@ fetch_tlsregs_from_thread (struct regcache *regcache)
>      = gdbarch_tdep<aarch64_gdbarch_tdep> (regcache->arch ());
>    int regno = tdep->tls_regnum;
>  
> -  gdb_assert (regno != -1);
> +  gdb_assert (regno != -1 && tdep->tls_register_count > 0);
Check just one thing per gdb_assert.

> @@ -786,7 +790,16 @@ aarch64_linux_core_read_description (struct gdbarch *gdbarch,
>    features.vq = aarch64_linux_core_read_vq (gdbarch, abfd);
>    features.pauth = hwcap & AARCH64_HWCAP_PACA;
>    features.mte = hwcap2 & HWCAP2_MTE;
> -  features.tls = tls != nullptr;
> +
> +  /* Handle the TLS section.  */
> +  asection *tls = bfd_get_section_by_name (abfd, ".reg-aarch-tls");
> +  if (tls != nullptr)
> +    {
> +      size_t size = bfd_section_size (tls);
> +      /* Convert the size to the number of actual registers, by
> +	 dividing by 8.  */
> +      features.tls = size >> 3;

Why not write it as `size / 8`?  Or, `size / AARCH64_TLS_REGISTER_SIZE`?

> @@ -3544,13 +3555,26 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>    /* Add the TLS register.  */
>    if (feature_tls != nullptr)
>      {
> -      tls_regnum = num_regs;
> -      /* Validate the descriptor provides the mandatory TLS register
> -	 and allocate its number.  */
> -      valid_p = tdesc_numbered_register (feature_tls, tdesc_data.get (),
> -					 tls_regnum, "tpidr");
> +      const char *tls_register_names[2] = { "tpidr", "tpidr2" };
> +      first_tls_regnum = num_regs;
> +
> +      /* Look for the TLS registers.  */
> +      for (i = 0; i < ARRAY_SIZE (tls_register_names); i++)
> +	{
> +	  valid_p
> +	    = tdesc_numbered_register (feature_tls, tdesc_data.get (),
> +				       first_tls_regnum + tls_register_count,
> +				       tls_register_names[i]);
> +	  if (valid_p)
> +	    tls_register_count++;
> +	}
> +      valid_p = true;

It would seem better to just use a different variable inside the loop,
if you don't want it to affect valid_p.  If valid_p is false before
entering the loop, you still set it to true afterwards, is that what you
want?

It seems to me like you would want the lookup of tpidr to affect
valid_p, as it would be invalid to have the tls feature without tpidr,
but the lookup of tpidr2 to not affect valid_p.

> diff --git a/gdb/aarch64-tdep.h b/gdb/aarch64-tdep.h
> index d8513023c37..8166df0ada8 100644
> --- a/gdb/aarch64-tdep.h
> +++ b/gdb/aarch64-tdep.h
> @@ -111,8 +111,9 @@ struct aarch64_gdbarch_tdep : gdbarch_tdep_base
>      return mte_reg_base != -1;
>    }
>  
> -  /* TLS register.  This is -1 if the TLS register is not available.  */
> +  /* TLS registers.  This is -1 if the TLS registers are not available.  */
>    int tls_regnum = 0;
> +  int tls_register_count = 0;

Perhaps rename tls_regnum to tls_regnum_base, since it not longer
represents just one register.

> diff --git a/gdb/nat/aarch64-linux.c b/gdb/nat/aarch64-linux.c
> index 421d1ecb53c..017c30948a9 100644
> --- a/gdb/nat/aarch64-linux.c
> +++ b/gdb/nat/aarch64-linux.c
> @@ -250,3 +250,20 @@ aarch64_ps_get_thread_area (struct ps_prochandle *ph,
>  
>    return PS_OK;
>  }
> +
> +/* See nat/aarch64-linux.h */

Dot and double space.

> +
> +int
> +aarch64_tls_register_count (int tid)
> +{
> +  uint64_t tls_regs[2];
> +  struct iovec iovec;
> +  iovec.iov_base = tls_regs;
> +  iovec.iov_len = sizeof (tls_regs);
> +
> +  if (ptrace (PTRACE_GETREGSET, tid, NT_ARM_TLS, &iovec) != 0)
> +    return 1;
> +
> +  /* TPIDR2 is available.  */
> +  return 2;

Can you explain what is happening here?  How does checking for success
reading NT_ARM_TLS telly ou that TPIDR2 exists?

Simon