Re: [PATCH 1/2] gdb/aarch64: Fix thread's gdbarch when SVE vector length changes

[Thiago Jung Bauermann via Gdb-patches <gdb-patches@sourceware.org>]

Hello Luis,

Luis Machado <luis.machado@arm.com> writes:

> Thanks for the patch.

Thank you for your prompt review!

> On 7/28/22 02:23, Thiago Jung Bauermann via Gdb-patches wrote:
>> --- a/gdb/aarch64-linux-nat.c
>> +++ b/gdb/aarch64-linux-nat.c
>> @@ -900,11 +900,16 @@ aarch64_linux_nat_target::thread_architecture (ptid_t ptid)
>>       /* We reach here if the vector length for the thread is different from its
>>        value at process start.  Lookup gdbarch via info (potentially creating a
>> -     new one), stashing the vector length inside id.  Use -1 for when SVE
>> -     unavailable, to distinguish from an unset value of 0.  */
>> +     new one) by using a target description that corresponds to the new vq value
>> +     and the current architecture features.  */
>> +
>> +  const struct target_desc *tdesc = gdbarch_target_desc (inf->gdbarch);
>> +  aarch64_features features = aarch64_features_from_target_desc (tdesc);
>> +  features.vq = vq;
>> +
>>     struct gdbarch_info info;
>>     info.bfd_arch_info = bfd_lookup_arch (bfd_arch_aarch64, bfd_mach_aarch64);
>> -  info.id = (int *) (vq == 0 ? -1 : vq);
>
> Even though we're removing this code, we're still actively using
> info.id in aarch64_gdbarch_init (). Will that work correctly?

Ah, well spotted. It will — the statements that check info.id become
dead code. Your comment made me notice that the line I remove above was
the only place in all of GDB that was setting info.id, and
aarch64_gdbarch_init () was the only place reading it.

So v2 will remove the id member of struct gdbarch_info altogether (and
thus the anonymous union it was part of) and simplify a bit the
vq-related code in aarch64_gdbarch_init ().

>> +  info.target_desc = aarch64_create_target_description (features);
>
> The current mechanism caches potentially multiple target description
> (to account for threads having different VL's). Instead of creating a
> fresh target description here,

Well spotted again. This was actually a thinko on my part. I thought
I was using the function that does the target description caching. In v2
this code will call aarch64_read_description () instead of
aarch64_create_target_description ().

> would it make more sense to figure out the features from this
> particular thread and then check if we have the right target
> description already?
>
> Maybe through aarch64_linux_nat_target::read_description ()?

This patch is figuring out the features from this particular thread by
looking at its current target description so I don't think it's
necessary to use aarch64_linux_nat_target::read_description (), unless
I'm misunderstanding your point.

I'll now work on your comments for the testcase patch and will respond
to it tomorrow.

-- 
Thiago