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

Simon Marchi via Gdb-patches
2023-02-01T16:21:30Z

>> 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