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

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

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

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

Good idea. For v3 I added:

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

>>   };
>>     extern std::list<thread_info *> all_threads;
>> diff --git a/gdbserver/linux-aarch64-low.cc b/gdbserver/linux-aarch64-low.cc
>> index cab4fc0a4674..786ce4071279 100644
>> --- a/gdbserver/linux-aarch64-low.cc
>> +++ b/gdbserver/linux-aarch64-low.cc
>> @@ -99,6 +99,9 @@ protected:
>>       void low_arch_setup () override;
>>   +  gdb::optional<const struct target_desc *>
>> +    arch_update_tdesc (const thread_info *thread) override;
>> +
>>     bool low_cannot_fetch_register (int regno) override;
>>       bool low_cannot_store_register (int regno) override;
>> @@ -184,6 +187,8 @@ struct arch_process_info
>>        same for each thread, it is reasonable for the data to live here.
>>        */
>>     struct aarch64_debug_reg_state debug_reg_state;
>> +
>> +  bool has_sve;
>>   };
>
> Though obvious, adding a comment like "has_sve" in gdb/aarch64-tdep.h will clarify the use
> of this field.

Indeed. For v3 I added:

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

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

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

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

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

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

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

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

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

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

-- 
Thiago