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

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

To that end, a tdesc field is added to struct thread_info.  If it's
nullptr (the default) it means that the thread uses the target description
stored in struct process_info and thus gdbserver behaviour is unchanged.
The get_thread_tdesc method is added to the linux_process_target class to
allow aarch64-linux code to probe the inferior's vq register and provide a
thread-specific target description reflecting the new vector length.

After this change, all targets except SVE-supporting aarch64-linux will
still use per-process target descriptions.

Reviewed-by: Luis Machado <luis.machado@arm.com>
---
 gdbserver/gdbthread.h          |  4 ++++
 gdbserver/linux-aarch64-low.cc | 31 +++++++++++++++++++++++++++++++
 gdbserver/linux-low.cc         | 17 +++++++++++++++++
 gdbserver/linux-low.h          |  6 ++++++
 gdbserver/regcache.cc          | 10 ++++++----
 gdbserver/regcache.h           |  4 ++++
 gdbserver/tdesc.cc             | 13 ++++++++++++-
 gdbserver/tdesc.h              |  5 +++++
 8 files changed, 85 insertions(+), 5 deletions(-)
  

On 1/30/23 04:45, Thiago Jung Bauermann wrote:
> This change prepares gdbserver to support remotely debugging programs in
> aarch64-linux where different threads have different SVE vector lengths.
> It allows gdbserver to support different inferior threads having different
> target descriptions.
> 
> To that end, a tdesc field is added to struct thread_info.  If it's
> nullptr (the default) it means that the thread uses the target description
> stored in struct process_info and thus gdbserver behaviour is unchanged.
> The get_thread_tdesc method is added to the linux_process_target class to
> allow aarch64-linux code to probe the inferior's vq register and provide a
> thread-specific target description reflecting the new vector length.
> 
> After this change, all targets except SVE-supporting aarch64-linux will
> still use per-process target descriptions.
> 
> Reviewed-by: Luis Machado <luis.machado@arm.com>
> ---
>   gdbserver/gdbthread.h          |  4 ++++
>   gdbserver/linux-aarch64-low.cc | 31 +++++++++++++++++++++++++++++++
>   gdbserver/linux-low.cc         | 17 +++++++++++++++++
>   gdbserver/linux-low.h          |  6 ++++++
>   gdbserver/regcache.cc          | 10 ++++++----
>   gdbserver/regcache.h           |  4 ++++
>   gdbserver/tdesc.cc             | 13 ++++++++++++-
>   gdbserver/tdesc.h              |  5 +++++
>   8 files changed, 85 insertions(+), 5 deletions(-)
> 
> diff --git a/gdbserver/gdbthread.h b/gdbserver/gdbthread.h
> index 493e1dbf6cb6..5b5ba6f8e521 100644
> --- a/gdbserver/gdbthread.h
> +++ b/gdbserver/gdbthread.h
> @@ -80,6 +80,10 @@ struct thread_info
>   
>     /* Branch trace target information for this thread.  */
>     struct btrace_target_info *btrace = nullptr;
> +
> +  /* 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 92c621e5548c..69765ee90db3 100644
> --- a/gdbserver/linux-aarch64-low.cc
> +++ b/gdbserver/linux-aarch64-low.cc
> @@ -99,6 +99,9 @@ protected:
>   
>     void low_arch_setup () override;
>   
> +  const struct target_desc *
> +    get_thread_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,9 @@ 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;
> +
> +  /* Whether this process has the Scalable Vector Extension available.  */
> +  bool has_sve;
>   };
>   
>   /* Return true if the size of register 0 is 8 byte.  */
> @@ -869,6 +875,9 @@ aarch64_target::low_arch_setup ()
>   
>         current_process ()->tdesc = aarch64_linux_read_description (features);
>   
> +      if (features.vq > 0)
> +	current_process ()->priv->arch_private->has_sve = true;
> +
>         /* Adjust the register sets we should use for this particular set of
>   	 features.  */
>         aarch64_adjust_register_sets (features);
> @@ -879,6 +888,28 @@ aarch64_target::low_arch_setup ()
>     aarch64_linux_get_debug_reg_capacity (lwpid_of (current_thread));
>   }
>   
> +/* Implementation of linux target ops method "get_thread_tdesc".  */
> +
> +const struct target_desc *
> +aarch64_target::get_thread_tdesc (const thread_info *thread)
> +{
> +  const struct process_info *process = get_thread_process (thread);
> +
> +  /* Only inferiors with SVE need a thread-specific target description.  */
> +  if (!process->priv->arch_private->has_sve)
> +    return nullptr;
> +
> +  const struct aarch64_features features = aarch64_get_arch_features (thread);
> +  const struct target_desc *tdesc = aarch64_linux_read_description (features);
> +
> +  /* If the target description we just found is the same as the process-wide
> +     one, there's no need to set a thread-specific one.  */
> +  if (tdesc == process->tdesc)
> +    return nullptr;
> +
> +  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 5cd22824e470..47916009ebaf 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 ();
>   }
>   
> +const struct target_desc *
> +linux_process_target::get_thread_tdesc (const thread_info *thread)
> +{
> +  return nullptr;
> +}
> +
>   int
>   linux_process_target::handle_extended_wait (lwp_info **orig_event_lwp,
>   					    int wstat)
> @@ -2348,6 +2354,17 @@ linux_process_target::filter_event (int lwpid, int wstat)
>   	      return;
>   	    }
>   	}
> +      else
> +	{
> +	  /* Give the arch code an opportunity to set the thread's target
> +	     description.  */
> +	  const struct target_desc *new_tdesc = get_thread_tdesc (thread);
> +	  if (new_tdesc != thread->tdesc)
> +	    {
> +	      free_register_cache_thread (thread);
> +	      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 221de85aa2ee..b52eb23cc444 100644
> --- a/gdbserver/linux-low.h
> +++ b/gdbserver/linux-low.h
> @@ -604,6 +604,12 @@ 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 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);
> +
>     /* 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 7b896a19767d..fb60e2c9c399 100644
> --- a/gdbserver/regcache.cc
> +++ b/gdbserver/regcache.cc
> @@ -39,11 +39,11 @@ get_thread_regcache (struct thread_info *thread, int fetch)
>        have.  */
>     if (regcache == NULL)
>       {
> -      struct process_info *proc = get_thread_process (thread);
> +      const target_desc *tdesc = get_thread_target_desc (thread);
>   
> -      gdb_assert (proc->tdesc != NULL);
> +      gdb_assert (tdesc != nullptr);
>   
> -      regcache = new_register_cache (proc->tdesc);
> +      regcache = new_register_cache (tdesc);
>         set_thread_regcache_data (thread, regcache);
>       }
>   
> @@ -270,7 +270,9 @@ find_regno (const struct target_desc *tdesc, const char *name)
>     internal_error ("Unknown register %s requested", name);
>   }
>   
> -static void
> +/* See regcache.h.  */
> +
> +void
>   free_register_cache_thread (struct thread_info *thread)
>   {
>     struct regcache *regcache = thread_regcache_data (thread);
> diff --git a/gdbserver/regcache.h b/gdbserver/regcache.h
> index 7248bcf5808a..4beea0139cd6 100644
> --- a/gdbserver/regcache.h
> +++ b/gdbserver/regcache.h
> @@ -79,6 +79,10 @@ void free_register_cache (struct regcache *regcache);
>   
>   void regcache_invalidate_thread (struct thread_info *);
>   
> +/* Invalidate and release the register cache of the given THREAD.  */
> +
> +void free_register_cache_thread (struct thread_info *thread);
> +
>   /* Invalidate cached registers for all threads of the given process.  */
>   
>   void regcache_invalidate_pid (int pid);
> diff --git a/gdbserver/tdesc.cc b/gdbserver/tdesc.cc
> index 2c7257c458f4..e3339dde4d6c 100644
> --- a/gdbserver/tdesc.cc
> +++ b/gdbserver/tdesc.cc
> @@ -123,13 +123,24 @@ copy_target_description (struct target_desc *dest,
>     dest->xmltarget = src->xmltarget;
>   }
>   
> +/* See tdesc.h.  */
> +
> +const struct target_desc *
> +get_thread_target_desc (const struct thread_info *thread)
> +{
> +  if (thread->tdesc != nullptr)
> +    return thread->tdesc;
> +
> +  return get_thread_process (thread)->tdesc;
> +}
> +
>   const struct target_desc *
>   current_target_desc (void)
>   {
>     if (current_thread == NULL)
>       return &default_description;
>   
> -  return current_process ()->tdesc;
> +  return get_thread_target_desc (current_thread);
>   }
>   
>   /* An empty structure.  */
> diff --git a/gdbserver/tdesc.h b/gdbserver/tdesc.h
> index 7fe7d0d8eb30..71cc5b51c84e 100644
> --- a/gdbserver/tdesc.h
> +++ b/gdbserver/tdesc.h
> @@ -88,6 +88,11 @@ void copy_target_description (struct target_desc *dest,
>   void init_target_desc (struct target_desc *tdesc,
>   		       const char **expedite_regs);
>   
> +/* Return the target description corresponding to the given THREAD.  */
> +
> +const struct target_desc *
> +  get_thread_target_desc (const struct thread_info *thread);
> +
>   /* Return the current inferior's target description.  Never returns
>      NULL.  */
>   

I don't have any comments on this code. It looks good to me.
  

Thiago Jung Bauermann via Gdb-patches <gdb-patches@sourceware.org>
writes:

> This change prepares gdbserver to support remotely debugging programs in
> aarch64-linux where different threads have different SVE vector lengths.
> It allows gdbserver to support different inferior threads having different
> target descriptions.
>
> To that end, a tdesc field is added to struct thread_info.  If it's
> nullptr (the default) it means that the thread uses the target description
> stored in struct process_info and thus gdbserver behaviour is unchanged.
> The get_thread_tdesc method is added to the linux_process_target class to
> allow aarch64-linux code to probe the inferior's vq register and provide a
> thread-specific target description reflecting the new vector length.
>
> After this change, all targets except SVE-supporting aarch64-linux will
> still use per-process target descriptions.
>
> Reviewed-by: Luis Machado <luis.machado@arm.com>
> ---
>  gdbserver/gdbthread.h          |  4 ++++
>  gdbserver/linux-aarch64-low.cc | 31 +++++++++++++++++++++++++++++++
>  gdbserver/linux-low.cc         | 17 +++++++++++++++++
>  gdbserver/linux-low.h          |  6 ++++++
>  gdbserver/regcache.cc          | 10 ++++++----
>  gdbserver/regcache.h           |  4 ++++
>  gdbserver/tdesc.cc             | 13 ++++++++++++-
>  gdbserver/tdesc.h              |  5 +++++
>  8 files changed, 85 insertions(+), 5 deletions(-)
>
> diff --git a/gdbserver/gdbthread.h b/gdbserver/gdbthread.h
> index 493e1dbf6cb6..5b5ba6f8e521 100644
> --- a/gdbserver/gdbthread.h
> +++ b/gdbserver/gdbthread.h
> @@ -80,6 +80,10 @@ struct thread_info
>  
>    /* Branch trace target information for this thread.  */
>    struct btrace_target_info *btrace = nullptr;
> +
> +  /* 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 92c621e5548c..69765ee90db3 100644
> --- a/gdbserver/linux-aarch64-low.cc
> +++ b/gdbserver/linux-aarch64-low.cc
> @@ -99,6 +99,9 @@ protected:
>  
>    void low_arch_setup () override;
>  
> +  const struct target_desc *
> +    get_thread_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,9 @@ 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;
> +
> +  /* Whether this process has the Scalable Vector Extension available.  */
> +  bool has_sve;
>  };
>  
>  /* Return true if the size of register 0 is 8 byte.  */
> @@ -869,6 +875,9 @@ aarch64_target::low_arch_setup ()
>  
>        current_process ()->tdesc = aarch64_linux_read_description (features);
>  
> +      if (features.vq > 0)
> +	current_process ()->priv->arch_private->has_sve = true;
> +
>        /* Adjust the register sets we should use for this particular set of
>  	 features.  */
>        aarch64_adjust_register_sets (features);
> @@ -879,6 +888,28 @@ aarch64_target::low_arch_setup ()
>    aarch64_linux_get_debug_reg_capacity (lwpid_of (current_thread));
>  }
>  
> +/* Implementation of linux target ops method "get_thread_tdesc".  */
> +
> +const struct target_desc *
> +aarch64_target::get_thread_tdesc (const thread_info *thread)
> +{
> +  const struct process_info *process = get_thread_process (thread);
> +
> +  /* Only inferiors with SVE need a thread-specific target description.  */
> +  if (!process->priv->arch_private->has_sve)
> +    return nullptr;
> +
> +  const struct aarch64_features features = aarch64_get_arch_features (thread);
> +  const struct target_desc *tdesc = aarch64_linux_read_description (features);
> +
> +  /* If the target description we just found is the same as the process-wide
> +     one, there's no need to set a thread-specific one.  */
> +  if (tdesc == process->tdesc)
> +    return nullptr;
> +
> +  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 5cd22824e470..47916009ebaf 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 ();
>  }
>  
> +const struct target_desc *
> +linux_process_target::get_thread_tdesc (const thread_info *thread)
> +{
> +  return nullptr;
> +}
> +
>  int
>  linux_process_target::handle_extended_wait (lwp_info **orig_event_lwp,
>  					    int wstat)
> @@ -2348,6 +2354,17 @@ linux_process_target::filter_event (int lwpid, int wstat)
>  	      return;
>  	    }
>  	}
> +      else
> +	{
> +	  /* Give the arch code an opportunity to set the thread's target
> +	     description.  */
> +	  const struct target_desc *new_tdesc = get_thread_tdesc (thread);
> +	  if (new_tdesc != thread->tdesc)
> +	    {
> +	      free_register_cache_thread (thread);
> +	      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 221de85aa2ee..b52eb23cc444 100644
> --- a/gdbserver/linux-low.h
> +++ b/gdbserver/linux-low.h
> @@ -604,6 +604,12 @@ 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 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);

I think the comment for this function is not correct.  The function does
not SET the thread's target description, but just GETS a target
description suitable for `thread`.  It's the caller's job to do the
setting.

Thanks,
Andrew

> +
>    /* 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 7b896a19767d..fb60e2c9c399 100644
> --- a/gdbserver/regcache.cc
> +++ b/gdbserver/regcache.cc
> @@ -39,11 +39,11 @@ get_thread_regcache (struct thread_info *thread, int fetch)
>       have.  */
>    if (regcache == NULL)
>      {
> -      struct process_info *proc = get_thread_process (thread);
> +      const target_desc *tdesc = get_thread_target_desc (thread);
>  
> -      gdb_assert (proc->tdesc != NULL);
> +      gdb_assert (tdesc != nullptr);
>  
> -      regcache = new_register_cache (proc->tdesc);
> +      regcache = new_register_cache (tdesc);
>        set_thread_regcache_data (thread, regcache);
>      }
>  
> @@ -270,7 +270,9 @@ find_regno (const struct target_desc *tdesc, const char *name)
>    internal_error ("Unknown register %s requested", name);
>  }
>  
> -static void
> +/* See regcache.h.  */
> +
> +void
>  free_register_cache_thread (struct thread_info *thread)
>  {
>    struct regcache *regcache = thread_regcache_data (thread);
> diff --git a/gdbserver/regcache.h b/gdbserver/regcache.h
> index 7248bcf5808a..4beea0139cd6 100644
> --- a/gdbserver/regcache.h
> +++ b/gdbserver/regcache.h
> @@ -79,6 +79,10 @@ void free_register_cache (struct regcache *regcache);
>  
>  void regcache_invalidate_thread (struct thread_info *);
>  
> +/* Invalidate and release the register cache of the given THREAD.  */
> +
> +void free_register_cache_thread (struct thread_info *thread);
> +
>  /* Invalidate cached registers for all threads of the given process.  */
>  
>  void regcache_invalidate_pid (int pid);
> diff --git a/gdbserver/tdesc.cc b/gdbserver/tdesc.cc
> index 2c7257c458f4..e3339dde4d6c 100644
> --- a/gdbserver/tdesc.cc
> +++ b/gdbserver/tdesc.cc
> @@ -123,13 +123,24 @@ copy_target_description (struct target_desc *dest,
>    dest->xmltarget = src->xmltarget;
>  }
>  
> +/* See tdesc.h.  */
> +
> +const struct target_desc *
> +get_thread_target_desc (const struct thread_info *thread)
> +{
> +  if (thread->tdesc != nullptr)
> +    return thread->tdesc;
> +
> +  return get_thread_process (thread)->tdesc;
> +}
> +
>  const struct target_desc *
>  current_target_desc (void)
>  {
>    if (current_thread == NULL)
>      return &default_description;
>  
> -  return current_process ()->tdesc;
> +  return get_thread_target_desc (current_thread);
>  }
>  
>  /* An empty structure.  */
> diff --git a/gdbserver/tdesc.h b/gdbserver/tdesc.h
> index 7fe7d0d8eb30..71cc5b51c84e 100644
> --- a/gdbserver/tdesc.h
> +++ b/gdbserver/tdesc.h
> @@ -88,6 +88,11 @@ void copy_target_description (struct target_desc *dest,
>  void init_target_desc (struct target_desc *tdesc,
>  		       const char **expedite_regs);
>  
> +/* Return the target description corresponding to the given THREAD.  */
> +
> +const struct target_desc *
> +  get_thread_target_desc (const struct thread_info *thread);
> +
>  /* Return the current inferior's target description.  Never returns
>     NULL.  */
>
  

>> diff --git a/gdbserver/linux-low.h b/gdbserver/linux-low.h
>> index 221de85aa2ee..b52eb23cc444 100644
>> --- a/gdbserver/linux-low.h
>> +++ b/gdbserver/linux-low.h
>> @@ -604,6 +604,12 @@ 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 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);
> 
> I think the comment for this function is not correct.  The function does
> not SET the thread's target description, but just GETS a target
> description suitable for `thread`.  It's the caller's job to do the
> setting.

This comment also gave me pause.  How does a getter set something.  I
then understood that it allowed the arch-specific code to provide a
thread-specific tdesc.  I would suggest just:

  /* Return a target description for THREAD.

     Return nullptr if no thread-specific description is necessary.  */

The other thought I had while re-reading the patch is why do we need to
return and store nullptr if the thread target description is the same as
the main one for the process.  get_thread_tdesc could just return
process_info->tdesc if we don't need a separate tdesc, and we would
store that same pointer in thread_info->tdesc.  And get_thread_tdesc
would just return that (in fact, get_thread_tdesc might not be necessary
then).  Perhaps it makes some things more complicated down the road, but
I can't think of anything.

Simon
  

On 2/1/23 16:21, Simon Marchi wrote:
> 
>>> diff --git a/gdbserver/linux-low.h b/gdbserver/linux-low.h
>>> index 221de85aa2ee..b52eb23cc444 100644
>>> --- a/gdbserver/linux-low.h
>>> +++ b/gdbserver/linux-low.h
>>> @@ -604,6 +604,12 @@ 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 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);
>>
>> I think the comment for this function is not correct.  The function does
>> not SET the thread's target description, but just GETS a target
>> description suitable for `thread`.  It's the caller's job to do the
>> setting.
> 
> This comment also gave me pause.  How does a getter set something.  I
> then understood that it allowed the arch-specific code to provide a
> thread-specific tdesc.  I would suggest just:

