]> git.ipfire.org Git - thirdparty/binutils-gdb.git/commitdiff
Do not write negative PID or TID in remote protocol
authorTom Tromey <tromey@adacore.com>
Wed, 11 Mar 2026 20:17:50 +0000 (14:17 -0600)
committerTom Tromey <tromey@adacore.com>
Wed, 1 Jul 2026 17:47:57 +0000 (11:47 -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.

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
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=33983

gdb/remote.c
gdb/unittests/ptid-selftests.c
gdbserver/remote-utils.cc
gdbsupport/ptid.cc
gdbsupport/ptid.h

index f8c7383cb037261aed3437033b98bc4663fbf19e..33222346d063ce6be7ea98284a1eb250d5f8beb2 100644 (file)
@@ -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
index 7b8ada9500a35c7bb54a5f91e17332424c12584e..c02401db952cb21d7d2424b12500f666385ccb55 100644 (file)
@@ -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);
+}
index e99f34dfc6d2c93b4436fdd56972d6cca7602cb7..ffde495677631fe87bcabca8ca9f5c31af5b76bc 100644 (file)
@@ -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
index 78978c15c06a8f66cad4c5691413ab03250f0502..731a5bc3da3e1dcf7329095104cc044e7237b66a 100644 (file)
@@ -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);
+}
index dfc0fd77df0be3649e7a88d82b3607ae61c3b838..ef6da77eb7e799f5713495b0c74186a08ad06703 100644 (file)
@@ -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 @@ public:
            || *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 @@ public:
   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;