Re: [PATCH v2 4/6] gdbserver/linux-aarch64: When thread stops, update its target description

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

On 11/28/22 15:47, Simon Marchi wrote:
> 
> 
>> diff --git a/gdbserver/linux-aarch64-low.cc b/gdbserver/linux-aarch64-low.cc
>> index cab4fc0a4674..786ce4071279 100644
>> --- a/gdbserver/linux-aarch64-low.cc
>> +++ b/gdbserver/linux-aarch64-low.cc
>> @@ -99,6 +99,9 @@ protected:
>>   
>>     void low_arch_setup () override;
>>   
>> +  gdb::optional<const struct target_desc *>
>> +    arch_update_tdesc (const thread_info *thread) override;
> 
> I'm all for using optional to be clear and explicit in general, but
> unless an empty optional and an optional wrapping nullptr are both valid
> and have different meanings (which doesn't seem, to be the case here), I
> wouldn't recommend wrapping a pointer in an optional.
> 
> Pointers have a dedicated value for "no value", that is well understood
> by everyone.  And then, it does create the possibility of returning an
> optional wrapping nullptr, which typically won't be a legitimate value.
> So it's just one more thing to worry about.
> 
>> @@ -840,8 +845,16 @@ aarch64_target::low_arch_setup ()
>>       {
>>         struct aarch64_features features
>>   	  = aarch64_get_arch_features (current_thread);
>> +      const target_desc *tdesc = aarch64_linux_read_description (features);
>>   
>> -      current_process ()->tdesc = aarch64_linux_read_description (features);
>> +      /* Only SVE-enabled inferiors need per-thread target descriptions.  */
>> +      if (features.vq > 0)
>> +	{
>> +	  current_thread->tdesc = tdesc;
>> +	  current_process ()->priv->arch_private->has_sve = true;
>> +	}
> 
> Is it not possible for a process to start with SVE disabled (vq == 0) and
> have some threads enable it later?

No. When supported, SVE is never disabled. It will always have a valid VL value, which is > 0 and restricted to a certain set of values depending on the hardware.

What does happen is the SVE state starts up as inactive, so we have neon vector data instead. But the SVE registers will always be there and the VL will always be valid.

> 
>> @@ -853,6 +866,28 @@ aarch64_target::low_arch_setup ()
>>     aarch64_linux_get_debug_reg_capacity (lwpid_of (current_thread));
>>   }
>>   
>> +/* Implementation of linux target ops method "arch_update_tdesc".  */
>> +
>> +gdb::optional<const struct target_desc *>
>> +aarch64_target::arch_update_tdesc (const thread_info *thread)
>> +{
>> +  /* Only processes using SVE need to update the thread's target description.  */
>> +  if (!get_thread_process (thread)->priv->arch_private->has_sve)
>> +    return {};
>> +
>> +  const struct aarch64_features features = aarch64_get_arch_features (thread);
>> +  const struct target_desc *tdesc = aarch64_linux_read_description (features);
>> +
>> +  if (tdesc == thread->tdesc)
>> +    return {};
>> +
>> +  /* Adjust the register sets we should use for this particular set of
>> +     features.  */
>> +  aarch64_adjust_register_sets(features);
>> +
>> +  return tdesc;
> 
> Naming nit: I don't think we need "update" in the method name, there's
> no "updating component" to it AFAICT.  It's basically "get this thread's
> tdesc if for some reason it differents from the process-wide we have".
> So, I don't know, "get_thread_tdesc" or just "thread_tdesc".
> 
>> @@ -2348,6 +2354,18 @@ linux_process_target::filter_event (int lwpid, int wstat)
>>   	      return;
>>   	    }
>>   	}
>> +      else
>> +	{
>> +	  /* Give the arch code an opportunity to update the thread's target
>> +	     description.  */
>> +	  gdb::optional<const struct target_desc *> new_tdesc
>> +	      = arch_update_tdesc (thread);
>> +	  if (new_tdesc.has_value ())
>> +	    {
>> +	      regcache_release ();
> 
> Hmm, regcache_release frees the regcache for all threads.  Can we free
> the regcache only for this thread?
> 
> Simon