FWIW, I read it as "the functions *allows* arch-specific code to set". So it doesn't set on its own, but it does allow something else to do it.

> 
>    /* Return a target description for THREAD.
> 
>       Return nullptr if no thread-specific description is necessary.  */
> 
> The other thought I had while re-reading the patch is why do we need to
> return and store nullptr if the thread target description is the same as
> the main one for the process.  get_thread_tdesc could just return
> process_info->tdesc if we don't need a separate tdesc, and we would
> store that same pointer in thread_info->tdesc.  And get_thread_tdesc
> would just return that (in fact, get_thread_tdesc might not be necessary
> then).  Perhaps it makes some things more complicated down the road, but
> I can't think of anything.

Sounds reasonable.

Moving towards thread-specific target descriptions/gdbarch would be a positive thing given the SVE precedent. The process-wide target description/gdbarch no
longer reflects the correct settings for each thread on AArch64's with SVE support.

> 
> Simon
  

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

> On 2/1/23 16:21, Simon Marchi wrote:
>> 
>>>> diff --git a/gdbserver/linux-low.h b/gdbserver/linux-low.h
>>>> index 221de85aa2ee..b52eb23cc444 100644
>>>> --- a/gdbserver/linux-low.h
>>>> +++ b/gdbserver/linux-low.h
>>>> @@ -604,6 +604,12 @@ 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 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);
>>>
>>> I think the comment for this function is not correct.  The function does
>>> not SET the thread's target description, but just GETS a target
>>> description suitable for `thread`.  It's the caller's job to do the
>>> setting.
>> This comment also gave me pause.  How does a getter set something.  I
>> then understood that it allowed the arch-specific code to provide a
>> thread-specific tdesc.  I would suggest just:
>
> FWIW, I read it as "the functions *allows* arch-specific code to set".
> So it doesn't set on its own, but it does allow something else to do
> it.

Yes, that's what was in my mind when I wrote the comment. But I agree
it's unclear, and I adopted Simon's suggested version.

>> The other thought I had while re-reading the patch is why do we need to
>> return and store nullptr if the thread target description is the same as
>> the main one for the process.  get_thread_tdesc could just return
>> process_info->tdesc if we don't need a separate tdesc, and we would
>> store that same pointer in thread_info->tdesc.

We don't need to return and store nullptr if the thread target
description is the same as the main one for the process. Things will
work fine if we do as you suggest. IIRC my private branch worked liked
that for a while, before I changed it to the current version.

