Re: [PATCH v2 6/6] gdb/aarch64: Detect vector length changes when debugging remotely

[Thiago Jung Bauermann <thiago.bauermann@linaro.org>]

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

> On 11/26/22 02:04, Thiago Jung Bauermann wrote:
>> If the remote target provides an expedited VG register, use it to update
>> the thread's gdbarch. This allows debugging programs that change their SVE
>> vector length during runtime.
>
> debugging programs -> debugging programs remotely

Indeed. Fixed.

>> This is accomplished by implementing the thread_architecture method in
>> remote_target, which returns the gdbarch corresponding to the expedited
>> registers provided by the last stop reply.
>> To allow changing the architecture based on the expedited registers, add a
>> new gdbarch method to allow arch-specific code to make the adjustment.

<snip>

>> diff --git a/gdb/gdbarch-components.py b/gdb/gdbarch-components.py
>> index 9b688998a7bd..fbb32c5e5853 100644
>> --- a/gdb/gdbarch-components.py
>> +++ b/gdb/gdbarch-components.py
>> @@ -2698,3 +2698,19 @@ Read core file mappings
>>       predefault="default_read_core_file_mappings",
>>       invalid=False,
>>   )
>> +
>> +Method(
>> +    comment="""
>> +An architecture may change based on the current contents of its registers.
>> +For instance, the length of the vector registers in AArch64's Scalable Vector
>> +Extension is given by the contents of the VL pseudo-register.
>
> VL -> VG

Ok, changed.

>> +This method allows an architecture to provide a new gdbarch corresponding to
>> +the registers in the given regcache.
>> +""",
>> +    type="struct gdbarch *",
>> +    name="update_architecture",
>> +    params=[("const std::vector<cached_reg_t> &", "regcache")],
>> +    predefault="default_update_architecture",
>> +    invalid=False,
>> +)

<snip>

>>   @@ -14382,6 +14407,23 @@ remote_target::thread_info_to_thread_handle (struct
>> thread_info *tp)
>>     return priv->thread_handle;
>>   }
>>   +struct gdbarch *
>> +remote_target::thread_architecture (ptid_t ptid)
>> +{
>> +  thread_info *thr = find_thread_ptid (this, ptid);
>> +  remote_thread_info *remote_thr = nullptr;
>> +
>> +  if (thr != nullptr)
>> +    remote_thr = get_remote_thread_info (thr);
>> +
>> +  if (remote_thr == nullptr || remote_thr->expedited_arch == nullptr)
>> +    /* The default thread_architecture implementation is the one from
>> +       process_stratum_target.  */
>> +    return process_stratum_target::thread_architecture(ptid);
>> +
>> +  return remote_thr->expedited_arch;
>> +}
>> +
>>   bool
>>   remote_target::can_async_p ()
>>   {
>
> Just to confirm, as I don't exactly remember how this went. Is the case where we switch
> threads during remote debugging covered in case we have threads with different vector
> lengths?
>
> I seem to recall we'd call thread_architecture in those cases, but I don't know how the
> expedited register comes into play for this case. Do we get expedited registers for each
> thread that has stopped?

I thought this was fine because my limited understanding of the remote
protocol was that whenever a thread stops, a T stop reply is sent with
the expedited registers.

However, when I tried testing this scenario it didn't work: I get a
“Truncated register 51 in remote 'g' packet” error when the main thread
hits a breakpoint and I try to switch to another thread with a different
vector length. It turns out that gdbserver only sends the T stop reply
for the thread that hit the breakpoint.

Thanks for spotting this problem. I will add a testcase to exercise this
scenario and see if I can come up with a fix.

> Otherwise LGTM.
>
> Reviewed-by: Luis Machado <luis.machado@arm.com>

Thanks!

-- 
Thiago