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

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

Hi Simon,

Thanks for the review.

On 12/5/22 20:36, Simon Marchi wrote:
>> 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.

Fixed.

> 
>> @@ -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`?
> 

Fixed.

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

Yes, indeed. I've switched this to using a different bool for the tpidr2 check.

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

Yeah. I deferred renaming this to avoid having more churn in the patch.

Done now.

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

If you attempt a read of two registers - sizeof (tls_regs) - and it fails, it means the Linux Kernel doesn't
support TPIDR2. Otherwise, it does.

I'll add a comment making this a bit more clear.

> 
> Simon
> 
I'll send an updated version.