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

  This change allows aarch64-linux to support debugging programs where
different threads have different SVE vector lengths.  It requires
gdbserver to support different inferior threads having different target
descriptions.

The arch_update_tdesc method is added to the linux_process_target class to
allow aarch64-linux to probe the inferior's vq register and provide an
updated thread target description reflecting the new vector length.

After this change, all targets except SVE-supporting aarch64-linux will
still use per-process target descriptions.
---
 gdbserver/gdbthread.h          |  2 ++
 gdbserver/linux-aarch64-low.cc | 37 +++++++++++++++++++++++++++++++++-
 gdbserver/linux-low.cc         | 18 +++++++++++++++++
 gdbserver/linux-low.h          |  5 +++++
 gdbserver/regcache.cc          | 11 +++++++---
 gdbserver/tdesc.cc             |  3 +++
 6 files changed, 72 insertions(+), 4 deletions(-)
  

Thanks for the patch.

On 11/26/22 02:04, Thiago Jung Bauermann wrote:
> This change allows aarch64-linux to support debugging programs where
> different threads have different SVE vector lengths.  It requires
> gdbserver to support different inferior threads having different target
> descriptions.
> 
> The arch_update_tdesc method is added to the linux_process_target class to
> allow aarch64-linux to probe the inferior's vq register and provide an
> updated thread target description reflecting the new vector length.
> 
> After this change, all targets except SVE-supporting aarch64-linux will
> still use per-process target descriptions.
> ---
>   gdbserver/gdbthread.h          |  2 ++
>   gdbserver/linux-aarch64-low.cc | 37 +++++++++++++++++++++++++++++++++-
>   gdbserver/linux-low.cc         | 18 +++++++++++++++++
>   gdbserver/linux-low.h          |  5 +++++
>   gdbserver/regcache.cc          | 11 +++++++---
>   gdbserver/tdesc.cc             |  3 +++
>   6 files changed, 72 insertions(+), 4 deletions(-)
> 
> diff --git a/gdbserver/gdbthread.h b/gdbserver/gdbthread.h
> index 8b897e73d33b..47b44d03b8e0 100644
> --- a/gdbserver/gdbthread.h
> +++ b/gdbserver/gdbthread.h
> @@ -80,6 +80,8 @@ struct thread_info
>   
>     /* Branch trace target information for this thread.  */
>     struct btrace_target_info *btrace = nullptr;
> +
> +  const struct target_desc *tdesc = nullptr;

Should we add a bit of information on how this new field is used, through a comment?

>   };
>   
>   extern std::list<thread_info *> all_threads;
> 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;
> +
>     bool low_cannot_fetch_register (int regno) override;
>   
>     bool low_cannot_store_register (int regno) override;
> @@ -184,6 +187,8 @@ struct arch_process_info
>        same for each thread, it is reasonable for the data to live here.
>        */
>     struct aarch64_debug_reg_state debug_reg_state;
> +
> +  bool has_sve;
>   };

Though obvious, adding a comment like "has_sve" in gdb/aarch64-tdep.h will clarify the use of this field.

>   
>   /* Return true if the size of register 0 is 8 byte.  */
> @@ -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;
> +	}
> +
> +      current_process ()->tdesc = tdesc;
>   
>         /* Adjust the register sets we should use for this particular set of
>   	 features.  */
> @@ -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;
> +}
> +
>   /* Implementation of linux target ops method "get_regs_info".  */
>   
>   const regs_info *
> diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
> index fc496275d6a3..7c510e26f0f5 100644
> --- a/gdbserver/linux-low.cc
> +++ b/gdbserver/linux-low.cc
> @@ -483,6 +483,12 @@ linux_process_target::arch_setup_thread (thread_info *thread)
>     low_arch_setup ();
>   }
>   
> +gdb::optional<const struct target_desc *>
> +linux_process_target::arch_update_tdesc (const thread_info *thread)
> +{
> +  return {};
> +}
> +
>   int
>   linux_process_target::handle_extended_wait (lwp_info **orig_event_lwp,
>   					    int wstat)
> @@ -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 ();
> +	      thread->tdesc = *new_tdesc;
> +	    }
> +	}
>       }
>   
>     if (WIFSTOPPED (wstat) && child->must_set_ptrace_flags)
> diff --git a/gdbserver/linux-low.h b/gdbserver/linux-low.h
> index 182a540f3bb3..ff14423e9e07 100644
> --- a/gdbserver/linux-low.h
> +++ b/gdbserver/linux-low.h
> @@ -604,6 +604,11 @@ class linux_process_target : public process_stratum_target
>     /* Architecture-specific setup for the current thread.  */
>     virtual void low_arch_setup () = 0;
>   
> +  /* Allows arch-specific code to update the thread's target description when
> +     the inferior stops.  */

