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

["Metzger, Markus T" <markus.t.metzger@intel.com>]

Hello Simon, Thiago,

Thanks for your reviews.


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

That came up in https://sourceware.org/pipermail/gdb-patches/2025-August/219582.html.

I had received an error from gdbserver in response to a Hc-1 from GDB
during startup of our GPU target.  That had been a long time ago and I can
no longer reproduce the error.

During the review discussion with Thiago, I dug into the code, which resulted
in this small patch series.

I dropped the patch that triggered this discussion from our GPU series, but
didn't want this to be lost.  Since I can no longer reproduce the error, this is
entirely based on reviewing the code.

Regards,
Markus.

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928