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

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

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>