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

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

On 4/4/22 09:06, Luis Machado wrote:
> 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.
> 

Sorry, scratch that. The set that isn't handled is the system call number.