Re: [PATCH 5/8] gdbserver: Move target description from being per-process to being per-thread

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

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

> On 9/8/22 07:41, Thiago Jung Bauermann via Gdb-patches wrote:
>> --- 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 keep a process-wide target description in case we fail to
> detect the target description for a given thread?

That is a great idea. I implemented it in v2, with the difference that
we never fail to detect the target description for a thread. What can
happen is that we fail to detect SVE, and in that case it is assumed to
be unsupported.

The thread-specific target description is used only for inferiors where
we detect SVE, and the process-wide one used for all the others.

> Also, it may simplify some of the changes in this patch.

Indeed, it simplified many of the changes in this patch and the others.
Thank you for the suggestion!

>> --- a/gdbserver/linux-aarch64-low.cc
>> +++ b/gdbserver/linux-aarch64-low.cc
>> @@ -191,9 +191,26 @@ struct arch_process_info
>>   static int
>>   is_64bit_tdesc (void)
>>   {
>> +  const target_desc *tdesc;
>> +
>> +  if (current_thread && current_thread->tdesc)
>> +    tdesc = current_thread->tdesc;
>> +  else
>> +    {
>> +      /* Find some thread that has a target description.  */
>> +      const struct thread_info *thread
>> +	  = find_thread (current_process ()->pid, [] (thread_info *thr) {
>> +	      return thr->tdesc != nullptr;
>> +	    });
>> +
>> +      gdb_assert (thread != nullptr);
>> +
>> +      tdesc = thread->tdesc;
>> +    }
>> +
>>     /* We may not have a current thread at this point, so go straight to
>>        the process's target description.  */
>> -  return register_size (current_process ()->tdesc, 0) == 8;
>> +  return register_size (tdesc, 0) == 8;
>>   }
>
> Keeping the process-wide target description would simplify this
> function in my opinion. We won't have 64-bit processes spawning 32-bit
> threads.

You are right. This patch series doesn't change this function anymore.

>> @@ -678,6 +695,31 @@ void
>>   aarch64_target::low_new_thread (lwp_info *lwp)
>>   {
>>     aarch64_linux_new_thread (lwp);
>> +
>> +  ptid_t ptid = ptid_of_lwp (lwp);
>> +  process_info *proc = find_process_pid (ptid.pid ());
>> +
>> +  /* If the inferior is still going through the shell or the thread isn't
>> +     stopped we can't read its registers in order to read the target
>> +     description.  */
>> +  if (proc->starting_up)
>> +    return;
>> +
>> +  thread_info *thread = get_lwp_thread (lwp);
>> +  unsigned int machine;
>> +  gdb_assert (ptid.lwp_p ());
>> +  bool is_elf64 = linux_pid_exe_is_elf_64_file (ptid.lwp (), &machine);
>> +
>> +  if (is_elf64)
>> +    {
>> +      gdb::optional<struct aarch64_features> features
>> +	  = aarch64_get_arch_features (thread);
>> +
>> +      if (features.has_value())
>> +	thread->tdesc = aarch64_linux_read_description (*features);
>> +    }
>> +  else
>> +    thread->tdesc = aarch32_linux_read_description ();
>>   }
>>     void
>> @@ -848,14 +890,14 @@ aarch64_target::low_arch_setup ()
>>   	 succeeds so all we can do here is assert that features is valid.  */
>>         gdb_assert (features.has_value ());
>>   -      current_process ()->tdesc = aarch64_linux_read_description (*features);
>> +      current_thread->tdesc = aarch64_linux_read_description (*features);
>>           /* Adjust the register sets we should use for this particular set of
>>   	 features.  */
>>         aarch64_adjust_register_sets (*features);
>>       }
>>     else
>> -    current_process ()->tdesc = aarch32_linux_read_description ();
>> +    current_thread->tdesc = aarch32_linux_read_description ();
>
> Given 32-bit Arm processes will have the same target description
> throughout their lives, would it make sense to have a single target
> description stored process-wide, and whenever we need the description
> we can find it there?

Yes, you are right. In v2 low_arch_setup keeps 32-bit Arm the same as
before, with only process-wide target description. AArch64 inferiors
that don't support SVE also get only a process-wide target description.

> Maybe code it as:
>
> return current_thread->tdesc == nullptr? current_process ()->tdesc :
> current_thread->tdesc;

I did that. It turns out that this logic is only needed in two places:
regcache.cc::get_thread_regcache and tdesc.cc::current_target_desc.

> This would make the changes to all the other targets unnecessary, as
> those would be able to lookup their target descriptions from the
> process instead of the threads.

Indeed!

-- 
Thiago