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

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

On 4/2/22 00:30, John Baldwin wrote:
> 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.

My idea is to have a shared struct between gdb and gdbserver. Then do 
feature checks based on that. I did do some of that for gdbserver, but 
it didn't make its way to gdb yet unfortunately.

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

Probably copy/pasting. I think it should be fine the way it is then, for 
the sake of keeping it consistent.

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

Right. Is your intent to make tpidr a mandatory register for FreeBSD 
going forward? So if it is missing, should it be considered an error?

For Linux it shouldn't make the feature selection fail, for example.

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

That's true.

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

I have to teach binutils about this particular register set 
(NT_ARM_TLS). I see readelf currently doesn't recognize the proper name 
in a core dump. I'll send a patch soon.

>>>    /* 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).

Right. The idea of recording things in the tdep struct is to better 
organize the register sets in the presence of optional features. You 
might have some of the features, but not others, and we can't leave a 
hole in the register numbering.

At the moment there is a mix of macros and structs. My plan is to 
eventually convert everything to a single mechanism. A regcache_map 
might work nicely.

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

Ah, I see.