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

[Tom Tromey <tromey@adacore.com>]

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