[PATCH] Do not write negative PID or TID in remote protocol

[PATCH] Do not write negative PID or TID in remote protocol

[Tom Tromey]

Currently both gdb and gdbserver can write a negative number for the
PID or TID.  However, the only negative value that really makes sense
is the special case of "-1" -- in other cases if the PID or TID has
the high bit set, it should still be written as a positive number.

This patch attempts to fix the bug.

v2 of this patch combines the implementations and moves them to
gdbsupport.  I tried making these standalone functions in rsp-low.cc,
but that runs afoul of libipa.  So, I made them methods of ptid_t.

The new unit tests pointed out that round-tripping a sign-extended
number didn't really work, because the checks were done via ULONGEST.
It's kind of unfortunate that the PID and LWP are host-dependent
types.  This should probably be fixed, but I haven't done so here.
Meanwhile I changed the checks to use the corresponding unsigned type.

v1 is here

    https://inbox.sourceware.org/gdb-patches/20260311202152.2704410-1-tromey@adacore.com/

Regression tested on x86-64 Fedora 43.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=25111
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=33979
---
 gdb/remote.c                   | 100 ++++++------------------------
 gdb/unittests/ptid-selftests.c |  46 ++++++++++++++
 gdbserver/remote-utils.cc      | 103 ++++---------------------------
 gdbsupport/ptid.cc             | 107 +++++++++++++++++++++++++++++++++
 gdbsupport/ptid.h              |  17 ++++++
 5 files changed, 201 insertions(+), 172 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index 1455838c2cf..e8ce08ca045 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -3684,31 +3684,17 @@ static char *pack_threadlist_request (char *pkt, int startflag,
 static int remote_newthread_step (threadref *ref, void *context);
 
 
-/* Write a PTID to BUF.  ENDBUF points to one-passed-the-end of the
+/* Write a PTID to BUF.  ENDBUF points to one-past-the-end of the
    buffer we're allowed to write to.  Returns
    BUF+CHARACTERS_WRITTEN.  */
 
 char *
 remote_target::write_ptid (char *buf, const char *endbuf, ptid_t ptid)
 {
-  ptid_t::pid_type pid;
-  ptid_t::lwp_type lwp;
-
-  if (m_features.remote_multi_process_p ())
-    {
-      pid = ptid.pid ();
-      if (pid < 0)
-	buf += xsnprintf (buf, endbuf - buf, "p-%x.", -pid);
-      else
-	buf += xsnprintf (buf, endbuf - buf, "p%x.", pid);
-    }
-  lwp = ptid.lwp ();
-  if (lwp < 0)
-    buf += xsnprintf (buf, endbuf - buf, "-%lx", -lwp);
-  else
-    buf += xsnprintf (buf, endbuf - buf, "%lx", lwp);
-
-  return buf;
+  std::string repr = ptid.to_rsp_string (m_features.remote_multi_process_p ());
+  gdb_assert (repr.length () < endbuf - buf);
+  strcpy (buf, repr.c_str ());
+  return buf + repr.length ();
 }
 
 /* Extract a PTID from BUF.  If non-null, OBUF is set to one past the
@@ -3718,67 +3704,21 @@ remote_target::write_ptid (char *buf, const char *endbuf, ptid_t ptid)
 static ptid_t
 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')
-    {
-      /* Multi-process ptid.  */
-      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);
-
-      if (obuf)
-	*obuf = pp;
-
-      return ptid_t (pid, lwp);
-    }
-
-  /* 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)
-    {
-      if (obuf)
-	*obuf = pp;
-      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
-     threads, use the magic number.  */
-  inferior *inf = current_inferior ();
-  if (inf->pid == 0)
-    pid = magic_null_ptid.pid ();
-  else
-    pid = inf->pid;
-
-  if (obuf)
-    *obuf = pp;
-
-  return ptid_t (pid, lwp);
+  return ptid_t::parse (buf, obuf, false,
+    [] ()
+      {
+	/* 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 threads, use the magic number.  */
+	inferior *inf = current_inferior ();
+	ptid_t::pid_type pid;
+	if (inf->pid == 0)
+	  pid = magic_null_ptid.pid ();
+	else
+	  pid = inf->pid;
+	return pid;
+      });
 }
 
 static int