I'd also mention sometimes we don't need to update the target description if nothing's
changed. So an empty return is expected.

> +  virtual gdb::optional<const struct target_desc *>
> +    arch_update_tdesc (const thread_info *thread);
> +
>     /* Return false if we can fetch/store the register, true if we cannot
>        fetch/store the register.  */
>     virtual bool low_cannot_fetch_register (int regno) = 0;
> diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc
> index 14236069f712..03ee88b3cfd1 100644
> --- a/gdbserver/regcache.cc
> +++ b/gdbserver/regcache.cc
> @@ -39,11 +39,16 @@ get_thread_regcache (struct thread_info *thread, int fetch)
>        have.  */
>     if (regcache == NULL)
>       {
> -      struct process_info *proc = get_thread_process (thread);
> +      /* First see if there's a thread-specific target description.  */
> +      const target_desc *tdesc = thread->tdesc;
>   
> -      gdb_assert (proc->tdesc != NULL);
> +      /* If not, get it from the process instead.  */
> +      if (tdesc == nullptr)
> +	tdesc = get_thread_process (thread)->tdesc;

Just a suggestion. Your call.

We could abstract away trying to fetch a tdesc from a thread and then from a process by having a function "get_tdesc ()" and calling it here.

Possibly with a better name.

>   
> -      regcache = new_register_cache (proc->tdesc);
> +      gdb_assert (tdesc != nullptr);
> +
> +      regcache = new_register_cache (tdesc);
>         set_thread_regcache_data (thread, regcache);
>       }
>   
> diff --git a/gdbserver/tdesc.cc b/gdbserver/tdesc.cc
> index 5693cc6626fb..3665ab0540d5 100644
> --- a/gdbserver/tdesc.cc
> +++ b/gdbserver/tdesc.cc
> @@ -129,6 +129,9 @@ current_target_desc (void)
>     if (current_thread == NULL)
>       return &default_description;
>   
> +  if (current_thread->tdesc != nullptr)
> +    return current_thread->tdesc;
> +
>     return current_process ()->tdesc;

We'd use the above function in here as well.

>   }
>   

Otherwise LGTM.

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

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

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

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
  

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

Thanks for clarifying.

Simon
  

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

> Thanks for the patch.
>
> On 11/26/22 02:04, Thiago Jung Bauermann wrote:
>> This change allows aarch64-linux to support debugging programs where
>> different threads have different SVE vector lengths.  It requires
>> gdbserver to support different inferior threads having different target
>> descriptions.
>> The arch_update_tdesc method is added to the linux_process_target class to
>> allow aarch64-linux to probe the inferior's vq register and provide an
>> updated thread target description reflecting the new vector length.
>> After this change, all targets except SVE-supporting aarch64-linux will
>> still use per-process target descriptions.
>> ---
>>   gdbserver/gdbthread.h          |  2 ++
>>   gdbserver/linux-aarch64-low.cc | 37 +++++++++++++++++++++++++++++++++-
>>   gdbserver/linux-low.cc         | 18 +++++++++++++++++
>>   gdbserver/linux-low.h          |  5 +++++
>>   gdbserver/regcache.cc          | 11 +++++++---
>>   gdbserver/tdesc.cc             |  3 +++
>>   6 files changed, 72 insertions(+), 4 deletions(-)
>> diff --git a/gdbserver/gdbthread.h b/gdbserver/gdbthread.h
>> index 8b897e73d33b..47b44d03b8e0 100644
>> --- a/gdbserver/gdbthread.h
>> +++ b/gdbserver/gdbthread.h
>> @@ -80,6 +80,8 @@ struct thread_info
>>       /* Branch trace target information for this thread.  */
>>     struct btrace_target_info *btrace = nullptr;
>> +
>> +  const struct target_desc *tdesc = nullptr;
>
> Should we add a bit of information on how this new field is used, through a comment?