I changed it because I thought it was a clearer mental model if
thread_info->tdesc is nullptr when there's not thread-specific target
description. I can make the get_thread_tdesc method always return a
valid target description if you think it's better that way.

>> And get_thread_tdesc would just return that (in fact,
>> get_thread_tdesc might not be necessary then). Perhaps it makes some
>> things more complicated down the road, but I can't think of anything.

Sorry, I don't understand this part. get_thread_tdesc is necessary
because it's the hook that allows arch-specific code to provide a target
description for the thread. I don't see how it can become unnecessary.

Perhaps you mean the get_thread_target_desc function? Sorry about the
names being so similar, I spent some time trying to think of a better
name for either the method or the function but failed.

In any case, it wouldn't be possible to make get_thread_target_desc just
return thread_info->tdesc because at least the way these patches are
currently written, when the inferior starts or a new thread of the
inferior is spawned thread_info->tdesc is nullptr. gdbserver will only
call get_thread_tdesc after the first stop (in get_thread_regcache, in
the process of obtaining the pc register), so we will need to cope with
that situation.

> Sounds reasonable.
>
> Moving towards thread-specific target descriptions/gdbarch would be a positive thing given
> the SVE precedent. The process-wide target description/gdbarch no
> longer reflects the correct settings for each thread on AArch64's with SVE support.

In the first version of these patches I removed the process-wide target
description and moved it to thread_info, but it was a big patch that
touched many targets. I can bring it back if you think it's worth it.
  

On 2/1/23 21:54, Thiago Jung Bauermann wrote:
> 
> Luis Machado <luis.machado@arm.com> writes:
> 
>> On 2/1/23 16:21, Simon Marchi wrote:
>>>
>>>>> diff --git a/gdbserver/linux-low.h b/gdbserver/linux-low.h
>>>>> index 221de85aa2ee..b52eb23cc444 100644
>>>>> --- a/gdbserver/linux-low.h
>>>>> +++ b/gdbserver/linux-low.h
>>>>> @@ -604,6 +604,12 @@ 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 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);
>>>>
>>>> I think the comment for this function is not correct.  The function does
>>>> not SET the thread's target description, but just GETS a target
>>>> description suitable for `thread`.  It's the caller's job to do the
>>>> setting.
>>> This comment also gave me pause.  How does a getter set something.  I
>>> then understood that it allowed the arch-specific code to provide a
>>> thread-specific tdesc.  I would suggest just:
>>
>> FWIW, I read it as "the functions *allows* arch-specific code to set".
>> So it doesn't set on its own, but it does allow something else to do
>> it.
> 
> Yes, that's what was in my mind when I wrote the comment. But I agree
> it's unclear, and I adopted Simon's suggested version.
> 
>>> The other thought I had while re-reading the patch is why do we need to
>>> return and store nullptr if the thread target description is the same as
>>> the main one for the process.  get_thread_tdesc could just return
>>> process_info->tdesc if we don't need a separate tdesc, and we would
>>> store that same pointer in thread_info->tdesc.
> 
> We don't need to return and store nullptr if the thread target
> description is the same as the main one for the process. Things will
> work fine if we do as you suggest. IIRC my private branch worked liked
> that for a while, before I changed it to the current version.
> 
> I changed it because I thought it was a clearer mental model if
> thread_info->tdesc is nullptr when there's not thread-specific target
> description. I can make the get_thread_tdesc method always return a
> valid target description if you think it's better that way.

Either way works.

>>> And get_thread_tdesc would just return that (in fact,
>>> get_thread_tdesc might not be necessary then). Perhaps it makes some
>>> things more complicated down the road, but I can't think of anything.
> 
> Sorry, I don't understand this part. get_thread_tdesc is necessary
> because it's the hook that allows arch-specific code to provide a target
> description for the thread. I don't see how it can become unnecessary.
> 
> Perhaps you mean the get_thread_target_desc function? Sorry about the
> names being so similar, I spent some time trying to think of a better
> name for either the method or the function but failed.

Err yeah, I meant the free function that returns the process' tdesc if
the thread doesn't have one.

> In any case, it wouldn't be possible to make get_thread_target_desc just
> return thread_info->tdesc because at least the way these patches are
> currently written, when the inferior starts or a new thread of the
> inferior is spawned thread_info->tdesc is nullptr. gdbserver will only
> call get_thread_tdesc after the first stop (in get_thread_regcache, in
> the process of obtaining the pc register), so we will need to cope with
> that situation.

Ok.  Would it work if a new thread initially inherited the tdesc from
its process?

>> Sounds reasonable.
>>
>> Moving towards thread-specific target descriptions/gdbarch would be a positive thing given
>> the SVE precedent. The process-wide target description/gdbarch no
>> longer reflects the correct settings for each thread on AArch64's with SVE support.
> 
> In the first version of these patches I removed the process-wide target
> description and moved it to thread_info, but it was a big patch that
> touched many targets. I can bring it back if you think it's worth it.

At least for the register description, if we decide it's now a
per-thread thing, what does it mean to have a process-wide description
anyway?  I think it would make sense to get rid of it, that could help
confirm our model works (and it would remove the chance of  using the
wrong tdesc by mistake for a thread that has its own tdesc).  It's
probably something that can be done incrementally though.

Simon
  

Simon Marchi <simark@simark.ca> writes:

> On 2/1/23 21:54, Thiago Jung Bauermann wrote:
>> 
>> Luis Machado <luis.machado@arm.com> writes:
>> 
>>> On 2/1/23 16:21, Simon Marchi wrote:
>>>>
>>>>>> diff --git a/gdbserver/linux-low.h b/gdbserver/linux-low.h
>>>>>> index 221de85aa2ee..b52eb23cc444 100644
>>>>>> --- a/gdbserver/linux-low.h
>>>>>> +++ b/gdbserver/linux-low.h
>>>>>> @@ -604,6 +604,12 @@ 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 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);
>>>>>
>>>>> I think the comment for this function is not correct.  The function does
>>>>> not SET the thread's target description, but just GETS a target
>>>>> description suitable for `thread`.  It's the caller's job to do the
>>>>> setting.
>>>> This comment also gave me pause.  How does a getter set something.  I
>>>> then understood that it allowed the arch-specific code to provide a
>>>> thread-specific tdesc.  I would suggest just:
>>>
>>> FWIW, I read it as "the functions *allows* arch-specific code to set".
>>> So it doesn't set on its own, but it does allow something else to do
>>> it.
>> 
>> Yes, that's what was in my mind when I wrote the comment. But I agree
>> it's unclear, and I adopted Simon's suggested version.
>> 
>>>> The other thought I had while re-reading the patch is why do we need to
>>>> return and store nullptr if the thread target description is the same as
>>>> the main one for the process.  get_thread_tdesc could just return
>>>> process_info->tdesc if we don't need a separate tdesc, and we would
>>>> store that same pointer in thread_info->tdesc.
>> 
>> We don't need to return and store nullptr if the thread target
>> description is the same as the main one for the process. Things will
>> work fine if we do as you suggest. IIRC my private branch worked liked
>> that for a while, before I changed it to the current version.
>> 
>> I changed it because I thought it was a clearer mental model if
>> thread_info->tdesc is nullptr when there's not thread-specific target
>> description. I can make the get_thread_tdesc method always return a
>> valid target description if you think it's better that way.
>
> Either way works.

Ok, if there's no preference I suggest leaving it as it is now (unless
we decide moving away from process_info->tdesc).

>>>> And get_thread_tdesc would just return that (in fact,
>>>> get_thread_tdesc might not be necessary then). Perhaps it makes some
>>>> things more complicated down the road, but I can't think of anything.
>> 
>> Sorry, I don't understand this part. get_thread_tdesc is necessary
>> because it's the hook that allows arch-specific code to provide a target
>> description for the thread. I don't see how it can become unnecessary.
>> 
>> Perhaps you mean the get_thread_target_desc function? Sorry about the
>> names being so similar, I spent some time trying to think of a better
>> name for either the method or the function but failed.
>
> Err yeah, I meant the free function that returns the process' tdesc if
> the thread doesn't have one.
>
>> In any case, it wouldn't be possible to make get_thread_target_desc just
>> return thread_info->tdesc because at least the way these patches are
>> currently written, when the inferior starts or a new thread of the
>> inferior is spawned thread_info->tdesc is nullptr. gdbserver will only
>> call get_thread_tdesc after the first stop (in get_thread_regcache, in
>> the process of obtaining the pc register), so we will need to cope with
>> that situation.
>
> Ok.  Would it work if a new thread initially inherited the tdesc from
> its process?

Yes, it would. Version 1 of these patches didn't work exactly like that
because I removed process_info->tdesc, but the new thread inherited from
the parent thread's tdesc. This happened in
linux_process_target::handle_extended_wait where the clone and fork
events are handled.

>>> Sounds reasonable.
>>>
>>> Moving towards thread-specific target descriptions/gdbarch would be a positive thing
>>> given
>>> the SVE precedent. The process-wide target description/gdbarch no
>>> longer reflects the correct settings for each thread on AArch64's with SVE support.
>> 
>> In the first version of these patches I removed the process-wide target
>> description and moved it to thread_info, but it was a big patch that
>> touched many targets. I can bring it back if you think it's worth it.
>
> At least for the register description, if we decide it's now a
> per-thread thing, what does it mean to have a process-wide description
> anyway?  I think it would make sense to get rid of it, that could help
> confirm our model works (and it would remove the chance of  using the
> wrong tdesc by mistake for a thread that has its own tdesc).  It's
> probably something that can be done incrementally though.

Would it be done incrementally by keeping process_info->tdesc but
changing each target in turn to use thread_info->tdesc and ignore the
process_info one?
  