diff --git a/gdb/unittests/ptid-selftests.c b/gdb/unittests/ptid-selftests.c
index 7b8ada9500a..c02401db952 100644
--- a/gdb/unittests/ptid-selftests.c
+++ b/gdb/unittests/ptid-selftests.c
@@ -18,6 +18,7 @@
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
 #include "gdbsupport/ptid.h"
+#include "gdbsupport/selftest.h"
 #include <type_traits>
 
 namespace selftests {
@@ -149,6 +150,51 @@ static_assert (both.matches (both), "both matches both");
 static_assert (!ptid_t (2, 2, 2).matches (both),
 	       "other both doesn't match both");
 
+/* Helper function to parse and return a single PTID.  */
+static ptid_t
+parse_one (const char *str, bool for_remote)
+{
+  const char *out;
+  ptid_t result = ptid_t::parse (str, &out, for_remote,
+    [] ()
+      {
+	return ptid_t::pid_type (23);
+      });
+  SELF_CHECK (*out == '\0');
+  return result;
+}
+
+/* Test ptid_t reading and writing.  */
+static void
+test ()
+{
+  SELF_CHECK (minus_one_ptid.to_rsp_string (false) == "0");
+  SELF_CHECK (minus_one_ptid.to_rsp_string (true) == "p-1.0");
+
+  SELF_CHECK (lwp.to_rsp_string (false) == "2");
+  SELF_CHECK (lwp.to_rsp_string (true) == "p1.2");
+
+  ptid_t negative (-57, -32);
+  /* This is kind of lame but currently the types are host-dependent
+     so we have to adapt to those here.  */
+  std::string the_pid = string_printf ("%x", (unsigned) -57);
+  std::string the_lwp = string_printf ("%lx", (unsigned long) -32);
+  std::string full = "p" + the_pid + "." + the_lwp;
+  SELF_CHECK (negative.to_rsp_string (false) == the_lwp);
+  SELF_CHECK (negative.to_rsp_string (true) == full);
+
+  SELF_CHECK (parse_one ("p1.2", false) == lwp);
+  SELF_CHECK (parse_one ("p1.-1", true) == ptid_t (1, -1));
+  SELF_CHECK (parse_one ("-1", true) == minus_one_ptid);
+  SELF_CHECK (parse_one ("0", false) == null_ptid);
+  SELF_CHECK (parse_one (full.c_str (), false) == negative);
+  SELF_CHECK (parse_one (full.c_str (), true) == negative);
+}
 
 } /* namespace ptid */
 } /* namespace selftests */
+
+INIT_GDB_FILE (ptid_selftests)
+{
+  selftests::register_test ("ptid_t", selftests::ptid::test);
+}
diff --git a/gdbserver/remote-utils.cc b/gdbserver/remote-utils.cc
index d7049baf083..1c899f141bc 100644
--- a/gdbserver/remote-utils.cc
+++ b/gdbserver/remote-utils.cc
@@ -519,43 +519,9 @@ char *
 write_ptid (char *buf, ptid_t ptid)
 {
   client_state &cs = get_client_state ();
-  ptid_t::pid_type pid;
-  ptid_t::lwp_type lwp;
-
-  if (cs.multi_process)
-    {
-      pid = ptid.pid ();
-      if (pid < 0)
-	buf += sprintf (buf, "p-%x.", -pid);
-      else
-	buf += sprintf (buf, "p%x.", pid);
-    }
-  lwp = ptid.lwp ();
-  if (lwp < 0)
-    buf += sprintf (buf, "-%lx", -lwp);
-  else
-    buf += sprintf (buf, "%lx", lwp);
-
-  return buf;
-}
-
-static ULONGEST
-hex_or_minus_one (const char *buf, const char **obuf)
-{
-  ULONGEST ret;
-
-  if (startswith (buf, "-1"))
-    {
-      ret = (ULONGEST) -1;
-      buf += 2;
-    }
-  else
-    buf = unpack_varlen_hex (buf, &ret);
-
-  if (obuf)
-    *obuf = buf;
-
-  return ret;
+  std::string repr = ptid.to_rsp_string (cs.multi_process);
+  strcpy (buf, repr.c_str ());
+  return buf + repr.length ();
 }
 
 /* Extract a PTID from BUF.  If non-null, OBUF is set to one past the last
@@ -563,61 +529,14 @@ hex_or_minus_one (const char *buf, const char **obuf)
 ptid_t
 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')
-    {
-      /* Multi-process ptid.  */
-      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);
-
-      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, lwp);
-    }
-
-  /* No multi-process.  Just a thread id.  */
-  hex = hex_or_minus_one (p, &pp);
-
-  /* Handle special thread ids.  */
-  if (hex == (ULONGEST) -1)
-    return minus_one_ptid;
-
-  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.  */
-  pid = get_first_process ()->pid;
-
-  if (obuf)
-    *obuf = pp;
-
-  return ptid_t (pid, lwp);
+  return ptid_t::parse (buf, obuf, true,
+    [] ()
+      {
+	/* 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.  */
+	return get_first_process ()->pid;
+      });
 }
 
 /* Write COUNT bytes in BUF to the client.  Returns true if all bytes
diff --git a/gdbsupport/ptid.cc b/gdbsupport/ptid.cc
index 78978c15c06..731a5bc3da3 100644
--- a/gdbsupport/ptid.cc
+++ b/gdbsupport/ptid.cc
@@ -19,6 +19,7 @@
 
 #include "ptid.h"
 #include "print-utils.h"
+#include "rsp-low.h"
 
 /* See ptid.h for these.  */
 