Good idea. For v3 I added:

  /* Target description for this thread.  Only present if it's different
     from the one in process_info.  */
  const struct target_desc *tdesc = nullptr;

>>   };
>>     extern std::list<thread_info *> all_threads;
>> 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;
>> +
>>     bool low_cannot_fetch_register (int regno) override;
>>       bool low_cannot_store_register (int regno) override;
>> @@ -184,6 +187,8 @@ struct arch_process_info
>>        same for each thread, it is reasonable for the data to live here.
>>        */
>>     struct aarch64_debug_reg_state debug_reg_state;
>> +
>> +  bool has_sve;
>>   };
>
> Though obvious, adding a comment like "has_sve" in gdb/aarch64-tdep.h will clarify the use
> of this field.

Indeed. For v3 I added:

  /* Whether this process has the Scalable Vector Extension available.  */
  bool has_sve;

>> diff --git a/gdbserver/linux-low.h b/gdbserver/linux-low.h
>> index 182a540f3bb3..ff14423e9e07 100644
>> --- a/gdbserver/linux-low.h
>> +++ b/gdbserver/linux-low.h
>> @@ -604,6 +604,11 @@ class linux_process_target : public process_stratum_target
>>     /* Architecture-specific setup for the current thread.  */
>>     virtual void low_arch_setup () = 0;
>>   +  /* Allows arch-specific code to update the thread's target description when
>> +     the inferior stops.  */
>
> I'd also mention sometimes we don't need to update the target description if nothing's
> changed. So an empty return is expected.

Good point. Including Simon's suggestions this is what I have for v3:

  /* Allows arch-specific code to set the thread's target description when the
     inferior stops.  Returns nullptr if no thread-specific target description
     is necessary.  */
  virtual const struct target_desc *
    get_thread_tdesc (const thread_info *thread);

>> +  virtual gdb::optional<const struct target_desc *>
>> +    arch_update_tdesc (const thread_info *thread);
>> +
>>     /* Return false if we can fetch/store the register, true if we cannot
>>        fetch/store the register.  */
>>     virtual bool low_cannot_fetch_register (int regno) = 0;
>> diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc
>> index 14236069f712..03ee88b3cfd1 100644
>> --- a/gdbserver/regcache.cc
>> +++ b/gdbserver/regcache.cc
>> @@ -39,11 +39,16 @@ get_thread_regcache (struct thread_info *thread, int fetch)
>>        have.  */
>>     if (regcache == NULL)
>>       {
>> -      struct process_info *proc = get_thread_process (thread);
>> +      /* First see if there's a thread-specific target description.  */
>> +      const target_desc *tdesc = thread->tdesc;
>>   -      gdb_assert (proc->tdesc != NULL);
>> +      /* If not, get it from the process instead.  */
>> +      if (tdesc == nullptr)
>> +	tdesc = get_thread_process (thread)->tdesc;
>
> Just a suggestion. Your call.
>
> We could abstract away trying to fetch a tdesc from a thread and then from a process by
> having a function "get_tdesc ()" and calling it here.
>
> Possibly with a better name.

I like the idea. I implemented it and named the function
“get_thread_target_desc”.

>
>>   -      regcache = new_register_cache (proc->tdesc);
>> +      gdb_assert (tdesc != nullptr);
>> +
>> +      regcache = new_register_cache (tdesc);
>>         set_thread_regcache_data (thread, regcache);
>>       }
>>   diff --git a/gdbserver/tdesc.cc b/gdbserver/tdesc.cc
>> index 5693cc6626fb..3665ab0540d5 100644
>> --- a/gdbserver/tdesc.cc
>> +++ b/gdbserver/tdesc.cc
>> @@ -129,6 +129,9 @@ current_target_desc (void)
>>     if (current_thread == NULL)
>>       return &default_description;
>>   +  if (current_thread->tdesc != nullptr)
>> +    return current_thread->tdesc;
>> +
>>     return current_process ()->tdesc;
>
> We'd use the above function in here as well.

I did that for v3. There's a small difference in the code with the new
version: instead of using current_process (), it ends up doing
“get_thread_process (current_thread)”. I expect it to be equivalent
though, and I'm not seeing any regression in the testsuite.

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

Thanks! Added the Reviewed-by tag to the patch description for v3.
  

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.