Re: [PATCH v3] [aarch64] Add TPIDR2 register support for Linux

[Simon Marchi <simon.marchi@efficios.com>]

On 12/6/22 08:16, Luis Machado wrote:
> Updates on v3:
> 
> - Addressed review comments. Variable renaming and more code comments.
> 
> Updates on v2:
> 
> - Added missing REGISTER_SIZE_VARIABLE to the tls regset, preventing a warning
> when GDB finds a section that doesn't match the regset size.

A few comments below, with those fixed:

Approved-By: Simon Marchi <simon.marchi@efficios.com>

> @@ -465,21 +466,23 @@ store_tlsregs_to_thread (struct regcache *regcache)
>  {
>    aarch64_gdbarch_tdep *tdep
>      = gdbarch_tdep<aarch64_gdbarch_tdep> (regcache->arch ());
> -  int regno = tdep->tls_regnum;
> +  int regno = tdep->tls_regnum_base;
>  
> -  gdb_assert (regno != -1);
> +  gdb_assert (regno != -1 && tdep->tls_register_count > 0);

Split this assert.

>  
> -  uint64_t tpidr = 0;
> +  uint64_t tpidrs[tdep->tls_register_count] = { 0 };
>  
> -  if (REG_VALID != regcache->get_register_status (regno))
> -    return;
> +  for (int i = 0; i < tdep->tls_register_count; i++)
> +    {
> +      if (REG_VALID != regcache->get_register_status (regno + i))
> +	continue;
>  
> -  regcache->raw_collect (regno, (char *) &tpidr);
> +      regcache->raw_collect (regno + i, (char *) &tpidrs[i]);
> +    }
>  
>    struct iovec iovec;
> -
> -  iovec.iov_base = &tpidr;
> -  iovec.iov_len = sizeof (tpidr);
> +  iovec.iov_base = &tpidrs;
> +  iovec.iov_len = sizeof (tpidrs);
>  
>    int tid = get_ptrace_pid (regcache->ptid ());
>    if (ptrace (PTRACE_SETREGSET, tid, NT_ARM_TLS, &iovec) != 0)
> @@ -531,7 +534,7 @@ aarch64_fetch_registers (struct regcache *regcache, int regno)
>        && (regno == tdep->mte_reg_base))
>      fetch_mteregs_from_thread (regcache);
>  
> -  if (tdep->has_tls () && regno == tdep->tls_regnum)
> +  if (tdep->has_tls () && regno == tdep->tls_regnum_base)
>      fetch_tlsregs_from_thread (regcache);

Should this condition be changed, like the one in
aarch64_store_registers?

> @@ -3614,15 +3624,40 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>      }
>  
>    /* Add the TLS register.  */
> +  int tls_register_count = 0;
>    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;
>  
> -      num_regs++;
> +      /* Look for the TLS registers.  tpidr is required, but tpidr2 is
> +	 optional.  */
> +      valid_p
> +	= tdesc_numbered_register (feature_tls, tdesc_data.get (),
> +				   first_tls_regnum,
> +				   tls_register_names[0]);
> +
> +      if (valid_p)
> +	{
> +	  tls_register_count++;
> +
> +	  bool has_tpidr2
> +	    = tdesc_numbered_register (feature_tls, tdesc_data.get (),
> +				       first_tls_regnum + tls_register_count,
> +				       tls_register_names[tls_register_count]);

Since each call is about a specific register, I would suggest getting
rid of tls_register_names and in-line the name, it will be clearer IMO.

Simon