@@ -27,8 +28,114 @@ ptid_t const minus_one_ptid = ptid_t::make_minus_one ();
 
 /* See ptid.h.  */
 
+std::string
+ptid_t::to_rsp_string (bool multi) const
+{
+  char result[50];
+  char *buf = result;
+  char *endbuf = &result[sizeof (result)];
+
+  if (multi)
+    {
+      if (m_pid == -1)
+	buf += xsnprintf (buf, endbuf - buf, "p-1.");
+      else
+	buf += xsnprintf (buf, endbuf - buf, "p%x.", (unsigned) m_pid);
+    }
+  if (m_lwp == -1)
+    xsnprintf (buf, endbuf - buf, "-1");
+  else
+    xsnprintf (buf, endbuf - buf, "%lx", (unsigned long) m_lwp);
+
+  return result;
+}
+
+/* See ptid.h.  */
+
 std::string
 ptid_t::to_string () const
 {
   return string_printf ("%d.%ld.%s", m_pid, m_lwp, pulongest (m_tid));
 }
+
+/* See ptid.h.  */
+
+static ULONGEST
+hex_or_minus_one (const char *buf, const char **obuf, bool for_remote)
+{
+  ULONGEST ret;
+
+  if (for_remote && startswith (buf, "-1"))
+    {
+      ret = (ULONGEST) -1;
+      buf += 2;
+    }
+  else
+    buf = unpack_varlen_hex (buf, &ret);
+
+  if (obuf != nullptr)
+    *obuf = buf;
+
+  return ret;
+}
+
+ptid_t
+ptid_t::parse (const char *buf, const char **obuf, bool for_remote,
+	       gdb::function_view<pid_type ()> default_pid)
+{
+  const char *p = buf;
+  const char *pp;
+  ptid_t::pid_type pid = 0;
+  ptid_t::lwp_type lwp = 0;
+  ULONGEST hex;
+
+  using unsigned_pid_type = std::make_unsigned<pid_type>::type;
+  using unsigned_lwp_type = std::make_unsigned<lwp_type>::type;
+
+  if (*p == 'p')
+    {
+      /* Multi-process ptid.  */
+      pp = unpack_varlen_hex (p + 1, &hex);
+      if (pp == (p + 1) || *pp != '.')
+	error (_("invalid remote ptid: %s"), buf);
+
+      pid = (ptid_t::pid_type) hex;
+      if (hex != ((unsigned_pid_type) pid))
+	error (_("invalid remote ptid: %s"), buf);
+
+      p = pp + 1;
+      hex = hex_or_minus_one (p, &pp, for_remote);
+      if (pp == p)
+	error (_("invalid remote ptid: %s"), buf);
+
+      lwp = (ptid_t::lwp_type) hex;
+      if (hex != ((unsigned_lwp_type) lwp))
+	error (_("invalid remote ptid: %s"), buf);
+
+      if (obuf != nullptr)
+	*obuf = pp;
+
+      return ptid_t (pid, lwp);
+    }
+
+  /* No multi-process.  Just a thread id.  */
+  hex = hex_or_minus_one (p, &pp, for_remote);
+
+  /* Handle special thread ids.  */
+  if (hex == (ULONGEST) -1)
+    return minus_one_ptid;
+
+  if (hex == 0)
+    return null_ptid;
+
+  lwp = (ptid_t::lwp_type) hex;
+  if (hex != ((unsigned_lwp_type) lwp))
+    error (_("invalid remote ptid: %s"), buf);
+
+  pid = default_pid ();
+
+  if (obuf != nullptr)
+    *obuf = pp;
+
+  return ptid_t (pid, lwp);
+}
diff --git a/gdbsupport/ptid.h b/gdbsupport/ptid.h
index dfc0fd77df0..ef6da77eb7e 100644
--- a/gdbsupport/ptid.h
+++ b/gdbsupport/ptid.h
@@ -35,6 +35,7 @@
 #include <functional>
 #include <string>
 #include "gdbsupport/common-types.h"