On 2/2/23 02:54, Thiago Jung Bauermann wrote:
> 
> Luis Machado <luis.machado@arm.com> writes:
> 
>> On 2/1/23 16:21, Simon Marchi wrote:
>>>
>>>>> diff --git a/gdbserver/linux-low.h b/gdbserver/linux-low.h
>>>>> index 221de85aa2ee..b52eb23cc444 100644
>>>>> --- a/gdbserver/linux-low.h
>>>>> +++ b/gdbserver/linux-low.h
>>>>> @@ -604,6 +604,12 @@ 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 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);
>>>>
>>>> I think the comment for this function is not correct.  The function does
>>>> not SET the thread's target description, but just GETS a target
>>>> description suitable for `thread`.  It's the caller's job to do the
>>>> setting.
>>> This comment also gave me pause.  How does a getter set something.  I
>>> then understood that it allowed the arch-specific code to provide a
>>> thread-specific tdesc.  I would suggest just:
>>
>> FWIW, I read it as "the functions *allows* arch-specific code to set".
>> So it doesn't set on its own, but it does allow something else to do
>> it.
> 
> Yes, that's what was in my mind when I wrote the comment. But I agree
> it's unclear, and I adopted Simon's suggested version.
> 
>>> The other thought I had while re-reading the patch is why do we need to
>>> return and store nullptr if the thread target description is the same as
>>> the main one for the process.  get_thread_tdesc could just return
>>> process_info->tdesc if we don't need a separate tdesc, and we would
>>> store that same pointer in thread_info->tdesc.
> 
> We don't need to return and store nullptr if the thread target
> description is the same as the main one for the process. Things will
> work fine if we do as you suggest. IIRC my private branch worked liked
> that for a while, before I changed it to the current version.
> 
> I changed it because I thought it was a clearer mental model if
> thread_info->tdesc is nullptr when there's not thread-specific target
> description. I can make the get_thread_tdesc method always return a
> valid target description if you think it's better that way.
> 
>>> And get_thread_tdesc would just return that (in fact,
>>> get_thread_tdesc might not be necessary then). Perhaps it makes some
>>> things more complicated down the road, but I can't think of anything.
> 
> Sorry, I don't understand this part. get_thread_tdesc is necessary
> because it's the hook that allows arch-specific code to provide a target
> description for the thread. I don't see how it can become unnecessary.
> 
> Perhaps you mean the get_thread_target_desc function? Sorry about the
> names being so similar, I spent some time trying to think of a better
> name for either the method or the function but failed.
> 
> In any case, it wouldn't be possible to make get_thread_target_desc just
> return thread_info->tdesc because at least the way these patches are
> currently written, when the inferior starts or a new thread of the
> inferior is spawned thread_info->tdesc is nullptr. gdbserver will only
> call get_thread_tdesc after the first stop (in get_thread_regcache, in
> the process of obtaining the pc register), so we will need to cope with
> that situation.
> 
>> Sounds reasonable.
>>
>> Moving towards thread-specific target descriptions/gdbarch would be a positive thing given
>> the SVE precedent. The process-wide target description/gdbarch no
>> longer reflects the correct settings for each thread on AArch64's with SVE support.
> 
> In the first version of these patches I removed the process-wide target
> description and moved it to thread_info, but it was a big patch that
> touched many targets. I can bring it back if you think it's worth it

No need. As Simon suggested, we can do this incrementally. I don't think we should hold off on
this series' particular improvements so we can get the more general case sorted.
  

On 2/2/23 03:47, Simon Marchi wrote:
> 
> 
> On 2/1/23 21:54, Thiago Jung Bauermann wrote:
>>
>> Luis Machado <luis.machado@arm.com> writes:
>>
>>> On 2/1/23 16:21, Simon Marchi wrote:
>>>>
>>>>>> diff --git a/gdbserver/linux-low.h b/gdbserver/linux-low.h
>>>>>> index 221de85aa2ee..b52eb23cc444 100644
>>>>>> --- a/gdbserver/linux-low.h
>>>>>> +++ b/gdbserver/linux-low.h
>>>>>> @@ -604,6 +604,12 @@ 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 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);
>>>>>
>>>>> I think the comment for this function is not correct.  The function does
>>>>> not SET the thread's target description, but just GETS a target
>>>>> description suitable for `thread`.  It's the caller's job to do the
>>>>> setting.
>>>> This comment also gave me pause.  How does a getter set something.  I
>>>> then understood that it allowed the arch-specific code to provide a
>>>> thread-specific tdesc.  I would suggest just:
>>>
>>> FWIW, I read it as "the functions *allows* arch-specific code to set".
>>> So it doesn't set on its own, but it does allow something else to do
>>> it.
>>
>> Yes, that's what was in my mind when I wrote the comment. But I agree
>> it's unclear, and I adopted Simon's suggested version.
>>
>>>> The other thought I had while re-reading the patch is why do we need to
>>>> return and store nullptr if the thread target description is the same as
>>>> the main one for the process.  get_thread_tdesc could just return
>>>> process_info->tdesc if we don't need a separate tdesc, and we would
>>>> store that same pointer in thread_info->tdesc.
>>
>> We don't need to return and store nullptr if the thread target
>> description is the same as the main one for the process. Things will
>> work fine if we do as you suggest. IIRC my private branch worked liked
>> that for a while, before I changed it to the current version.
>>
>> I changed it because I thought it was a clearer mental model if
>> thread_info->tdesc is nullptr when there's not thread-specific target
>> description. I can make the get_thread_tdesc method always return a
>> valid target description if you think it's better that way.
> 
> Either way works.
> 
>>>> And get_thread_tdesc would just return that (in fact,
>>>> get_thread_tdesc might not be necessary then). Perhaps it makes some
>>>> things more complicated down the road, but I can't think of anything.
>>
>> Sorry, I don't understand this part. get_thread_tdesc is necessary
>> because it's the hook that allows arch-specific code to provide a target
>> description for the thread. I don't see how it can become unnecessary.
>>
>> Perhaps you mean the get_thread_target_desc function? Sorry about the
>> names being so similar, I spent some time trying to think of a better
>> name for either the method or the function but failed.
> 
> Err yeah, I meant the free function that returns the process' tdesc if
> the thread doesn't have one.
> 
>> In any case, it wouldn't be possible to make get_thread_target_desc just
>> return thread_info->tdesc because at least the way these patches are
>> currently written, when the inferior starts or a new thread of the
>> inferior is spawned thread_info->tdesc is nullptr. gdbserver will only
>> call get_thread_tdesc after the first stop (in get_thread_regcache, in
>> the process of obtaining the pc register), so we will need to cope with
>> that situation.
> 
> Ok.  Would it work if a new thread initially inherited the tdesc from
> its process?
> 

It should be fine because the first time we fetch a process target description, it is eventually obtained from the first and only thread. So the SVE vector length should be correct.

Any subsequent attempts to use the process' target description (the first one we obtained), after further stops, may end up using an incorrect description.

I think this is handled correctly by the target architecture target hook though. But there are other places where this is potentially incorrect.

For example...

- When using gcore to dump a core file, GDB only dumps a single target description. While this might be correct for a target with a fixed target description or a AArch64 target that doesn't support SVE, it likely won't be correctly for one
AArch64 target supporting SVE if its threads changed vector length mid-execution. Either we emit target description notes by thread, or we don't emit a target description note for those cases.

- When loading the above/older gcore core files back, GDB will use a potentially incorrect target description. If we decide to emit per-thread target descriptions, it should be fine. Otherwise we may need to have a "thread architecture" hook for core files as well.

- The remote has no concept of a thread architecture (Thiago is addressing this with this patch series).

- AArch64 frames may have slightly different vg values, which means their gdbarches are different as well.

Given the differences between two gdbarches are small, we mostly get away with it. But if there are further differences (different hooks, for example), I fear we may run into a situation where we use an incorrect gdbarch to call a particular hook.

>>> Sounds reasonable.
>>>
>>> Moving towards thread-specific target descriptions/gdbarch would be a positive thing given
>>> the SVE precedent. The process-wide target description/gdbarch no
>>> longer reflects the correct settings for each thread on AArch64's with SVE support.
>>
>> In the first version of these patches I removed the process-wide target
>> description and moved it to thread_info, but it was a big patch that
>> touched many targets. I can bring it back if you think it's worth it.
> 
> At least for the register description, if we decide it's now a
> per-thread thing, what does it mean to have a process-wide description
> anyway?  I think it would make sense to get rid of it, that could help

For AArch64 SVE, process-wide target descriptions mean it is the initial state. But it can't be trusted for further stops/events, in which case we need to trust the thread architecture hook.

> confirm our model works (and it would remove the chance of  using the
> wrong tdesc by mistake for a thread that has its own tdesc).  It's
> probably something that can be done incrementally though.
> 
> Simon
  

On 2/3/23 03:47, Thiago Jung Bauermann wrote:
> 
> Simon Marchi <simark@simark.ca> writes:
> 
>> On 2/1/23 21:54, Thiago Jung Bauermann wrote:
>>>
>>> Luis Machado <luis.machado@arm.com> writes:
>>>
>>>> On 2/1/23 16:21, Simon Marchi wrote:
>>>>>
>>>>>>> diff --git a/gdbserver/linux-low.h b/gdbserver/linux-low.h
>>>>>>> index 221de85aa2ee..b52eb23cc444 100644
>>>>>>> --- a/gdbserver/linux-low.h
>>>>>>> +++ b/gdbserver/linux-low.h
>>>>>>> @@ -604,6 +604,12 @@ 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 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);
>>>>>>
>>>>>> I think the comment for this function is not correct.  The function does
>>>>>> not SET the thread's target description, but just GETS a target
>>>>>> description suitable for `thread`.  It's the caller's job to do the
>>>>>> setting.
>>>>> This comment also gave me pause.  How does a getter set something.  I
>>>>> then understood that it allowed the arch-specific code to provide a
>>>>> thread-specific tdesc.  I would suggest just:
>>>>
>>>> FWIW, I read it as "the functions *allows* arch-specific code to set".
>>>> So it doesn't set on its own, but it does allow something else to do
>>>> it.
>>>
>>> Yes, that's what was in my mind when I wrote the comment. But I agree
>>> it's unclear, and I adopted Simon's suggested version.
>>>
>>>>> The other thought I had while re-reading the patch is why do we need to
>>>>> return and store nullptr if the thread target description is the same as
>>>>> the main one for the process.  get_thread_tdesc could just return
>>>>> process_info->tdesc if we don't need a separate tdesc, and we would
>>>>> store that same pointer in thread_info->tdesc.
>>>
>>> We don't need to return and store nullptr if the thread target
>>> description is the same as the main one for the process. Things will
>>> work fine if we do as you suggest. IIRC my private branch worked liked
>>> that for a while, before I changed it to the current version.
>>>
>>> I changed it because I thought it was a clearer mental model if
>>> thread_info->tdesc is nullptr when there's not thread-specific target
>>> description. I can make the get_thread_tdesc method always return a
>>> valid target description if you think it's better that way.
>>
>> Either way works.
> 
> Ok, if there's no preference I suggest leaving it as it is now (unless
> we decide moving away from process_info->tdesc).
> 
>>>>> And get_thread_tdesc would just return that (in fact,
>>>>> get_thread_tdesc might not be necessary then). Perhaps it makes some
>>>>> things more complicated down the road, but I can't think of anything.
>>>
>>> Sorry, I don't understand this part. get_thread_tdesc is necessary
>>> because it's the hook that allows arch-specific code to provide a target
>>> description for the thread. I don't see how it can become unnecessary.
>>>
>>> Perhaps you mean the get_thread_target_desc function? Sorry about the
>>> names being so similar, I spent some time trying to think of a better
>>> name for either the method or the function but failed.
>>
>> Err yeah, I meant the free function that returns the process' tdesc if
>> the thread doesn't have one.
>>
>>> In any case, it wouldn't be possible to make get_thread_target_desc just
>>> return thread_info->tdesc because at least the way these patches are
>>> currently written, when the inferior starts or a new thread of the
>>> inferior is spawned thread_info->tdesc is nullptr. gdbserver will only
>>> call get_thread_tdesc after the first stop (in get_thread_regcache, in
>>> the process of obtaining the pc register), so we will need to cope with
>>> that situation.
>>
>> Ok.  Would it work if a new thread initially inherited the tdesc from
>> its process?
> 
> Yes, it would. Version 1 of these patches didn't work exactly like that
> because I removed process_info->tdesc, but the new thread inherited from
> the parent thread's tdesc. This happened in
> linux_process_target::handle_extended_wait where the clone and fork
> events are handled.
> 
>>>> Sounds reasonable.
>>>>
>>>> Moving towards thread-specific target descriptions/gdbarch would be a positive thing
>>>> given
>>>> the SVE precedent. The process-wide target description/gdbarch no
>>>> longer reflects the correct settings for each thread on AArch64's with SVE support.
>>>
>>> In the first version of these patches I removed the process-wide target
>>> description and moved it to thread_info, but it was a big patch that
>>> touched many targets. I can bring it back if you think it's worth it.
>>
>> At least for the register description, if we decide it's now a
>> per-thread thing, what does it mean to have a process-wide description
>> anyway?  I think it would make sense to get rid of it, that could help
>> confirm our model works (and it would remove the chance of  using the
>> wrong tdesc by mistake for a thread that has its own tdesc).  It's
>> probably something that can be done incrementally though.
> 
> Would it be done incrementally by keeping process_info->tdesc but
> changing each target in turn to use thread_info->tdesc and ignore the
> process_info one?
> 

