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

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

On 12/1/22 03:16, Thiago Jung Bauermann wrote:
> 
> Simon Marchi <simark@simark.ca> writes:
> 
>> On 11/25/22 21:04, Thiago Jung Bauermann via Gdb-patches 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.
>>>
>>> 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.
>>
>> I get this when applying, can you address that?
>>
>> Applying: gdb/aarch64: Detect vector length changes when debugging remotely
>> .git/rebase-apply/patch:164: indent with spaces.
>>                         "gdbarch_dump: update_architecture = <%s>\n",
>> .git/rebase-apply/patch:165: indent with spaces.
>>                         host_address_to_string (gdbarch->update_architecture));
>> .git/rebase-apply/patch:186: indent with spaces.
>>                                    gdbarch_update_architecture_ftype update_architecture)
>> warning: 3 lines add whitespace errors.
> 
> This is because gdbarch.py generates these lines in gdbarch.c with
> spaces-only indentation. I will send a separate patch to fix it.
> 
>> I think the idea makes sense.  Short of adding a packet for "get thread
>> architecture" or extending the stop reply format to add a note about a
>> thread having a special tdesc, I don't see another way to do this.  We
> 
> I guess we could define such a packet, but this approach is indeed
> simpler.
> 

Just a FYI, we now have a feature hash for determining the target features for aarch64. It is a 64-bit
number and doesn't rely on positioning of registers.

This situation with vg/SVE will also apply to svg/SME in the future.

>> can't read the vq register using 'g', because to interpret the 'g'
>> result, we need to know the full architecture.  We wouldn't know for
>> sure the offset of vq in the reply.  By using expedited registers, we
>> just need to make sure vq's register number is stable.  I guess that is
>> the case?
> 
> The register number is determined by the function
> aarch64_create_target_description. If SVE is supported, VG will always
> have the register number 85. If not, then AFAIU the register number will
> be unused (since the PAuth, MTE and TLS features only have a few
> registers). But it could potentially be used by another feature in the
> future...
> 
> Do we have to reserve the number 85 for the VG pseudo-register?
> 
>>> @@ -8068,6 +8078,21 @@ remote_target::process_stop_reply (struct stop_reply *stop_reply,
>>>         /* Expedited registers.  */
>>>         if (!stop_reply->regcache.empty ())
>>>   	{
>>> +	  /* If GDB already knows about this thread, we can give the
>>> +	     architecture-specific code a chance to update the gdbarch based on
>>> +	     the expedited registers.  */
>>> +	  if (find_thread_ptid (this, ptid) != nullptr)
>>> +	    {
>>> +	      stop_reply->arch = gdbarch_update_architecture (stop_reply->arch,
>>> +							      stop_reply->regcache);
>>> +
>>> +	      /* Save stop_reply->arch so that it can be returned by the
>>> +		 thread_architecture method.  */
>>> +	      remote_thread_info *remote_thr = get_remote_thread_info (this,
>>> +								       ptid);
>>> +	      remote_thr->expedited_arch = stop_reply->arch;
>>> +	    }
>>> +
>>>   	  struct regcache *regcache
>>>   	    = get_thread_arch_regcache (this, ptid, stop_reply->arch);
>>
>>
>> What happens with threads that are not yet known?  Imagine the process
>> starts with SVE == X, the main threads spawns a worker thread, the
>> worker thread updates its SVE to Y, and the worker thread hits a
>> breakpoint.  This is the first time we hear about that worker thread.
> 
> I tested this scenario and... it sort of works, but not quite. When the
> unknown thread hits the breakpoint, gdbserver sends a T stop reply
> packet with the new VG value in the expedited registers. It will be the
> first time we hear about that worker thread, but at the same time we
> will also learn about the value of its VG pseudo-register. So GDB
> reports the breakpoint hit event and gets to the prompt as expected. You
> can even print the new value of the VG pseudo-register, or any other
> expedited register such as PC or SP.
> 
> It's only when you try to print the value of a non-expedited register
> that you get an error indicating that GDB and gdbserver don't agree on
> the size of the SVE registers (“Truncated register 51 in remote 'g'
> packet”).
> 
>> If we don't do a "gdbarch_update_architecture" for it and save it
>> somewhere, we'll never set the gdbarch with SVE == Y, will we?
> 
> We do that in remote_target::process_stop_reply, and I was under the
> impression that it happened early enough for GDB to have the correct
> gdbarch available when it needs it, but unfortunately that's not the
> case.
> 
> I will add a testcase exercising this issue and see if I can find a
> solution. Thank you for spotting it.
>