+#include "gdbsupport/function-view.h"
 
 class ptid_t
 {
@@ -130,6 +131,12 @@ class ptid_t
 	    || *this == filter);
   }
 
+  /* Return a remote-protocol-compatible string representation of this
+     ptid.  The MULTI argument controls whether the multi-process
+     representation is used.  */
+
+  std::string to_rsp_string (bool multi) const;
+
   /* Return a string representation of the ptid.
 
      This is only meant to be used in debug messages.  */
@@ -146,6 +153,16 @@ class ptid_t
   static constexpr ptid_t make_minus_one ()
   { return ptid_t (-1, 0, 0); }
 
+  /* Extract a PTID from BUF.  If non-null, OBUF is set to one past
+     the last parsed char.  FOR_REMOTE is true for "gdbserver" style
+     parsing, that is, applying a special meaning to "-1" (see RSP
+     docs for details).  DEFAULT_PID is called when the parsed string
+     does not provide a process ID; it should return the default PID
+     to use.  This function throws on error.  */
+
+  static ptid_t parse (const char *buf, const char **obuf, bool for_remote,
+		       gdb::function_view<pid_type ()> default_pid);
+
 private:
   /* Process id.  */
   pid_type m_pid;

base-commit: 97d3d451522bb9e7e0f80c68c4c1acab21499e98
-- 
2.53.0

Re: [PATCH] Do not write negative PID or TID in remote protocol

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tromey@adacore.com> writes:

Ping for this patch.

I guess at some point I'll just put it in.
Note below...

Tom> Currently both gdb and gdbserver can write a negative number for the
Tom> PID or TID.  However, the only negative value that really makes sense
Tom> is the special case of "-1" -- in other cases if the PID or TID has
Tom> the high bit set, it should still be written as a positive number.

Tom> This patch attempts to fix the bug.

Tom> v2 of this patch combines the implementations and moves them to
Tom> gdbsupport.  I tried making these standalone functions in rsp-low.cc,
Tom> but that runs afoul of libipa.  So, I made them methods of ptid_t.

Tom> The new unit tests pointed out that round-tripping a sign-extended
Tom> number didn't really work, because the checks were done via ULONGEST.
Tom> It's kind of unfortunate that the PID and LWP are host-dependent
Tom> types.  This should probably be fixed, but I haven't done so here.

Maybe this should be done.

Also at some point I should update the RSP docs, though the exact update
depends on the outcome of whether or not we decide to widen the types.

Tom

Re: [PATCH] Do not write negative PID or TID in remote protocol

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tromey@adacore.com> writes:

Tom> Ping for this patch.

Tom> I guess at some point I'll just put it in.

I'm going to check this in now.  Although this doesn't implement the
ideal (IMO) approach, it does still fix an existing bug and so it's an
improvement.

Tom

Re: [PATCH] Do not write negative PID or TID in remote protocol

[Mark Wielaard]

Hi Tom,

On Thu, Apr 30, 2026 at 11:59:04AM -0600, Tom Tromey wrote:
> Currently both gdb and gdbserver can write a negative number for the
> PID or TID.  However, the only negative value that really makes sense
> is the special case of "-1" -- in other cases if the PID or TID has
> the high bit set, it should still be written as a positive number.
> 
> This patch attempts to fix the bug.
> 
> v2 of this patch combines the implementations and moves them to
> gdbsupport.  I tried making these standalone functions in rsp-low.cc,
> but that runs afoul of libipa.  So, I made them methods of ptid_t.
> 
> The new unit tests pointed out that round-tripping a sign-extended
> number didn't really work, because the checks were done via ULONGEST.
> It's kind of unfortunate that the PID and LWP are host-dependent
> types.  This should probably be fixed, but I haven't done so here.
> Meanwhile I changed the checks to use the corresponding unsigned type.