Given only AArch64 has this requirement at the moment, it may be easier to leave the process-wide tdesc up until the point we can safely/easily switch to thread-specific ones.

After that, AArch64 will have potentially distinct thread tdescs while other architectures will have threads sharing the same tdesc.
  

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

> On 2/2/23 02:54, Thiago Jung Bauermann wrote:
>> Luis Machado <luis.machado@arm.com> writes:
>> 
>>> On 2/1/23 16:21, Simon Marchi wrote:
>>>>
>>>>>> diff --git a/gdbserver/linux-low.h b/gdbserver/linux-low.h
>>>>>> index 221de85aa2ee..b52eb23cc444 100644
>>>>>> --- a/gdbserver/linux-low.h
>>>>>> +++ b/gdbserver/linux-low.h
>>>>>> @@ -604,6 +604,12 @@ 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 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);
>>>>>
>>>>> I think the comment for this function is not correct.  The function does
>>>>> not SET the thread's target description, but just GETS a target
>>>>> description suitable for `thread`.  It's the caller's job to do the
>>>>> setting.
>>>> This comment also gave me pause.  How does a getter set something.  I
>>>> then understood that it allowed the arch-specific code to provide a
>>>> thread-specific tdesc.  I would suggest just:
>>>
>>> FWIW, I read it as "the functions *allows* arch-specific code to set".
>>> So it doesn't set on its own, but it does allow something else to do
>>> it.
>> Yes, that's what was in my mind when I wrote the comment. But I agree
>> it's unclear, and I adopted Simon's suggested version.
>> 
>>>> The other thought I had while re-reading the patch is why do we need to
>>>> return and store nullptr if the thread target description is the same as
>>>> the main one for the process.  get_thread_tdesc could just return
>>>> process_info->tdesc if we don't need a separate tdesc, and we would
>>>> store that same pointer in thread_info->tdesc.
>> We don't need to return and store nullptr if the thread target
>> description is the same as the main one for the process. Things will
>> work fine if we do as you suggest. IIRC my private branch worked liked
>> that for a while, before I changed it to the current version.
>> I changed it because I thought it was a clearer mental model if
>> thread_info->tdesc is nullptr when there's not thread-specific target
>> description. I can make the get_thread_tdesc method always return a
>> valid target description if you think it's better that way.
>> 
>>>> And get_thread_tdesc would just return that (in fact,
>>>> get_thread_tdesc might not be necessary then). Perhaps it makes some
>>>> things more complicated down the road, but I can't think of anything.
>> Sorry, I don't understand this part. get_thread_tdesc is necessary
>> because it's the hook that allows arch-specific code to provide a target
>> description for the thread. I don't see how it can become unnecessary.
>> Perhaps you mean the get_thread_target_desc function? Sorry about the
>> names being so similar, I spent some time trying to think of a better
>> name for either the method or the function but failed.
>> In any case, it wouldn't be possible to make get_thread_target_desc just
>> return thread_info->tdesc because at least the way these patches are
>> currently written, when the inferior starts or a new thread of the
>> inferior is spawned thread_info->tdesc is nullptr. gdbserver will only
>> call get_thread_tdesc after the first stop (in get_thread_regcache, in
>> the process of obtaining the pc register), so we will need to cope with
>> that situation.
>> 
>>> Sounds reasonable.
>>>
>>> Moving towards thread-specific target descriptions/gdbarch would be a positive thing
>>> given
>>> the SVE precedent. The process-wide target description/gdbarch no
>>> longer reflects the correct settings for each thread on AArch64's with SVE support.
>> In the first version of these patches I removed the process-wide target
>> description and moved it to thread_info, but it was a big patch that
>> touched many targets. I can bring it back if you think it's worth it
>
> No need. As Simon suggested, we can do this incrementally. I don't think we should hold
> off on
> this series' particular improvements so we can get the more general case sorted.

Sounds good to me. I'll be glad to work on a follow-up series with these
improvements after this one is done.
  

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

> On 2/2/23 03:47, Simon Marchi wrote:
>> On 2/1/23 21:54, Thiago Jung Bauermann wrote:
>>> In any case, it wouldn't be possible to make get_thread_target_desc just
>>> return thread_info->tdesc because at least the way these patches are
>>> currently written, when the inferior starts or a new thread of the
>>> inferior is spawned thread_info->tdesc is nullptr. gdbserver will only
>>> call get_thread_tdesc after the first stop (in get_thread_regcache, in
>>> the process of obtaining the pc register), so we will need to cope with
>>> that situation.
>> Ok.  Would it work if a new thread initially inherited the tdesc from
>> its process?
>> 
>
> It should be fine because the first time we fetch a process target
> description, it is eventually obtained from the first and only thread.
> So the SVE vector length should be correct.
>
> Any subsequent attempts to use the process' target description (the
> first one we obtained), after further stops, may end up using an
> incorrect description.
>
> I think this is handled correctly by the target architecture target
> hook though. But there are other places where this is potentially
> incorrect.
>
> For example...
>
> - When using gcore to dump a core file, GDB only dumps a single target
> description. While this might be correct for a target with a fixed
> target description or a AArch64 target that doesn't support SVE, it
> likely won't be correctly for one AArch64 target supporting SVE if its
> threads changed vector length mid-execution. Either we emit target
> description notes by thread, or we don't emit a target description
> note for those cases.
>
> - When loading the above/older gcore core files back, GDB will use a
> potentially incorrect target description. If we decide to emit
> per-thread target descriptions, it should be fine. Otherwise we may
> need to have a "thread architecture" hook for core files as well.
>
> - The remote has no concept of a thread architecture (Thiago is
> addressing this with this patch series).
>
> - AArch64 frames may have slightly different vg values, which means
> their gdbarches are different as well.
>
> Given the differences between two gdbarches are small, we mostly get
> away with it. But if there are further differences (different hooks,
> for example), I fear we may run into a situation where we use an
> incorrect gdbarch to call a particular hook.

Indeed, good points! Thank you for bringing them up. I can address core
file dumping/loading after this series.

Regarding frames with different vg values, it's important to be aware of
this discrepancy but IMHO it makes sense to work on it when it becomes
a problem...
  

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

> On 2/3/23 03:47, Thiago Jung Bauermann wrote:
>> Simon Marchi <simark@simark.ca> writes:
>>> On 2/1/23 21:54, Thiago Jung Bauermann wrote:
>>>> Luis Machado <luis.machado@arm.com> writes:
>>>>> Moving towards thread-specific target descriptions/gdbarch would
>>>>> be a positive thing given the SVE precedent. The process-wide
>>>>> target description/gdbarch no longer reflects the correct settings
>>>>> for each thread on AArch64's with SVE support.
>>>>
>>>> In the first version of these patches I removed the process-wide target
>>>> description and moved it to thread_info, but it was a big patch that
>>>> touched many targets. I can bring it back if you think it's worth it.
>>>
>>> At least for the register description, if we decide it's now a
>>> per-thread thing, what does it mean to have a process-wide description
>>> anyway?  I think it would make sense to get rid of it, that could help
>>> confirm our model works (and it would remove the chance of  using the
>>> wrong tdesc by mistake for a thread that has its own tdesc).  It's
>>> probably something that can be done incrementally though.
>>
>> Would it be done incrementally by keeping process_info->tdesc but
>> changing each target in turn to use thread_info->tdesc and ignore the
>> process_info one?
>
> Given only AArch64 has this requirement at the moment, it may be
> easier to leave the process-wide tdesc up until the point we can
> safely/easily switch to thread-specific ones.
>
> After that, AArch64 will have potentially distinct thread tdescs while
> other architectures will have threads sharing the same tdesc.

Ok, sounds good. Thanks for the clarification!
  

On 2/4/23 15:21, Thiago Jung Bauermann wrote:
> 
> Luis Machado <luis.machado@arm.com> writes:
> 
>> On 2/2/23 03:47, Simon Marchi wrote:
>>> On 2/1/23 21:54, Thiago Jung Bauermann wrote:
>>>> In any case, it wouldn't be possible to make get_thread_target_desc just
>>>> return thread_info->tdesc because at least the way these patches are
>>>> currently written, when the inferior starts or a new thread of the
>>>> inferior is spawned thread_info->tdesc is nullptr. gdbserver will only
>>>> call get_thread_tdesc after the first stop (in get_thread_regcache, in
>>>> the process of obtaining the pc register), so we will need to cope with
>>>> that situation.
>>> Ok.  Would it work if a new thread initially inherited the tdesc from
>>> its process?
>>>
>>
>> It should be fine because the first time we fetch a process target
>> description, it is eventually obtained from the first and only thread.
>> So the SVE vector length should be correct.
>>
>> Any subsequent attempts to use the process' target description (the
>> first one we obtained), after further stops, may end up using an
>> incorrect description.
>>
>> I think this is handled correctly by the target architecture target
>> hook though. But there are other places where this is potentially
>> incorrect.
>>
>> For example...
>>
>> - When using gcore to dump a core file, GDB only dumps a single target
>> description. While this might be correct for a target with a fixed
>> target description or a AArch64 target that doesn't support SVE, it
>> likely won't be correctly for one AArch64 target supporting SVE if its
>> threads changed vector length mid-execution. Either we emit target
>> description notes by thread, or we don't emit a target description
>> note for those cases.
>>
>> - When loading the above/older gcore core files back, GDB will use a
>> potentially incorrect target description. If we decide to emit
>> per-thread target descriptions, it should be fine. Otherwise we may
>> need to have a "thread architecture" hook for core files as well.
>>
>> - The remote has no concept of a thread architecture (Thiago is
>> addressing this with this patch series).
>>
>> - AArch64 frames may have slightly different vg values, which means
>> their gdbarches are different as well.
>>
>> Given the differences between two gdbarches are small, we mostly get
>> away with it. But if there are further differences (different hooks,
>> for example), I fear we may run into a situation where we use an
>> incorrect gdbarch to call a particular hook.
> 
> Indeed, good points! Thank you for bringing them up. I can address core
> file dumping/loading after this series.

