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

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

Simon Marchi <simark@simark.ca> writes:

>> 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.

I like optional because it forces the caller to check that the result is
valid before using it, whereas with an unwrapped pointer it's easy to
accidentally do a null pointer dereference.

I hadn't considered the situation of an optional wrapping a nullptr
though. Interesting point. For v3 I changed the function to return an
unwrapped pointer and check for nullptr in the caller.

>> @@ -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?

Thank you Luis for clarifying this point.

>> @@ -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".

That's true. I renamed the method to “get_thread_tdesc”. Thanks for the
suggestion.

>> @@ -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?

Indeed. regcache_release simply calls the static function
“free_register_cache_thread” on all threads, so for v3 I made it public
and called it just for this thread.

-- 
Thiago