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

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

Simon Marchi <simark@simark.ca> writes:

> On 2025-08-14 20:29, Thiago Jung Bauermann wrote:
>> Simon Marchi <simark@simark.ca> writes:
>> 
>>> On 8/14/25 12:29 AM, Thiago Jung Bauermann wrote:
>>>> Markus Metzger <markus.t.metzger@intel.com> writes:
>>>>> -      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?
>> 
>> For gdbserver, yes. My main doubt is whether we can assume other stubs
>> "out there" behave the same way or do they treat Hg0 and Hg-1
>> differently? The comment and code in remote_target::set_thread assume
>> the latter, so is that a bug?
>
> About the other stubs: I can't really help you there, I think the answer
> is that we'll never really know.
>
> Can you clarify what you think might be a bug?

I'm a bit concerned that GDB's remote_target::set_thread considers Hg-1
and Hg0 to be different things (doesn't say how they are different
though) while gdbserver's process_serial_event handling of the H packet
treats them as synonyms. I think that's strange, and was wondering if
anyone knew why. Maybe they just diverged without any practical
consequence and we can just bring them back to sync. Or maybe gdbserver
changed its behaviour but there are stubs out there that didn't.

This patches changes GDB from sending Hc-1 to Hg0. For gdbserver, the
change from -1 to 0 is meaningless. My question is whether we can assume
it will be meaningless to other stubs as well. But I agree that we'll
never really know, and if we just keep all the RSP warts forever it will
be hard to maintain and evolve it.

All this was just a long way of saying that I wanted to add my
Reviewed-by to this patch but I don't feel confident enough to do it.

-- 
Thiago