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

[John Baldwin <jhb@FreeBSD.org>]

On 3/28/22 3:16 AM, Luis Machado wrote:
>> -  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.

Mmm, a struct would be nice, yes.  Maybe I will do that as a followup change.

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

Oops, did miss a MTE rename.  Note that the MTE register set also contains a
single register but still uses the plural (but I will fix this).  If you think
it's clearer I could remove the aarch64_tls_register_names and just call
tdesc_numbered_register() once without the loop with the specific register name.

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

It requires the single "tpidr" register I think?
  
>> 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.

Mm, yes.  Arguably this structure should be the thing that replaces the
parameters passed to aarch64_read_description(), though I think it would
need the vq value and not just a bool for sve to serve that purpose.

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

So for this particular patch, I'm just trying to add the arch feature
itself.  The later patches in the series (some of which don't seem to
have made it to the mailing list actually, so I will resend), make use
of it in aarch64-fbsd-tdep.c and aarch64-fbsd-nat.c.  The changes to
aarch64-linux-nat.c in this patch is just to support the new argument
to aarch64_read_description().

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

I can probably take a stab at this based on the FreeBSD patches in the
series (they should be fairly similar, at least for the tdep.c file).
>>    
>>    /* 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.

Ok, I was just following what had been done for MTE.  I'm fine with just
making it a single value.  Alternatively, it might be nice to allocate a
fixed value for the register in enum aarch64_regnum.  I only picked a
variable base due to copying what was done for the PAUTH registers.  If
adding a new AARCH64_TPIDR_REGNUM after AARCH64_SVE_VG_REGNUM is ok, it
would be simpler (e.g. I could use a static regcache_map instead of having
to construct one dynamically).

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

It is used in one of the FreeBSD patches that didn't make it to the list
(but was included in the cover letter).

-- 
John Baldwin