Re: [PATCH 2/2] gdb, remote: fix set_thread () in start_remote ()

[Simon Marchi <simark@simark.ca>]

On 8/14/25 12:29 AM, Thiago Jung Bauermann wrote:
> Markus Metzger <markus.t.metzger@intel.com> writes:
>> diff --git a/gdb/remote.c b/gdb/remote.c
>> index 6208a90f94a..9b76578bd80 100644
>> --- a/gdb/remote.c
>> +++ b/gdb/remote.c
>> @@ -5293,7 +5293,7 @@ remote_target::start_remote_1 (int from_tty, int extended_p)
>>        target_update_thread_list ();
>>  
>>        /* Let the stub know that we want it to return the thread.  */
> 
> This comment is a bit cryptic. What about something like:
> 
>   /* Let the stub know that we want it to return the thread id in the
>      qC reply from the get_current_thread call below.  */
> 
>> -      set_continue_thread (minus_one_ptid);
>> +      set_general_thread (any_thread_ptid);
>>  
>>        if (thread_count (this) == 0)
>>  	{
> 
> Also, your explanation implies that the doc comment in
> remote_target::set_thread isn't quite right. What about changing it to
> something along the following lines?
> 
> -/* If PTID is MAGIC_NULL_PTID, don't set any thread.  If PTID is
> -   MINUS_ONE_PTID, set the thread to -1, so the stub returns the
> -   thread.  If GEN is set, set the general thread, if not, then set
> -   the step/continue thread.  */
> +/* If PTID is MAGIC_NULL_PTID or ANY_THREAD_PTID, set the thread to 0.
> +   If PTID is MINUS_ONE_PTID, set the thread to -1.  If GEN is set, set
> +   the general thread, if not, then set the step/continue thread.  If
> +   the thread is 0 or -1 and GEN is set, the stub returns the thread in
> +   the qC packet reply.  */

The parts "set the thread to 0" and "set the thread to -1" seem not very
useful to me.  What does that mean concretely?  It would be more useful
to explain what that instruct the remote target to do.

My understanding is that passing either MAGIC_NULL_PTID or
ANY_THREAD_PTID will result in sending `Hg0` or `Hc0`.  Passing
MINUS_ONE_PTID will result in sending `Hg-1` or `Hc-1`.

On the other side, all of these will result in variable THREAD_ID being
set to null_ptid.  For Hg, we'll arbitrarily pick the first thread.  For
Hc, we'll set cs.cont_thread to null_ptid.

Does that sound right?

>  void
>  remote_target::set_thread (ptid_t ptid, int gen)
>  {
> @@ -3267,6 +3268,7 @@ remote_target::set_thread (ptid_t ptid, int gen)
>  
>    *buf++ = 'H';
>    *buf++ = gen ? 'g' : 'c';
> +  /* NB: gdbserver interprets 0 and -1 the same way in the H packet.  */
>    if (ptid == magic_null_ptid)
>      xsnprintf (buf, endbuf - buf, "0");
>    else if (ptid == any_thread_ptid)
> 
> One final comment: I agree with your conclusions, but unfortunately I'm
> not sure of them. The doc comment in remote_target::set_thread gives me
> a bit of pause. Because of that I don't think I should add a
> Reviewed-by.
> 
> Hopefully someone with more experience with the RSP and remote stubs can
> weigh in.

Markus, could you elaborate on how this bug was a problem for you, what
consequences it has down the line?  The change seems right, but I also
need some help convincing myself that it is indeed right.  It seems to
me like any "recent" gdbserver will send back a stop reply on
connection, so remote_target::get_current_thread will not send a qC to
return the current thread.

Simon