I have an upcoming patch for SME that should address this for core files, but it still needs some testing.

> 
> Regarding frames with different vg values, it's important to be aware of
> this discrepancy but IMHO it makes sense to work on it when it becomes
> a problem...
> 

Indeed. It will be a problem for SME and streaming mode, but I have another upcoming patch to hopefully address this as well.
  

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

> On 2/4/23 15:21, Thiago Jung Bauermann wrote:
>> Luis Machado <luis.machado@arm.com> writes:
>>
>>> On 2/2/23 03:47, Simon Marchi wrote:
>>>> Ok.  Would it work if a new thread initially inherited the tdesc from
>>>> its process?
>>>>
>>>
>>> It should be fine because the first time we fetch a process target
>>> description, it is eventually obtained from the first and only thread.
>>> So the SVE vector length should be correct.
>>>
>>> Any subsequent attempts to use the process' target description (the
>>> first one we obtained), after further stops, may end up using an
>>> incorrect description.
>>>
>>> I think this is handled correctly by the target architecture target
>>> hook though. But there are other places where this is potentially
>>> incorrect.
>>>
>>> For example...
>>>
>>> - When using gcore to dump a core file, GDB only dumps a single target
>>> description. While this might be correct for a target with a fixed
>>> target description or a AArch64 target that doesn't support SVE, it
>>> likely won't be correctly for one AArch64 target supporting SVE if its
>>> threads changed vector length mid-execution. Either we emit target
>>> description notes by thread, or we don't emit a target description
>>> note for those cases.
>>>
>>> - When loading the above/older gcore core files back, GDB will use a
>>> potentially incorrect target description. If we decide to emit
>>> per-thread target descriptions, it should be fine. Otherwise we may
>>> need to have a "thread architecture" hook for core files as well.
>>>
>>> - The remote has no concept of a thread architecture (Thiago is
>>> addressing this with this patch series).
>>>
>>> - AArch64 frames may have slightly different vg values, which means
>>> their gdbarches are different as well.
>>>
>>> Given the differences between two gdbarches are small, we mostly get
>>> away with it. But if there are further differences (different hooks,
>>> for example), I fear we may run into a situation where we use an
>>> incorrect gdbarch to call a particular hook.
>> Indeed, good points! Thank you for bringing them up. I can address core
>> file dumping/loading after this series.
>
> I have an upcoming patch for SME that should address this for core
> files, but it still needs some testing.
>
>> Regarding frames with different vg values, it's important to be aware of
>> this discrepancy but IMHO it makes sense to work on it when it becomes
>> a problem...
>>
>
> Indeed. It will be a problem for SME and streaming mode, but I have
> another upcoming patch to hopefully address this as well.

Nice! Thank you.
  

On 2023-02-02 2:54 a.m., Thiago Jung Bauermann via Gdb-patches wrote:
> 
> Luis Machado <luis.machado@arm.com> writes:
> 
>> On 2/1/23 16:21, Simon Marchi wrote:
>>>
>>>>> diff --git a/gdbserver/linux-low.h b/gdbserver/linux-low.h
>>>>> index 221de85aa2ee..b52eb23cc444 100644
>>>>> --- a/gdbserver/linux-low.h
>>>>> +++ b/gdbserver/linux-low.h
>>>>> @@ -604,6 +604,12 @@ 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 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);
>>>>
>>>> I think the comment for this function is not correct.  The function does
>>>> not SET the thread's target description, but just GETS a target
>>>> description suitable for `thread`.  It's the caller's job to do the
>>>> setting.
>>> This comment also gave me pause.  How does a getter set something.  I
>>> then understood that it allowed the arch-specific code to provide a
>>> thread-specific tdesc.  I would suggest just:
>>
>> FWIW, I read it as "the functions *allows* arch-specific code to set".
>> So it doesn't set on its own, but it does allow something else to do
>> it.
> 
> Yes, that's what was in my mind when I wrote the comment. But I agree
> it's unclear, and I adopted Simon's suggested version.
> 
>>> The other thought I had while re-reading the patch is why do we need to
>>> return and store nullptr if the thread target description is the same as
>>> the main one for the process.  get_thread_tdesc could just return
>>> process_info->tdesc if we don't need a separate tdesc, and we would
>>> store that same pointer in thread_info->tdesc.
> 
> We don't need to return and store nullptr if the thread target
> description is the same as the main one for the process. Things will
> work fine if we do as you suggest. IIRC my private branch worked liked
> that for a while, before I changed it to the current version.
> 
> I changed it because I thought it was a clearer mental model if
> thread_info->tdesc is nullptr when there's not thread-specific target
> description. I can make the get_thread_tdesc method always return a
> valid target description if you think it's better that way.
> 
>>> And get_thread_tdesc would just return that (in fact,
>>> get_thread_tdesc might not be necessary then). Perhaps it makes some
>>> things more complicated down the road, but I can't think of anything.
> 
> Sorry, I don't understand this part. get_thread_tdesc is necessary
> because it's the hook that allows arch-specific code to provide a target
> description for the thread. I don't see how it can become unnecessary.
> 
> Perhaps you mean the get_thread_target_desc function? Sorry about the
> names being so similar, I spent some time trying to think of a better
> name for either the method or the function but failed.

Please drop the "get_" prefix from the class method, it doesn't really
add value, and we typically don't add it.  Most GDB getter/setters are
in  foo() / set_foo()  pair style, rather than get_foo() / set_foo().

A "get_" prefix is however typically used for global getter functions.

FWIW, I have the same thought as Simon while reading this.

> 
> In any case, it wouldn't be possible to make get_thread_target_desc just
> return thread_info->tdesc because at least the way these patches are
> currently written, when the inferior starts or a new thread of the
> inferior is spawned thread_info->tdesc is nullptr. gdbserver will only
> call get_thread_tdesc after the first stop (in get_thread_regcache, in
> the process of obtaining the pc register), so we will need to cope with
> that situation.
> 
>> Sounds reasonable.
>>
>> Moving towards thread-specific target descriptions/gdbarch would be a positive thing given
>> the SVE precedent. The process-wide target description/gdbarch no
>> longer reflects the correct settings for each thread on AArch64's with SVE support.
> 
> In the first version of these patches I removed the process-wide target
> description and moved it to thread_info, but it was a big patch that
> touched many targets. I can bring it back if you think it's worth it.
> 
>
  

On 2023-02-04 3:21 p.m., Thiago Jung Bauermann via Gdb-patches wrote:
> 
> Luis Machado <luis.machado@arm.com> writes:
> 
>> On 2/2/23 03:47, Simon Marchi wrote:
>>> On 2/1/23 21:54, Thiago Jung Bauermann wrote:
>>>> In any case, it wouldn't be possible to make get_thread_target_desc just
>>>> return thread_info->tdesc because at least the way these patches are
>>>> currently written, when the inferior starts or a new thread of the
>>>> inferior is spawned thread_info->tdesc is nullptr. gdbserver will only
>>>> call get_thread_tdesc after the first stop (in get_thread_regcache, in
>>>> the process of obtaining the pc register), so we will need to cope with
>>>> that situation.
>>> Ok.  Would it work if a new thread initially inherited the tdesc from
>>> its process?
>>>
>>
>> It should be fine because the first time we fetch a process target
>> description, it is eventually obtained from the first and only thread.
>> So the SVE vector length should be correct.
>>
>> Any subsequent attempts to use the process' target description (the
>> first one we obtained), after further stops, may end up using an
>> incorrect description.
>>
>> I think this is handled correctly by the target architecture target
>> hook though. But there are other places where this is potentially
>> incorrect.
>>
>> For example...
>>
>> - When using gcore to dump a core file, GDB only dumps a single target
>> description. While this might be correct for a target with a fixed
>> target description or a AArch64 target that doesn't support SVE, it
>> likely won't be correctly for one AArch64 target supporting SVE if its
>> threads changed vector length mid-execution. Either we emit target
>> description notes by thread, or we don't emit a target description
>> note for those cases.
>>
>> - When loading the above/older gcore core files back, GDB will use a
>> potentially incorrect target description. If we decide to emit
>> per-thread target descriptions, it should be fine. Otherwise we may
>> need to have a "thread architecture" hook for core files as well.
>>
>> - The remote has no concept of a thread architecture (Thiago is
>> addressing this with this patch series).
>>
>> - AArch64 frames may have slightly different vg values, which means
>> their gdbarches are different as well.
>>
>> Given the differences between two gdbarches are small, we mostly get
>> away with it. But if there are further differences (different hooks,
>> for example), I fear we may run into a situation where we use an
>> incorrect gdbarch to call a particular hook.
> 
> Indeed, good points! Thank you for bringing them up. I can address core
> file dumping/loading after this series.
> 
> Regarding frames with different vg values, it's important to be aware of
> this discrepancy but IMHO it makes sense to work on it when it becomes
> a problem...
> 


Yeah, one thought that keeps crossing my mind is if really modeling all this
stuff as different target descriptions is really the best approach.  The Intel AMX
support posted on the list last year also ran into a similar problem, with the 
matrix registers height/width changing at runtime, and it is impractical (or really,
it really smells like the wrong approach) to have different target descriptions for
every potential matrix size.  Which is not unlike different SVE sizes.  It feels like
a single tdesc should be able to be a bit more dynamic.  It's a bit funny that ptrace
manages to work with a single registers interface, while we don't.

What if we extended the target description mechanism so that a single description
could describe all SVE sizes?  For example, what if a tdesc could describe the SVE
width/length as a dynamic property, retrieved from elsewhere, e.g., from another register?

BTW, for core files, where are we going to retrieve the SVE length from?
  

