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

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

On 12/5/22 22:37, Thiago Jung Bauermann wrote:
> 
> Luis Machado <luis.machado@arm.com> writes:
> 
>> On 11/26/22 02:04, Thiago Jung Bauermann wrote:
>>> @@ -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);
>>>    @@ -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 recalled this. One deficiency of the current SVE implementation
>> is that it doesn't have a proper way to hide the Z register when they
>> don't have any meaningful state (SVE not active, so we have fpsimd
>> state).
> 
> I see the SVE_HEADER_FLAG_SVE being checked/set in
> aarch64_linux_supply_sve_regset and aarch64_linux_collect_sve_regset
> (though in the latter it is set unconditionally), and also HAS_SVE_STATE
> being used in aarch64_sve_regs_copy_to_reg_buf and
> aarch64_sve_regs_copy_from_reg_buf.
> 
> But I wasn't able to find similar logic regarding hiding the Z
> registers. Do you have any pointers?
> 

There isn't logic to do that at present. This is something I'm pondering for SME, and would possibly apply to
SVE as well. But it may be more complicated than useful.

>> Given the VL will always be reported as > 0, even if there is no
>> active SVE state, gdb will always attempt to show Z registers. And
>> those are quite verbose.
> 
> The VG pseudo-register is defined with an int type, so we could return a
> negative value to indicate that the Z registers are unused (and the
> module of the value would still be the vector granule). Would that
> be ok?

I think a value of 0 would be fine for this purpose but...

> 
>> In this sense, the implementations of the the thread_architecture
>> methods for both native aarch64 and remote differ. While native
>> aarch64's implementation is capable of detecting the lack of active
>> SVE state, the remote implementation can't. If gdbserver decided to
>> drop the Z registers mid-execution (before the SVE state is not
>> active), there wouldn't be vg for gdb to make a decision.
> 
> Looking at aarch64_linux_nat_target::thread_architecture my impression
> is that its result depends only on aarch64_sve_get_vq, so I don't see
> how it's different from the remote implementation.

... Right, but for native debugging we actually call ptrace to extract the vector length. That is completely independent from
the XML target description or the available register sets.

In the case of remote debugging, the expedited registers are returned based on an existing register description.

Take, for example, the following:

- SVE state is enabled and we have the vg register.
- Program updates the SVE vector length and, as a result, we no longer have an active SVE state (reverted to FPSIMD state)
- GDB drops the sve feature from its target description.
- Program executes a sve instruction and SVE state is made active.
- gdbserver tries to tell gdb what the value of vg is, but gdb doesn't know anything about vg at this point, because it has dropped the sve feature previously.

I think this is the downside of using expedited registers to communicate the architecture changes. For this case, returning a full XML would make it easy for gdb to
figure things out.

But, going back to hiding sve/sme registers when they're not in use, I'm not yet sure that is something we should pursue.

I'd say if you manage to get things working with the expedited registers (for the problematic cases Simon and I pointed out), that's fine.

> 
>> I wonder if there is a better way to handle this that would still
>> allow us to hide these scalable registers when there is no active
>> state.
> 
> I guess it depends on whether you would consider using negative VG
> values as better. :-)
>