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

Luis Machado via Gdb-patches
2023-02-01T16:32:41Z

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