On 2/6/23 20:29, Pedro Alves wrote:
> On 2023-02-04 3:21 p.m., Thiago Jung Bauermann via Gdb-patches wrote:
>>
>> Luis Machado <luis.machado@arm.com> writes:
>>
>>> On 2/2/23 03:47, Simon Marchi wrote:
>>>> On 2/1/23 21:54, Thiago Jung Bauermann wrote:
>>>>> In any case, it wouldn't be possible to make get_thread_target_desc just
>>>>> return thread_info->tdesc because at least the way these patches are
>>>>> currently written, when the inferior starts or a new thread of the
>>>>> inferior is spawned thread_info->tdesc is nullptr. gdbserver will only
>>>>> call get_thread_tdesc after the first stop (in get_thread_regcache, in
>>>>> the process of obtaining the pc register), so we will need to cope with
>>>>> that situation.
>>>> Ok.  Would it work if a new thread initially inherited the tdesc from
>>>> its process?
>>>>
>>>
>>> It should be fine because the first time we fetch a process target
>>> description, it is eventually obtained from the first and only thread.
>>> So the SVE vector length should be correct.
>>>
>>> Any subsequent attempts to use the process' target description (the
>>> first one we obtained), after further stops, may end up using an
>>> incorrect description.
>>>
>>> I think this is handled correctly by the target architecture target
>>> hook though. But there are other places where this is potentially
>>> incorrect.
>>>
>>> For example...
>>>
>>> - When using gcore to dump a core file, GDB only dumps a single target
>>> description. While this might be correct for a target with a fixed
>>> target description or a AArch64 target that doesn't support SVE, it
>>> likely won't be correctly for one AArch64 target supporting SVE if its
>>> threads changed vector length mid-execution. Either we emit target
>>> description notes by thread, or we don't emit a target description
>>> note for those cases.
>>>
>>> - When loading the above/older gcore core files back, GDB will use a
>>> potentially incorrect target description. If we decide to emit
>>> per-thread target descriptions, it should be fine. Otherwise we may
>>> need to have a "thread architecture" hook for core files as well.
>>>
>>> - The remote has no concept of a thread architecture (Thiago is
>>> addressing this with this patch series).
>>>
>>> - AArch64 frames may have slightly different vg values, which means
>>> their gdbarches are different as well.
>>>
>>> Given the differences between two gdbarches are small, we mostly get
>>> away with it. But if there are further differences (different hooks,
>>> for example), I fear we may run into a situation where we use an
>>> incorrect gdbarch to call a particular hook.
>>
>> Indeed, good points! Thank you for bringing them up. I can address core
>> file dumping/loading after this series.
>>
>> Regarding frames with different vg values, it's important to be aware of
>> this discrepancy but IMHO it makes sense to work on it when it becomes
>> a problem...
>>
> 
> 
> Yeah, one thought that keeps crossing my mind is if really modeling all this
> stuff as different target descriptions is really the best approach.  The Intel AMX
> support posted on the list last year also ran into a similar problem, with the
> matrix registers height/width changing at runtime, and it is impractical (or really,
> it really smells like the wrong approach) to have different target descriptions for
> every potential matrix size.  Which is not unlike different SVE sizes.  It feels like
> a single tdesc should be able to be a bit more dynamic.  It's a bit funny that ptrace
> manages to work with a single registers interface, while we don't.

My understanding is that this was a bit hard to justify back when SVE was implemented, as it would
have to touch some other bits of the type system to create a sizeless type for SVE vectors, where
the size would be determined by context.

> 
> What if we extended the target description mechanism so that a single description
> could describe all SVE sizes?  For example, what if a tdesc could describe the SVE
> width/length as a dynamic property, retrieved from elsewhere, e.g., from another register?
> 
> BTW, for core files, where are we going to retrieve the SVE length from?

The thread-specific vector length should be available from the SVE register dump sections.
  

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

> On 2/6/23 20:29, Pedro Alves wrote:
>> On 2023-02-04 3:21 p.m., Thiago Jung Bauermann via Gdb-patches wrote:
>>>
>>> Luis Machado <luis.machado@arm.com> writes:
>>>
>>>> On 2/2/23 03:47, Simon Marchi wrote:
>>>>> Ok.  Would it work if a new thread initially inherited the tdesc from
>>>>> its process?
>>>>>
>>>>
>>>> It should be fine because the first time we fetch a process target
>>>> description, it is eventually obtained from the first and only thread.
>>>> So the SVE vector length should be correct.
>>>>
>>>> Any subsequent attempts to use the process' target description (the
>>>> first one we obtained), after further stops, may end up using an
>>>> incorrect description.
>>>>
>>>> I think this is handled correctly by the target architecture target
>>>> hook though. But there are other places where this is potentially
>>>> incorrect.
>>>>
>>>> For example...
>>>>
>>>> - When using gcore to dump a core file, GDB only dumps a single target
>>>> description. While this might be correct for a target with a fixed
>>>> target description or a AArch64 target that doesn't support SVE, it
>>>> likely won't be correctly for one AArch64 target supporting SVE if its
>>>> threads changed vector length mid-execution. Either we emit target
>>>> description notes by thread, or we don't emit a target description
>>>> note for those cases.
>>>>
>>>> - When loading the above/older gcore core files back, GDB will use a
>>>> potentially incorrect target description. If we decide to emit
>>>> per-thread target descriptions, it should be fine. Otherwise we may
>>>> need to have a "thread architecture" hook for core files as well.
>>>>
>>>> - The remote has no concept of a thread architecture (Thiago is
>>>> addressing this with this patch series).
>>>>
>>>> - AArch64 frames may have slightly different vg values, which means
>>>> their gdbarches are different as well.
>>>>
>>>> Given the differences between two gdbarches are small, we mostly get
>>>> away with it. But if there are further differences (different hooks,
>>>> for example), I fear we may run into a situation where we use an
>>>> incorrect gdbarch to call a particular hook.
>>>
>>> Indeed, good points! Thank you for bringing them up. I can address core
>>> file dumping/loading after this series.
>>>
>>> Regarding frames with different vg values, it's important to be aware of
>>> this discrepancy but IMHO it makes sense to work on it when it becomes
>>> a problem...
>>>
>> Yeah, one thought that keeps crossing my mind is if really modeling
>> all this stuff as different target descriptions is really the best
>> approach. The Intel AMX support posted on the list last year also ran
>> into a similar problem, with the matrix registers height/width
>> changing at runtime, and it is impractical (or really, it really
>> smells like the wrong approach) to have different target descriptions
>> for every potential matrix size. Which is not unlike different SVE
>> sizes. It feels like a single tdesc should be able to be a bit more
>> dynamic. It's a bit funny that ptrace manages to work with a single
>> registers interface, while we don't.
>
> My understanding is that this was a bit hard to justify back when SVE
> was implemented, as it would have to touch some other bits of the type
> system to create a sizeless type for SVE vectors, where the size would
> be determined by context.

When I started working on these patches I read some older discussions on
the topic of SVE in the mailing list archives and I saw this idea.

So before going down the per-thread target description route I spent
some time trying to figure out a way to make the vector register size a
function of the contents of another register (a pseudo-register, in the
case of SVE), but I failed to see how I could do it.

I'm open to giving it another try but I would need some guidance. It
doesn't need to be detailed instructions, but a high level idea of how
it can be done would be very helpful.
  

Pedro Alves <pedro@palves.net> writes:

> On 2023-02-02 2:54 a.m., Thiago Jung Bauermann via Gdb-patches wrote:
>> 
>> Luis Machado <luis.machado@arm.com> writes:
>> 
>>> On 2/1/23 16:21, Simon Marchi wrote:
>>>>
>>>>>> diff --git a/gdbserver/linux-low.h b/gdbserver/linux-low.h
>>>>>> index 221de85aa2ee..b52eb23cc444 100644
>>>>>> --- a/gdbserver/linux-low.h
>>>>>> +++ b/gdbserver/linux-low.h
>>>>>> @@ -604,6 +604,12 @@ 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 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);
>>>>>
>>>>> I think the comment for this function is not correct.  The function does
>>>>> not SET the thread's target description, but just GETS a target
>>>>> description suitable for `thread`.  It's the caller's job to do the
>>>>> setting.
>>>> This comment also gave me pause.  How does a getter set something.  I
>>>> then understood that it allowed the arch-specific code to provide a
>>>> thread-specific tdesc.  I would suggest just:
>>>
>>> FWIW, I read it as "the functions *allows* arch-specific code to set".
>>> So it doesn't set on its own, but it does allow something else to do
>>> it.
>> 
>> Yes, that's what was in my mind when I wrote the comment. But I agree
>> it's unclear, and I adopted Simon's suggested version.
>> 
>>>> The other thought I had while re-reading the patch is why do we need to
>>>> return and store nullptr if the thread target description is the same as
>>>> the main one for the process.  get_thread_tdesc could just return
>>>> process_info->tdesc if we don't need a separate tdesc, and we would
>>>> store that same pointer in thread_info->tdesc.
>> 
>> We don't need to return and store nullptr if the thread target
>> description is the same as the main one for the process. Things will
>> work fine if we do as you suggest. IIRC my private branch worked liked
>> that for a while, before I changed it to the current version.
>> 
>> I changed it because I thought it was a clearer mental model if
>> thread_info->tdesc is nullptr when there's not thread-specific target
>> description. I can make the get_thread_tdesc method always return a
>> valid target description if you think it's better that way.
>> 
>>>> And get_thread_tdesc would just return that (in fact,
>>>> get_thread_tdesc might not be necessary then). Perhaps it makes some
>>>> things more complicated down the road, but I can't think of anything.
>> 
>> Sorry, I don't understand this part. get_thread_tdesc is necessary
>> because it's the hook that allows arch-specific code to provide a target
>> description for the thread. I don't see how it can become unnecessary.
>> 
>> Perhaps you mean the get_thread_target_desc function? Sorry about the
>> names being so similar, I spent some time trying to think of a better
>> name for either the method or the function but failed.
>
> Please drop the "get_" prefix from the class method, it doesn't really
> add value, and we typically don't add it.  Most GDB getter/setters are
> in  foo() / set_foo()  pair style, rather than get_foo() / set_foo().
>
> A "get_" prefix is however typically used for global getter functions.

Ok, I will drop the prefix. Thank you for explaining the pattern.
I wasn't aware of it.

> FWIW, I have the same thought as Simon while reading this.

Ok, I'll change the code to always store a tdesc in thread_info even if
it's the same as the process-wide one. If both you and Simon thought
along the same lines it may be a more intuitive mental model than the
one I was considering.

That is, assuming we continue with the thread-specific tdesc approach
rather than the one which expands tdescs to allow describing one
register's type in terms of another one. I'll revisit my notes and think
more about this.
  

> That is, assuming we continue with the thread-specific tdesc approach
> rather than the one which expands tdescs to allow describing one
> register's type in terms of another one. I'll revisit my notes and think
> more about this.

Can we expand about this idea?  I think I like it, but I don't see 100%
how it would work.  I can imagine a vector of registers whose size
depends directly on the value of some other register that comes before,
like:

  <vector id="vec" type="some_type" count="some_other_register"/>

Here, "some_other_register" would be a scalar register that comes before
"vec", and whose value dictates directly the number of elements of
"vec".  But if you wanted to say that the number of elements in "vec" is
the value of some_other_register, times 2?  I guess we could write:

  <vector id="vec" type="some_type" count="some_other_register * 2"/>

.. but then we get in the realm of defining a grammar, building a
parser / evaluator, etc.

The type of the vector elements needs to be dynamic too, how do
we define that?

If the number of possibilities is known and static, we could have some
kind of "variant" type, where we list all the possible types, and select
among them at runtime based on the value of a preceding register.

If I understand correctly, all of this makes it so the size of the
response to the g packet will be dynamic too?

Simon
  

Simon Marchi <simark@simark.ca> writes:

>> That is, assuming we continue with the thread-specific tdesc approach
>> rather than the one which expands tdescs to allow describing one
>> register's type in terms of another one. I'll revisit my notes and think
>> more about this.
>
> Can we expand about this idea?  I think I like it, but I don't see 100%
> how it would work.

