[PATCH 1/2] gdbserver, read_ptid: handle '0' and '-1' thread ids

[PATCH 1/2] gdbserver, read_ptid: handle '0' and '-1' thread ids

[Markus Metzger]

The special thread id '-1' means 'all threads'.
The special thread id '0' means 'any thread'.

Read_ptid () currently returns

    <current pid>.-1.0

and

    <current pid>.0.0

respectively.

Change that to minus_one_ptid for '-1' and to null_ptid for '0'.

CC: Thiago Jung Bauermann  <thiago.bauermann@linaro.org>
---
 gdbserver/remote-utils.cc | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/gdbserver/remote-utils.cc b/gdbserver/remote-utils.cc
index 15f073dd6be..9562cc2e1fb 100644
--- a/gdbserver/remote-utils.cc
+++ b/gdbserver/remote-utils.cc
@@ -566,6 +566,26 @@ read_ptid (const char *buf, const char **obuf)
   const char *p = buf;
   const char *pp;
 
+  /* Handle special thread ids.  */
+  if (*p == '0')
+    {
+      if (obuf)
+	*obuf = p + 1;
+
+      return null_ptid;
+    }
+
+  if (*p == '-')
+    {
+      if (p[1] != '1')
+	return null_ptid;
+
+      if (obuf)
+	*obuf = p + 2;
+
+      return minus_one_ptid;
+    }
+
   if (*p == 'p')
     {
       ULONGEST pid;
-- 
2.34.1

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

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

[Markus Metzger]

remote_target::start_remote_1 () calls set_continue_thread (minus_one_ptid)
with the intent to

      /* Let the stub know that we want it to return the thread.  */
      set_continue_thread (minus_one_ptid);

I interpret it such that it expects a later get_current_thread () to
return the thread selected by the target:

          /* We have thread information; select the thread the target
             says should be current.  If we're reconnecting to a
             multi-threaded program, this will ideally be the thread
             that last reported an event before GDB disconnected.  */
          ptid_t curr_thread = get_current_thread (wait_status);

This results in the packet sequence Hc-1, qC.

Hc simply sets cont_thread:

          else if (cs.own_buf[1] == 'c')
            cs.cont_thread = thread_id;

          write_ok (cs.own_buf);

and qC returns the general thread.  This doesn't match.

It also has some special treatment for null_ptid and minus_one_ptid:

    if (cs.general_thread != null_ptid && cs.general_thread != minus_one_ptid)
      ptid = cs.general_thread;
    else
      {
        init_thread_iter ();
        ptid = thread_iter->id;
      }

Similarly, Hg has some special treatment for null_ptid:

    if (cs.own_buf[1] == 'g')
      {
        if (thread_id == null_ptid)
          {
            /* GDB is telling us to choose any thread.  Check if
               the currently selected thread is still valid. If
               it is not, select the first available.  */
            thread_info *thread = find_thread_ptid (cs.general_thread);
            if (thread == NULL)
              thread = get_first_thread ();
            thread_id = thread->id;
          }

        cs.general_thread = thread_id;

The comment at Hg matches the intent of GDB for sending Hc-1.

Change the set_thread () call in remote_target::start_remote_1 () to

    set_general_thread (any_thread_ptid);

This results in GDB sending Hg0 and gdbserver preserving the currently
selected thread that is later returned in response to qC.

CC: Thiago Jung Bauermann  <thiago.bauermann@linaro.org>
---
 gdb/remote.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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.  */
-      set_continue_thread (minus_one_ptid);
+      set_general_thread (any_thread_ptid);
 
       if (thread_count (this) == 0)
 	{
-- 
2.34.1

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

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

[Thiago Jung Bauermann]

Markus Metzger <markus.t.metzger@intel.com> writes:

> remote_target::start_remote_1 () calls set_continue_thread (minus_one_ptid)
> with the intent to
>
>       /* Let the stub know that we want it to return the thread.  */
>       set_continue_thread (minus_one_ptid);
>
> I interpret it such that it expects a later get_current_thread () to
> return the thread selected by the target:
>
>           /* We have thread information; select the thread the target
>              says should be current.  If we're reconnecting to a
>              multi-threaded program, this will ideally be the thread
>              that last reported an event before GDB disconnected.  */
>           ptid_t curr_thread = get_current_thread (wait_status);

Oh, that makes sense. Thanks for digging into it!

> This results in the packet sequence Hc-1, qC.
>
> Hc simply sets cont_thread:
>
>           else if (cs.own_buf[1] == 'c')
>             cs.cont_thread = thread_id;
>
>           write_ok (cs.own_buf);
>
> and qC returns the general thread.  This doesn't match.

🤦

> It also has some special treatment for null_ptid and minus_one_ptid:
>
>     if (cs.general_thread != null_ptid && cs.general_thread != minus_one_ptid)
>       ptid = cs.general_thread;
>     else
>       {
>         init_thread_iter ();
>         ptid = thread_iter->id;
>       }
>
> Similarly, Hg has some special treatment for null_ptid:
>
>     if (cs.own_buf[1] == 'g')
>       {
>         if (thread_id == null_ptid)
>           {
>             /* GDB is telling us to choose any thread.  Check if
>                the currently selected thread is still valid. If
>                it is not, select the first available.  */
>             thread_info *thread = find_thread_ptid (cs.general_thread);
>             if (thread == NULL)
>               thread = get_first_thread ();
>             thread_id = thread->id;
>           }
>
>         cs.general_thread = thread_id;
>
> The comment at Hg matches the intent of GDB for sending Hc-1.

Indeed!

> Change the set_thread () call in remote_target::start_remote_1 () to
>
>     set_general_thread (any_thread_ptid);
>
> This results in GDB sending Hg0 and gdbserver preserving the currently
> selected thread that is later returned in response to qC.
>
> CC: Thiago Jung Bauermann  <thiago.bauermann@linaro.org>
> ---
>  gdb/remote.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Great! My only suggestion is to try to make the GDB comments more
consistent:

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

-- 
Thiago

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

[Simon Marchi]

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

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

[Thiago Jung Bauermann]

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?

-- 
Thiago

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

[Simon Marchi]

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?

Simon

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

[Thiago Jung Bauermann]

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

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

[Metzger, Markus T]

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

Re: [PATCH 1/2] gdbserver, read_ptid: handle '0' and '-1' thread ids

[Thiago Jung Bauermann]

Hello Markus,

Markus Metzger <markus.t.metzger@intel.com> writes:

> The special thread id '-1' means 'all threads'.
> The special thread id '0' means 'any thread'.
>
> Read_ptid () currently returns
>
>     <current pid>.-1.0
>
> and
>
>     <current pid>.0.0
>
> respectively.
>
> Change that to minus_one_ptid for '-1' and to null_ptid for '0'.
>
> CC: Thiago Jung Bauermann  <thiago.bauermann@linaro.org>
> ---
>  gdbserver/remote-utils.cc | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)

Nice improvement. Just a minor comment.

> diff --git a/gdbserver/remote-utils.cc b/gdbserver/remote-utils.cc
> index 15f073dd6be..9562cc2e1fb 100644
> --- a/gdbserver/remote-utils.cc
> +++ b/gdbserver/remote-utils.cc
> @@ -566,6 +566,26 @@ read_ptid (const char *buf, const char **obuf)
>    const char *p = buf;
>    const char *pp;
>  
> +  /* Handle special thread ids.  */
> +  if (*p == '0')

I think this should also check the character after '0':

-  if (*p == '0')
+  if (*p == '0' && !isxdigit (p[1]))

> +    {
> +      if (obuf)
> +	*obuf = p + 1;
> +
> +      return null_ptid;
> +    }
> +
> +  if (*p == '-')
> +    {
> +      if (p[1] != '1')

Same here:

-      if (p[1] != '1')
+      if (p[1] != '1' || isxdigit (p[2]))

> +	return null_ptid;
> +
> +      if (obuf)
> +	*obuf = p + 2;
> +
> +      return minus_one_ptid;
> +    }
> +
>    if (*p == 'p')
>      {
>        ULONGEST pid;

From reading the RSP documentation, thread ids are followed only by ',',
';', ':' or '\0', but checking for an hex digit should be enough.

With the changes:

Reviewed-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>

-- 
Thiago

Re: [PATCH 1/2] gdbserver, read_ptid: handle '0' and '-1' thread ids

[Simon Marchi]

On 8/13/25 11:36 PM, Thiago Jung Bauermann wrote:
> Hello Markus,
> 
> Markus Metzger <markus.t.metzger@intel.com> writes:
> 
>> The special thread id '-1' means 'all threads'.
>> The special thread id '0' means 'any thread'.
>>
>> Read_ptid () currently returns
>>
>>     <current pid>.-1.0
>>
>> and
>>
>>     <current pid>.0.0
>>
>> respectively.
>>
>> Change that to minus_one_ptid for '-1' and to null_ptid for '0'.

I can see that there are read_ptid functions both on GDB and
GDBserver-side.  We can see that they are similar, but they have
diverged a bit (and they will diverge even more with this patch).  IWBN
if possible to have just one shared implementation.  One part that
differs between the implementations is where we get the default pid
value.  That value would have to be passed in as a parameter.  The nice
side-effect of that is that it make it easy to unit test this function,
which we don't do right now.

>>
>> CC: Thiago Jung Bauermann  <thiago.bauermann@linaro.org>
>> ---
>>  gdbserver/remote-utils.cc | 20 ++++++++++++++++++++
>>  1 file changed, 20 insertions(+)
> 
> Nice improvement. Just a minor comment.
> 
>> diff --git a/gdbserver/remote-utils.cc b/gdbserver/remote-utils.cc
>> index 15f073dd6be..9562cc2e1fb 100644
>> --- a/gdbserver/remote-utils.cc
>> +++ b/gdbserver/remote-utils.cc
>> @@ -566,6 +566,26 @@ read_ptid (const char *buf, const char **obuf)
>>    const char *p = buf;
>>    const char *pp;
>>  
>> +  /* Handle special thread ids.  */
>> +  if (*p == '0')
> 
> I think this should also check the character after '0':

Agreed.  I think it would be valid to pass `0A`, to represent tid 10.

Can we instead let the code go through hex_or_minus_one below and handle
"tid == 0" there?

Simon

RE: [PATCH 1/2] gdbserver, read_ptid: handle '0' and '-1' thread ids

[Metzger, Markus T]

Hello Simon, Thiago,

Thanks for your reviews.

>>> The special thread id '-1' means 'all threads'.
>>> The special thread id '0' means 'any thread'.
>>>
>>> Read_ptid () currently returns
>>>
>>>     <current pid>.-1.0
>>>
>>> and
>>>
>>>     <current pid>.0.0
>>>
>>> respectively.
>>>
>>> Change that to minus_one_ptid for '-1' and to null_ptid for '0'.
>
>I can see that there are read_ptid functions both on GDB and
>GDBserver-side.  We can see that they are similar, but they have
>diverged a bit (and they will diverge even more with this patch).  IWBN
>if possible to have just one shared implementation.  One part that
>differs between the implementations is where we get the default pid
>value.  That value would have to be passed in as a parameter.  The nice
>side-effect of that is that it make it easy to unit test this function,
>which we don't do right now.

They are more different than it first appears.  The gdbserver side
accepts pN.-1, for example, while the gdb side doesn't.  The gdb side
doesn't accept -1 at all.

When the string does not start with 'p', both fall back to the
respective inferior on either side, but gdb has special handling for a
not-yet-known process id, whereas the gdbserver side doesn't.


>>>
>>> CC: Thiago Jung Bauermann  <thiago.bauermann@linaro.org>
>>> ---
>>>  gdbserver/remote-utils.cc | 20 ++++++++++++++++++++
>>>  1 file changed, 20 insertions(+)
>>
>> Nice improvement. Just a minor comment.
>>
>>> diff --git a/gdbserver/remote-utils.cc b/gdbserver/remote-utils.cc
>>> index 15f073dd6be..9562cc2e1fb 100644
>>> --- a/gdbserver/remote-utils.cc
>>> +++ b/gdbserver/remote-utils.cc
>>> @@ -566,6 +566,26 @@ read_ptid (const char *buf, const char **obuf)
>>>    const char *p = buf;
>>>    const char *pp;
>>>
>>> +  /* Handle special thread ids.  */
>>> +  if (*p == '0')
>>
>> I think this should also check the character after '0':
>
>Agreed.  I think it would be valid to pass `0A`, to represent tid 10.
>
>Can we instead let the code go through hex_or_minus_one below and handle
>"tid == 0" there?

That is indeed a nice improvement that simplifies this patch to just:

diff --git a/gdbserver/remote-utils.cc b/gdbserver/remote-utils.cc
index 760655994c5..f5d70c546d3 100644
--- a/gdbserver/remote-utils.cc
+++ b/gdbserver/remote-utils.cc
@@ -587,6 +587,13 @@ read_ptid (const char *buf, const char **obuf)
   /* No multi-process.  Just a tid.  */
   ULONGEST tid = hex_or_minus_one (p, &pp);
 
+  /* Handle special thread ids.  */
+  if (tid == (ULONGEST) -1)
+    return minus_one_ptid;
+
+  if (tid == 0)
+    return null_ptid;
+
   /* Since GDB is not sending a process id (multi-process extensions
      are off), then there's only one process.  Default to the first in
      the list.  */

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

Re: [PATCH 1/2] gdbserver, read_ptid: handle '0' and '-1' thread ids

[Tom Tromey]

> They are more different than it first appears.  The gdbserver side
> accepts pN.-1, for example, while the gdb side doesn't.  The gdb side
> doesn't accept -1 at all.

> When the string does not start with 'p', both fall back to the
> respective inferior on either side, but gdb has special handling for a
> not-yet-known process id, whereas the gdbserver side doesn't.


I haven't been following this super closely, but there's also a bugzilla
bug in this area:

https://sourceware.org/bugzilla/show_bug.cgi?id=25111

Tom

RE: [PATCH 1/2] gdbserver, read_ptid: handle '0' and '-1' thread ids

[Metzger, Markus T]

Hello Tom,

>> They are more different than it first appears.  The gdbserver side
>> accepts pN.-1, for example, while the gdb side doesn't.  The gdb side
>> doesn't accept -1 at all.
>
>> When the string does not start with 'p', both fall back to the
>> respective inferior on either side, but gdb has special handling for a
>> not-yet-known process id, whereas the gdbserver side doesn't.
>
>
>I haven't been following this super closely, but there's also a bugzilla
>bug in this area:
>
>https://sourceware.org/bugzilla/show_bug.cgi?id=25111

That seems to be caused by truncating a long ptid.tid to int before
writing it out.

In general, read_ptid() and write_ptid() don't seem to care about
types and conversions; read_ptid(), for example, reads everything
into ULONGEST and then constructs a ptid without checking for
overflows.

I could add a fix to this series, but, as with the other patches, this
would be solely based on reviewing the code without an actual bug
(I cannot test this zephyr target) nor a test.

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

RE: [PATCH 1/2] gdbserver, read_ptid: handle '0' and '-1' thread ids

[Metzger, Markus T]

Hello Tom,

>In general, read_ptid() and write_ptid() don't seem to care about
>types and conversions; read_ptid(), for example, reads everything
>into ULONGEST and then constructs a ptid without checking for
>overflows.
>
>I could add a fix to this series, but, as with the other patches, this
>would be solely based on reviewing the code without an actual bug
>(I cannot test this zephyr target) nor a test.

Here's the patch.  I'll add it to v2 of this series.

I tested this on ubuntu 22.04 running on x86-64.  Is it just me or is
everybody seeing lots of sporadic fails that appear and disappear
between runs?

I'm running tests with FORCE_PARALLEL=1 for each patch in a series
plus a few upstream patches in front and then filter out fails that
appear and disappear manually.

Regards,
Markus.

---

commit b6ec4c40ec46fdba710369e2541699d2d704c828
Author: Markus Metzger <markus.t.metzger@intel.com>
Date:   Wed Sep 24 12:08:55 2025 +0000

    gdb, gdbserver: fix read/write_ptid types
    
    In write_ptid(), a ptid's LWP member, which is declared long, is stored in
    an int local variable before printing, potentially truncating it.  Fix it.
    
    In read_ptid(), both PID and LWP are read as ULONGEST and then cast to
    their respective type without checking for overflows.  Fix it.
    
    In read_ptid(), an empty component is treated as zero.  Diagnose that as
    an error, instead.
    
    CC: Tom Tromey <tom@tromey.com>

diff --git a/gdb/remote.c b/gdb/remote.c
index 961322f398b..6d609ced02b 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -3464,7 +3464,8 @@ static int remote_newthread_step (threadref *ref, void *context);
 char *
 remote_target::write_ptid (char *buf, const char *endbuf, ptid_t ptid)
 {
-  int pid, tid;
+  ptid_t::pid_type pid;
+  ptid_t::lwp_type lwp;
 
   if (m_features.remote_multi_process_p ())
     {
@@ -3474,11 +3475,11 @@ remote_target::write_ptid (char *buf, const char *endbuf, ptid_t ptid)
       else
 	buf += xsnprintf (buf, endbuf - buf, "p%x.", pid);
     }
-  tid = ptid.lwp ();
-  if (tid < 0)
-    buf += xsnprintf (buf, endbuf - buf, "-%x", -tid);
+  lwp = ptid.lwp ();
+  if (lwp < 0)
+    buf += xsnprintf (buf, endbuf - buf, "-%lx", -lwp);
   else
-    buf += xsnprintf (buf, endbuf - buf, "%x", tid);
+    buf += xsnprintf (buf, endbuf - buf, "%lx", lwp);
 
   return buf;
 }
@@ -3492,24 +3493,38 @@ read_ptid (const char *buf, const char **obuf)
 {
   const char *p = buf;
   const char *pp;
-  ULONGEST pid = 0, tid = 0;
+  ptid_t::pid_type pid = 0;
+  ptid_t::lwp_type lwp = 0;
+  ULONGEST hex;
 
   if (*p == 'p')
     {
       /* Multi-process ptid.  */
-      pp = unpack_varlen_hex (p + 1, &pid);
-      if (*pp != '.')
-	error (_("invalid remote ptid: %s"), p);
+      pp = unpack_varlen_hex (p + 1, &hex);
+      if ((pp == (p + 1)) || (*pp != '.'))
+	error (_("invalid remote ptid: %s"), buf);
+
+      pid = (ptid_t::pid_type) (LONGEST) hex;
+      if (hex != ((ULONGEST) pid))
+	error (_("invalid remote ptid: %s"), buf);
+
+      p = pp + 1;
+      pp = unpack_varlen_hex (p, &hex);
+      if (pp == p)
+	error (_("invalid remote ptid: %s"), buf);
+
+      lwp = (ptid_t::lwp_type) (LONGEST) hex;
+      if (hex != ((ULONGEST) lwp))
+	error (_("invalid remote ptid: %s"), buf);
 
-      p = pp;
-      pp = unpack_varlen_hex (p + 1, &tid);
       if (obuf)
 	*obuf = pp;
-      return ptid_t (pid, tid);
+
+      return ptid_t (pid, lwp);
     }
 
-  /* No multi-process.  Just a tid.  */
-  pp = unpack_varlen_hex (p, &tid);
+  /* No multi-process.  Just a thread id.  */
+  pp = unpack_varlen_hex (p, &hex);
 
   /* Return null_ptid when no thread id is found.  */
   if (p == pp)
@@ -3519,6 +3534,10 @@ read_ptid (const char *buf, const char **obuf)
       return null_ptid;
     }
 
+  lwp = (ptid_t::lwp_type) (LONGEST) hex;
+  if (hex != ((ULONGEST) lwp))
+    error (_("invalid remote ptid: %s"), buf);
+
   /* Since the stub is not sending a process id, default to what's
      current_inferior, unless it doesn't have a PID yet.  If so,
      then since there's no way to know the pid of the reported
@@ -3531,7 +3550,8 @@ read_ptid (const char *buf, const char **obuf)
 
   if (obuf)
     *obuf = pp;
-  return ptid_t (pid, tid);
+
+  return ptid_t (pid, lwp);
 }
 
 static int
diff --git a/gdbserver/remote-utils.cc b/gdbserver/remote-utils.cc
index f5d70c546d3..fe2b6acb4e9 100644
--- a/gdbserver/remote-utils.cc
+++ b/gdbserver/remote-utils.cc
@@ -519,7 +519,8 @@ char *
 write_ptid (char *buf, ptid_t ptid)
 {
   client_state &cs = get_client_state ();
-  int pid, tid;
+  ptid_t::pid_type pid;
+  ptid_t::lwp_type lwp;
 
   if (cs.multi_process)
     {
@@ -529,11 +530,11 @@ write_ptid (char *buf, ptid_t ptid)
       else
 	buf += sprintf (buf, "p%x.", pid);
     }
-  tid = ptid.lwp ();
-  if (tid < 0)
-    buf += sprintf (buf, "-%x", -tid);
+  lwp = ptid.lwp ();
+  if (lwp < 0)
+    buf += sprintf (buf, "-%lx", -lwp);
   else
-    buf += sprintf (buf, "%x", tid);
+    buf += sprintf (buf, "%lx", lwp);
 
   return buf;
 }
@@ -564,45 +565,59 @@ read_ptid (const char *buf, const char **obuf)
 {
   const char *p = buf;
   const char *pp;
+  ptid_t::pid_type pid = 0;
+  ptid_t::lwp_type lwp = 0;
+  ULONGEST hex;
 
   if (*p == 'p')
     {
-      ULONGEST pid;
-
       /* Multi-process ptid.  */
-      pp = unpack_varlen_hex (p + 1, &pid);
-      if (*pp != '.')
-	error ("invalid remote ptid: %s\n", p);
+      pp = unpack_varlen_hex (p + 1, &hex);
+      if ((pp == (p + 1)) || (*pp != '.'))
+	error ("invalid remote ptid: %s\n", buf);
+
+      pid = (ptid_t::pid_type) (LONGEST) hex;
+      if (hex != ((ULONGEST) pid))
+	error (_("invalid remote ptid: %s"), buf);
 
       p = pp + 1;
+      hex = hex_or_minus_one (p, &pp);
+      if (pp == p)
+	error ("invalid remote ptid: %s\n", buf);
 
-      ULONGEST tid = hex_or_minus_one (p, &pp);
+      lwp = (ptid_t::lwp_type) (LONGEST) hex;
+      if (hex != ((ULONGEST) lwp))
+	error (_("invalid remote ptid: %s"), buf);
 
       if (obuf)
 	*obuf = pp;
 
-      return ptid_t (pid, tid);
+      return ptid_t (pid, lwp);
     }
 
-  /* No multi-process.  Just a tid.  */
-  ULONGEST tid = hex_or_minus_one (p, &pp);
+  /* No multi-process.  Just a thread id.  */
+  hex = hex_or_minus_one (p, &pp);
 
   /* Handle special thread ids.  */
-  if (tid == (ULONGEST) -1)
+  if (hex == (ULONGEST) -1)
     return minus_one_ptid;
 
-  if (tid == 0)
+  if (hex == 0)
     return null_ptid;
 
+  lwp = (ptid_t::lwp_type) (LONGEST) hex;
+  if (hex != ((ULONGEST) lwp))
+    error (_("invalid remote ptid: %s"), buf);
+
   /* Since GDB is not sending a process id (multi-process extensions
      are off), then there's only one process.  Default to the first in
      the list.  */
-  int pid = get_first_process ()->pid;
+  pid = get_first_process ()->pid;
 
   if (obuf)
     *obuf = pp;
 
-  return ptid_t (pid, tid);
+  return ptid_t (pid, lwp);
 }
 
 /* Write COUNT bytes in BUF to the client.

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