It looks like there is still something wrong on 32bit arches.
At least after this patch the i386 debian buildbot fails:
https://builder.sourceware.org/buildbot/#/builders/105/builds/12989

> +ptid_t
> +ptid_t::parse (const char *buf, const char **obuf, bool for_remote,
> +	       gdb::function_view<pid_type ()> default_pid)
> +{
> +  const char *p = buf;
> +  const char *pp;
> +  ptid_t::pid_type pid = 0;
> +  ptid_t::lwp_type lwp = 0;
> +  ULONGEST hex;
> +
> +  using unsigned_pid_type = std::make_unsigned<pid_type>::type;
> +  using unsigned_lwp_type = std::make_unsigned<lwp_type>::type;
> +
> +  if (*p == 'p')
> +    {
> +      /* Multi-process ptid.  */
> +      pp = unpack_varlen_hex (p + 1, &hex);
> +      if (pp == (p + 1) || *pp != '.')
> +	error (_("invalid remote ptid: %s"), buf);
> +
> +      pid = (ptid_t::pid_type) hex;
> +      if (hex != ((unsigned_pid_type) pid))
> +	error (_("invalid remote ptid: %s"), buf);
> +
> +      p = pp + 1;
> +      hex = hex_or_minus_one (p, &pp, for_remote);
> +      if (pp == p)
> +	error (_("invalid remote ptid: %s"), buf);
> +
> +      lwp = (ptid_t::lwp_type) hex;
> +      if (hex != ((unsigned_lwp_type) lwp))
> +	error (_("invalid remote ptid: %s"), buf);

Are the types/castes here correct? hex is an ULONGEST that is being
cast to an ptid_t::lwp_type and then an unsigned_lwp_type. Then this
sanity check seems to fail (on i386 at least). I think this is because
ULONGEST on i386 is unsigned long long, while unsigned_lwp_type is
unsigned long.

Cheers,

Mark

Re: [PATCH] Do not write negative PID or TID in remote protocol

[Tom Tromey]

Mark> It looks like there is still something wrong on 32bit arches.
Mark> At least after this patch the i386 debian buildbot fails:
Mark> https://builder.sourceware.org/buildbot/#/builders/105/builds/12989

I'm sorry about that.  This patch turned out to be kind of bad.

I built a -m32 gdb to reproduce this and I'll send the fix shortly.

>> +      hex = hex_or_minus_one (p, &pp, for_remote);
>> +      if (pp == p)
>> +	error (_("invalid remote ptid: %s"), buf);
>> +
>> +      lwp = (ptid_t::lwp_type) hex;
>> +      if (hex != ((unsigned_lwp_type) lwp))
>> +	error (_("invalid remote ptid: %s"), buf);

Mark> Are the types/castes here correct? hex is an ULONGEST that is being
Mark> cast to an ptid_t::lwp_type and then an unsigned_lwp_type. Then this
Mark> sanity check seems to fail (on i386 at least). I think this is because
Mark> ULONGEST on i386 is unsigned long long, while unsigned_lwp_type is
Mark> unsigned long.

Yes, I agree; and I think the fix is to special-case the "-1" value
here.

Tom

Re: [PATCH] Do not write negative PID or TID in remote protocol

[Tom Tromey]

>>>>> "Kevin" == Kevin Buettner <kevinb@redhat.com> writes:

>> This patch attempts to fix the bug.  I am not really sure how to test
>> it.
>> 
>> As an aside there seems to be more low-level RSP code that could be
>> shared here.

Kevin> LGTM.

Kevin> FWIW, Claude Opus 4.6 suggested that unit test might be employed for
Kevin> testing these changes.  I'm not sure that's necessary, but I thought I'd
Kevin> mention it.

Yeah, good advice.
And doing this pointed out a problem.  I'll send v2 shortly.

Tom

Re: [PATCH] Do not write negative PID or TID in remote protocol

[Kevin Buettner]

On Wed, 11 Mar 2026 14:21:52 -0600
Tom Tromey <tromey@adacore.com> wrote:

> From: Tom Tromey <tromey@adacore.com>
> To: gdb-patches@sourceware.org
> Cc: Tom Tromey <tromey@adacore.com>
> Subject: [PATCH] Do not write negative PID or TID in remote protocol
> Date: Wed, 11 Mar 2026 14:21:52 -0600
> 
> Currently both gdb and gdbserver can write a negative number for the
> PID or TID.  However, the only negative value that really makes sense
> is the special case of "-1" -- in other cases if the PID or TID has
> the high bit set, it should still be written as a positive number.
> 
> This patch attempts to fix the bug.  I am not really sure how to test
> it.
> 
> As an aside there seems to be more low-level RSP code that could be
> shared here.
> 
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=25111
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=33979

LGTM.

FWIW, Claude Opus 4.6 suggested that unit test might be employed for
testing these changes.  I'm not sure that's necessary, but I thought I'd
mention it.

Approved-by: Kevin Buettner <kevinb@redhat.com>

[PATCH] Do not write negative PID or TID in remote protocol

[Tom Tromey]

Currently both gdb and gdbserver can write a negative number for the
PID or TID.  However, the only negative value that really makes sense
is the special case of "-1" -- in other cases if the PID or TID has
the high bit set, it should still be written as a positive number.

This patch attempts to fix the bug.  I am not really sure how to test
it.

As an aside there seems to be more low-level RSP code that could be
shared here.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=25111
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=33979
---
 gdb/remote.c              | 19 ++++++++-----------
 gdbserver/remote-utils.cc | 12 ++++++------
 2 files changed, 14 insertions(+), 17 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index 88b06e688bc..54aeaba0ce5 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -3684,22 +3684,19 @@ static int remote_newthread_step (threadref *ref, void *context);
 char *
 remote_target::write_ptid (char *buf, const char *endbuf, ptid_t ptid)
 {
-  ptid_t::pid_type pid;
-  ptid_t::lwp_type lwp;
-
   if (m_features.remote_multi_process_p ())
     {
-      pid = ptid.pid ();
-      if (pid < 0)
-	buf += xsnprintf (buf, endbuf - buf, "p-%x.", -pid);
+      ptid_t::pid_type pid = ptid.pid ();
+      if (pid == -1)
+	buf += xsnprintf (buf, endbuf - buf, "p-1.");
       else
-	buf += xsnprintf (buf, endbuf - buf, "p%x.", pid);
+	buf += xsnprintf (buf, endbuf - buf, "p%x.", (unsigned) pid);
     }
-  lwp = ptid.lwp ();
-  if (lwp < 0)
-    buf += xsnprintf (buf, endbuf - buf, "-%lx", -lwp);
+  ptid_t::lwp_type lwp = ptid.lwp ();
+  if (lwp == -1)
+    buf += xsnprintf (buf, endbuf - buf, "-1");
   else
-    buf += xsnprintf (buf, endbuf - buf, "%lx", lwp);
+    buf += xsnprintf (buf, endbuf - buf, "%lx", (unsigned long) lwp);
 
   return buf;
 }
diff --git a/gdbserver/remote-utils.cc b/gdbserver/remote-utils.cc
index 7671206ebc1..a0ae04bd632 100644
--- a/gdbserver/remote-utils.cc
+++ b/gdbserver/remote-utils.cc
@@ -525,16 +525,16 @@ write_ptid (char *buf, ptid_t ptid)
   if (cs.multi_process)
     {
       pid = ptid.pid ();
-      if (pid < 0)
-	buf += sprintf (buf, "p-%x.", -pid);
+      if (pid == -1)
+	buf += sprintf (buf, "p-1.");
       else
-	buf += sprintf (buf, "p%x.", pid);
+	buf += sprintf (buf, "p%x.", (unsigned) pid);
     }
   lwp = ptid.lwp ();
-  if (lwp < 0)
-    buf += sprintf (buf, "-%lx", -lwp);
+  if (lwp == -1)
+    buf += sprintf (buf, "-1");
   else
-    buf += sprintf (buf, "%lx", lwp);
+    buf += sprintf (buf, "%lx", (unsigned long) lwp);
 
   return buf;
 }

base-commit: b18149c8e933d8539b8a49c5d200b1ace513e339
-- 
2.53.0