Sorry, I can't expand much on it. I like it too, and as I mentioned in
another email I spent some time investigating how it could be done but
I wasn't able to make progress so I decided to do the per-thread tdesc
implementation instead.

I would be willing to try again but I would need help with high-level
design.

> I can imagine a vector of registers whose size
> depends directly on the value of some other register that comes before,
> like:
>
>   <vector id="vec" type="some_type" count="some_other_register"/>
>
> Here, "some_other_register" would be a scalar register that comes before
> "vec", and whose value dictates directly the number of elements of
> "vec".  But if you wanted to say that the number of elements in "vec" is
> the value of some_other_register, times 2?  I guess we could write:
>
>   <vector id="vec" type="some_type" count="some_other_register * 2"/>
>
> .. but then we get in the realm of defining a grammar, building a
> parser / evaluator, etc.

We could rein complexity in by supporting only the simplest of
expressions, e.g. only a very rigid form such as “<operand 1> <op>
<operand 2>” where <op> is one of the basic arithmetic operations.
If that turns out to not be enough then we can increasingly support more
complex operations.

> The type of the vector elements needs to be dynamic too, how do
> we define that?

This is the part where I got stuck, especially on how to make GDB's type
system allow expressing such a type.

> If the number of possibilities is known and static, we could have some
> kind of "variant" type, where we list all the possible types, and select
> among them at runtime based on the value of a preceding register.

Yes, in the case of SVE it's known and static. The maximum vector length
is an architectural feature of the processor, and GDB/gdbserver can get
it via ptrace in the NT_ARM_SVE regset. And it's always a multiple of
16.

It's an interesting idea. Perhaps it's enough, at least for SVE?

> If I understand correctly, all of this makes it so the size of the
> response to the g packet will be dynamic too?

We /could/ set the size of the g packet to always correspond to the
largest vector length possible but it would be a big overhead,
especially if there are many threads involved.

So in practice yes, it will be dynamic if we can help it.
  

On 2/10/23 03:29, Thiago Jung Bauermann wrote:
> 
> Simon Marchi <simark@simark.ca> writes:
> 
>>> That is, assuming we continue with the thread-specific tdesc approach
>>> rather than the one which expands tdescs to allow describing one
>>> register's type in terms of another one. I'll revisit my notes and think
>>> more about this.
>>
>> Can we expand about this idea?  I think I like it, but I don't see 100%
>> how it would work.
> 
> Sorry, I can't expand much on it. I like it too, and as I mentioned in
> another email I spent some time investigating how it could be done but
> I wasn't able to make progress so I decided to do the per-thread tdesc
> implementation instead.
> 
> I would be willing to try again but I would need help with high-level
> design.
> 

My take on it is that it is he appropriate solution, and it would allow us to have a single target description for all the threads.

But it is also a larger effort that has to revamp some things in gdb's type system to allow for sizeless types, and that also has
implications in other areas of gdb.

Also, quite a bit of effort was put into supporting dynamic vector lengths mid-execution for SVE in native gdb, including
some new target hooks to adjust the architecture and the register cache format.

Changing this also means having to support the debugging stubs out there that already support SVE target descriptions. One can say
those stubs are not fully functional in terms of supporting dynamic vector lengths mid-execution, but they already produce target descriptions
in the current format (gdbserver is one of those stubs and QEMU is probably the second most important).

I'd be happy with an intermediate solution like what Thiago put together. It would solve a long-standing issue for SVE and gdbserver and it seems simpler at this point,
plus Thiago already put the effort to write the code.

>> I can imagine a vector of registers whose size
>> depends directly on the value of some other register that comes before,
>> like:
>>
>>    <vector id="vec" type="some_type" count="some_other_register"/>
>>
>> Here, "some_other_register" would be a scalar register that comes before
>> "vec", and whose value dictates directly the number of elements of
>> "vec".  But if you wanted to say that the number of elements in "vec" is
>> the value of some_other_register, times 2?  I guess we could write:
>>
>>    <vector id="vec" type="some_type" count="some_other_register * 2"/>
>>
>> .. but then we get in the realm of defining a grammar, building a
>> parser / evaluator, etc.
> 
> We could rein complexity in by supporting only the simplest of
> expressions, e.g. only a very rigid form such as “<operand 1> <op>
> <operand 2>” where <op> is one of the basic arithmetic operations.
> If that turns out to not be enough then we can increasingly support more
> complex operations.
> 
>> The type of the vector elements needs to be dynamic too, how do
>> we define that?
> 
> This is the part where I got stuck, especially on how to make GDB's type
> system allow expressing such a type.
> 
>> If the number of possibilities is known and static, we could have some
>> kind of "variant" type, where we list all the possible types, and select
>> among them at runtime based on the value of a preceding register.
> 
> Yes, in the case of SVE it's known and static. The maximum vector length
> is an architectural feature of the processor, and GDB/gdbserver can get
> it via ptrace in the NT_ARM_SVE regset. And it's always a multiple of
> 16.
> 

SME is a bit more strict with the allowed vector lengths. It is always a power of 2 between 128 and 2048 bits inclusive.

So in practice 128/256/512/1024/2048 bits.

> It's an interesting idea. Perhaps it's enough, at least for SVE?
> 
>> If I understand correctly, all of this makes it so the size of the
>> response to the g packet will be dynamic too?
> 
> We /could/ set the size of the g packet to always correspond to the
> largest vector length possible but it would be a big overhead,
> especially if there are many threads involved.

Or we could use the opportunity to break free from the g/G packet restrictions and go for a more flexible format while at it.

Given most of the fields contained in these big registers is 0 or same value, there is potential for quite a bit of compression.

Just an idea, while we're throwing ideas out there.

> 
> So in practice yes, it will be dynamic if we can help it.
>
  

Luis> But it is also a larger effort that has to revamp some things in
Luis> gdb's type system to allow for sizeless types, and that also has
Luis> implications in other areas of gdb.

I doubt I understand the problem here, but we do have dynamic types and
dynamic type resolution now.  This lets you express a type's size,
offsets of members, and even fields (via variant parts) by referring to
other members of the object.

Luis> I'd be happy with an intermediate solution like what Thiago put
Luis> together. It would solve a long-standing issue for SVE and
Luis> gdbserver and it seems simpler at this point, plus Thiago already
Luis> put the effort to write the code.

I haven't followed this discussion, but with the remote protocol,
whatever we do now will be baked in forever.  So, it's worth spending
extra time up front to get a really solid approach.  (I'm not saying
this isn't, just pointing out that there's a long-term cost.)

Tom
  

On 2/10/23 15:04, Tom Tromey wrote:
> Luis> But it is also a larger effort that has to revamp some things in
> Luis> gdb's type system to allow for sizeless types, and that also has
> Luis> implications in other areas of gdb.
> 
> I doubt I understand the problem here, but we do have dynamic types and
> dynamic type resolution now.  This lets you express a type's size,
> offsets of members, and even fields (via variant parts) by referring to
> other members of the object.

In summary, we need to define a vector register with size equal to the value of a particular
register (vg).

With every distinct value of that register, you'll have a distinct size in the register buffer, core
files, remote packet etc.

We do have dynamic types, but I don't particularly remember having a type that has a size defined by some
other entity outside of the type system, like a register (or dwarf register).

> 
> Luis> I'd be happy with an intermediate solution like what Thiago put
> Luis> together. It would solve a long-standing issue for SVE and
> Luis> gdbserver and it seems simpler at this point, plus Thiago already
> Luis> put the effort to write the code.
> 
> I haven't followed this discussion, but with the remote protocol,
> whatever we do now will be baked in forever.  So, it's worth spending
> extra time up front to get a really solid approach.  (I'm not saying
> this isn't, just pointing out that there's a long-term cost.)

Sure, but we can always add feature checks for that and fallback to known RSP if a particular feature isn't supported.

The approach Thiago took was, at one point, deemed the appropriate way to get dynamic vector lengths working in gdbserver. So some
amount of thought was put into it already.

I think there are two separate issues here. Per-thread target descriptions/gdbarch and the problem with dynamic vector lengths.

Realistically, having per-thread target descriptions/gdbarch seems like a more relevant feature at this point. It would allow
threads to have different architectures, and I think that is a good addition going forwards. Cell BE made use of this, but the
port has been removed now. It might also be useful for heterogeneous debugging, where you have cores of different architectures.

A separate issue is the one with the SVE dynamic vector length. We did have a chance to make gdb cope with sizeless types when SVE was
initially put together. But at the time it was deemed best to do (potentially) smaller adjustments to make it work for the native case. As a result,
the code we have now handles these changes via per-thread target descriptions.

I suppose AMX has the same issue, but I'm not sure how it was implemented. I remember reading about using a fixed-size buffer, but I haven't
followed that discussion closely.

So if we have other ports that could benefit from such a change to better handle types defined based on external entities (registers, variables etc), I guess
it would make sense to explore that possibility. Otherwise, if we're only trying to solve the particular case of SVE, coming up with a completely new design
might be a bit too much.

Just my $0.02.

> 
> Tom
  

Tom Tromey <tom@tromey.com> writes:

> Luis> I'd be happy with an intermediate solution like what Thiago put
> Luis> together. It would solve a long-standing issue for SVE and
> Luis> gdbserver and it seems simpler at this point, plus Thiago already
> Luis> put the effort to write the code.
>
> I haven't followed this discussion, but with the remote protocol,
> whatever we do now will be baked in forever.  So, it's worth spending
> extra time up front to get a really solid approach.  (I'm not saying
> this isn't, just pointing out that there's a long-term cost.)

Andrew recommended adding a new feature to the qSupported packet so it
would be an optional extension of the remote protocol — not really baked
into it but rather a complementary muffin.

Per-thread target descriptions are also a very generic feature that
I can see being useful for heterogeneous computers, though I don't know
if there's any actual interest in that in the context of GDB and the
remote protocol.
  

On 2/10/23 12:26, Thiago Jung Bauermann via Gdb-patches wrote:
> 
> Tom Tromey <tom@tromey.com> writes:
> 
>> Luis> I'd be happy with an intermediate solution like what Thiago put
>> Luis> together. It would solve a long-standing issue for SVE and
>> Luis> gdbserver and it seems simpler at this point, plus Thiago already
>> Luis> put the effort to write the code.
>>
>> I haven't followed this discussion, but with the remote protocol,
>> whatever we do now will be baked in forever.  So, it's worth spending
>> extra time up front to get a really solid approach.  (I'm not saying
>> this isn't, just pointing out that there's a long-term cost.)
> 
> Andrew recommended adding a new feature to the qSupported packet so it
> would be an optional extension of the remote protocol — not really baked
> into it but rather a complementary muffin.
> 
> Per-thread target descriptions are also a very generic feature that
> I can see being useful for heterogeneous computers, though I don't know
> if there's any actual interest in that in the context of GDB and the
> remote protocol.

Maybe Pedro will correct me but... for ROCm, it's possible we'll want to
support remote debugging in the future.  And our model is a mix of host
and GPU threads in a single inferior.  So I can see this feature being
useful for that.

Simon