[PATCH 1/1] gdb, gdbserver: replace PBUFSIZ with a target op

[Klaus Gerlicher <klaus.gerlicher@intel.com>]

From: "Gerlicher, Klaus" <klaus.gerlicher@intel.com>

PBUFSIZ is a hardcoded value that needs to be as least twice as big as
the register set.

This means that this could be tailored to the target register size as
not all targets need a big buffer. For SIMD targets the buffer will
obviously be much bigger than for regular scalar targets.

This patch changes this and adds a target op query_pbuf_size (). It also
moves the static global client_state into the get_client_state ()
function and lazily dynamically allocates storage for its own_buf.
---
 gdb/remote.h             |  8 +++---
 gdb/target-delegates.c   | 31 +++++++++++++++++++++--
 gdb/target.h             |  7 ++++++
 gdbserver/hostio.cc      | 54 ++++++++++++++++++++++++----------------
 gdbserver/notif.cc       |  8 +++---
 gdbserver/server.cc      | 26 +++++++++++--------
 gdbserver/server.h       | 31 ++++++++++++++++-------
 gdbserver/target.cc      |  9 +++++++
 gdbserver/target.h       |  9 +++++++
 gdbserver/tdesc.cc       | 11 +++++---
 gdbserver/tracepoint.cc  |  4 +--
 gdbsupport/common-defs.h |  5 ++++
 12 files changed, 146 insertions(+), 57 deletions(-)

diff --git a/gdb/remote.h b/gdb/remote.h
index 74366cdbcfb..b09b8c89663 100644
--- a/gdb/remote.h
+++ b/gdb/remote.h
@@ -56,10 +56,10 @@ extern void getpkt (remote_target *remote,
 		    char **buf, long *sizeof_buf, int forever);
 
 /* Send a packet to the remote machine, with error checking.  The data
-   of the packet is in BUF.  The string in BUF can be at most PBUFSIZ
-   - 5 to account for the $, # and checksum, and for a possible /0 if
-   we are debugging (remote_debug) and want to print the sent packet
-   as a string.  */
+   of the packet is in BUF.  The string in BUF can be at most
+   target_query_pbuf_size () - 5 to account for the $, # and checksum,
+   and for a possible /0 if we are debugging (remote_debug) and want to
+   print the sent packet as a string.  */
 
 extern int putpkt (remote_target *remote, const char *buf);
 
diff --git a/gdb/target-delegates.c b/gdb/target-delegates.c
index 8b066249624..bbb9614fcb2 100644
--- a/gdb/target-delegates.c
+++ b/gdb/target-delegates.c
@@ -197,6 +197,7 @@ struct dummy_target : public target_ops
   bool fetch_memtags (CORE_ADDR arg0, size_t arg1, gdb::byte_vector &arg2, int arg3) override;
   bool store_memtags (CORE_ADDR arg0, size_t arg1, const gdb::byte_vector &arg2, int arg3) override;
   x86_xsave_layout fetch_x86_xsave_layout () override;
+  int query_pbuf_size () override;
 };
 
 struct debug_target : public target_ops
@@ -372,6 +373,7 @@ struct debug_target : public target_ops
   bool fetch_memtags (CORE_ADDR arg0, size_t arg1, gdb::byte_vector &arg2, int arg3) override;
   bool store_memtags (CORE_ADDR arg0, size_t arg1, const gdb::byte_vector &arg2, int arg3) override;
   x86_xsave_layout fetch_x86_xsave_layout () override;
+  int query_pbuf_size () override;
 };
 
 void
@@ -4553,12 +4555,37 @@ dummy_target::fetch_x86_xsave_layout ()
 x86_xsave_layout
 debug_target::fetch_x86_xsave_layout ()
 {
-  x86_xsave_layout result;
   gdb_printf (gdb_stdlog, "-> %s->fetch_x86_xsave_layout (...)\n", this->beneath ()->shortname ());
-  result = this->beneath ()->fetch_x86_xsave_layout ();
+  x86_xsave_layout result
+    = this->beneath ()->fetch_x86_xsave_layout ();
   gdb_printf (gdb_stdlog, "<- %s->fetch_x86_xsave_layout (", this->beneath ()->shortname ());
   gdb_puts (") = ", gdb_stdlog);
   target_debug_print_x86_xsave_layout (result);
   gdb_puts ("\n", gdb_stdlog);
   return result;
 }
+
+int
+target_ops::query_pbuf_size ()
+{
+  return this->beneath ()->query_pbuf_size ();
+}
+
+int
+dummy_target::query_pbuf_size ()
+{
+  return PBUFSIZ;
+}
+
+int
+debug_target::query_pbuf_size ()
+{
+  gdb_printf (gdb_stdlog, "-> %s->query_pbuf_size (...)\n", this->beneath ()->shortname ());
+  int result
+    = this->beneath ()->query_pbuf_size ();
+  gdb_printf (gdb_stdlog, "<- %s->query_pbuf_size (", this->beneath ()->shortname ());
+  gdb_puts (") = ", gdb_stdlog);
+  target_debug_print_int (result);
+  gdb_puts ("\n", gdb_stdlog);
+  return result;
+}
diff --git a/gdb/target.h b/gdb/target.h
index aa07075f9f2..8a2cc0a8b24 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -1330,6 +1330,10 @@ struct target_ops
     /* Return the x86 XSAVE extended state area layout.  */
     virtual x86_xsave_layout fetch_x86_xsave_layout ()
       TARGET_DEFAULT_RETURN (x86_xsave_layout ());
+
+    /* Query PBUFSIZ from target instead of hard-coded #define'ing it.  */
+    virtual int query_pbuf_size ()
+      TARGET_DEFAULT_RETURN (PBUFSIZ);
   };
 
 /* Deleter for std::unique_ptr.  See comments in
@@ -2573,4 +2577,7 @@ extern void target_prepare_to_generate_core (void);
 /* See to_done_generating_core.  */
 extern void target_done_generating_core (void);
 
+/* See target_ops::query_pbuf_size.  */
+extern int target_query_pbuf_size ();
+
 #endif /* !defined (TARGET_H) */
diff --git a/gdbserver/hostio.cc b/gdbserver/hostio.cc
index ea70c26da0f..6092a33ec02 100644
--- a/gdbserver/hostio.cc
+++ b/gdbserver/hostio.cc
@@ -28,6 +28,7 @@
 #include <sys/types.h>
 #include <sys/stat.h>
 #include "gdbsupport/fileio.h"
+#include <string>
 
 struct fd_list
 {
@@ -54,11 +55,17 @@ safe_fromhex (char a, int *nibble)
 
 /* Filenames are hex encoded, so the maximum we can handle is half the
    packet buffer size.  Cap to PATH_MAX, if it is shorter.  */
-#if !defined (PATH_MAX) || (PATH_MAX > (PBUFSIZ / 2 + 1))
-#  define HOSTIO_PATH_MAX (PBUFSIZ / 2 + 1)
+static int
+hostio_path_max ()
+{
+#if !defined (PATH_MAX)
+  return target_query_pbuf_size () / 2 + 1;
 #else
-#  define HOSTIO_PATH_MAX PATH_MAX
+  return (PATH_MAX > (target_query_pbuf_size () / 2 + 1))
+	 ? target_query_pbuf_size () / 2 + 1
+	 : PATH_MAX;
 #endif
+}
 
 static int
 require_filename (char **pp, char *filename)
@@ -74,7 +81,7 @@ require_filename (char **pp, char *filename)
       int nib1, nib2;
 
       /* Don't allow overflow.  */
-      if (count >= HOSTIO_PATH_MAX - 1)
+      if (count >= hostio_path_max () - 1)
 	return -1;
 
       if (safe_fromhex (p[0], &nib1)
@@ -222,7 +229,7 @@ hostio_reply_with_data (char *own_buf, char *buffer, int len,
   sprintf (own_buf, "F%x;", len);
   output_index = strlen (own_buf);
 
-  out_maxlen = PBUFSIZ;
+  out_maxlen = target_query_pbuf_size ();
 
   for (input_index = 0; input_index < len; input_index++)
     {
@@ -298,7 +305,7 @@ handle_setfs (char *own_buf)
 static void
 handle_open (char *own_buf)
 {
-  char filename[HOSTIO_PATH_MAX];
+  std::vector<char> filename (hostio_path_max ());
   char *p;
   int fileio_flags, fileio_mode, flags, fd;
   mode_t mode;
@@ -306,7 +313,7 @@ handle_open (char *own_buf)
 
   p = own_buf + strlen ("vFile:open:");
 
-  if (require_filename (&p, filename)
+  if (require_filename (&p, filename.data ())
       || require_comma (&p)
       || require_int (&p, &fileio_flags)
       || require_comma (&p)
@@ -322,9 +329,10 @@ handle_open (char *own_buf)
   /* We do not need to convert MODE, since the fileio protocol
      uses the standard values.  */
   if (hostio_fs_pid != 0)
-    fd = the_target->multifs_open (hostio_fs_pid, filename, flags, mode);
+    fd = the_target->multifs_open (hostio_fs_pid, filename.data (), flags,
+				   mode);
   else
-    fd = open (filename, flags, mode);
+    fd = open (filename.data (), flags, mode);
 
   if (fd == -1)
     {
@@ -367,8 +375,8 @@ handle_pread (char *own_buf, int *new_packet_len)
      too much because of escaping, but this is handled below.  */
   if (max_reply_size == -1)
     {
-      sprintf (own_buf, "F%x;", PBUFSIZ);
-      max_reply_size = PBUFSIZ - strlen (own_buf);
+      sprintf (own_buf, "F%x;", target_query_pbuf_size ());
+      max_reply_size = target_query_pbuf_size () - strlen (own_buf);
     }
   if (len > max_reply_size)
     len = max_reply_size;
@@ -527,13 +535,13 @@ handle_close (char *own_buf)
 static void
 handle_unlink (char *own_buf)
 {
-  char filename[HOSTIO_PATH_MAX];
+  std::vector<char> filename (hostio_path_max ());
   char *p;
   int ret;
 
   p = own_buf + strlen ("vFile:unlink:");
 
-  if (require_filename (&p, filename)
+  if (require_filename (&p, filename.data ())
       || require_end (p))
     {
       hostio_packet_error (own_buf);
@@ -541,9 +549,9 @@ handle_unlink (char *own_buf)
     }
 
   if (hostio_fs_pid != 0)
-    ret = the_target->multifs_unlink (hostio_fs_pid, filename);
+    ret = the_target->multifs_unlink (hostio_fs_pid, filename.data ());
   else
-    ret = unlink (filename);
+    ret = unlink (filename.data ());
 
   if (ret == -1)
     {
@@ -557,13 +565,14 @@ handle_unlink (char *own_buf)
 static void
 handle_readlink (char *own_buf, int *new_packet_len)
 {
-  char filename[HOSTIO_PATH_MAX], linkname[HOSTIO_PATH_MAX];
+  std::vector<char> filename (hostio_path_max ());
+  std::vector<char> linkname (hostio_path_max ());
   char *p;
   int ret, bytes_sent;
 
   p = own_buf + strlen ("vFile:readlink:");
 
-  if (require_filename (&p, filename)
+  if (require_filename (&p, filename.data ())
       || require_end (p))
     {
       hostio_packet_error (own_buf);
@@ -571,11 +580,11 @@ handle_readlink (char *own_buf, int *new_packet_len)
     }
 
   if (hostio_fs_pid != 0)
-    ret = the_target->multifs_readlink (hostio_fs_pid, filename,
-					linkname,
-					sizeof (linkname) - 1);
+    ret = the_target->multifs_readlink (hostio_fs_pid, filename.data (),
+					linkname.data (),
+					linkname.size () - 1);
   else
-    ret = readlink (filename, linkname, sizeof (linkname) - 1);
+    ret = readlink (filename.data (), linkname.data (), linkname.size ());
 
   if (ret == -1)
     {
@@ -583,7 +592,8 @@ handle_readlink (char *own_buf, int *new_packet_len)
       return;
     }
 
-  bytes_sent = hostio_reply_with_data (own_buf, linkname, ret, new_packet_len);
+  bytes_sent = hostio_reply_with_data (own_buf, linkname.data (), ret,
+				       new_packet_len);
 
   /* If the response does not fit into a single packet, do not attempt
      to return a partial response, but simply fail.  */
diff --git a/gdbserver/notif.cc b/gdbserver/notif.cc
index 9b323b4a869..3fc93ac2e69 100644
--- a/gdbserver/notif.cc
+++ b/gdbserver/notif.cc
@@ -140,13 +140,13 @@ notif_push (struct notif_server *np, struct notif_event *new_event)
      about it, by sending a corresponding notification.  */
   if (is_first_event)
     {
-      char buf[PBUFSIZ];
-      char *p = buf;
+      std::vector<char> buf (target_query_pbuf_size ());
+      char *p = buf.data ();
 
-      xsnprintf (p, PBUFSIZ, "%s:", np->notif_name);
+      xsnprintf (p, target_query_pbuf_size (), "%s:", np->notif_name);
       p += strlen (p);
 
       np->write (new_event, p);
-      putpkt_notif (buf);
+      putpkt_notif (buf.data ());
     }
 }
diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index c57270175b4..1314db953c3 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -161,13 +161,17 @@ static struct btrace_config current_btrace_conf;
 
 /* The client remote protocol state. */
 
-static client_state g_client_state;
-
 client_state &
 get_client_state ()
 {
-  client_state &cs = g_client_state;
-  return cs;
+  static client_state g_client_state;
+
+  /* Lazily allocate ownbuf as clientstate is only ever used once a target is
+     available.  */
+  if (the_target != nullptr && g_client_state.own_buf == nullptr)
+      g_client_state.alloc_own_buf ();
+
+  return g_client_state;
 }
 
 
@@ -398,7 +402,7 @@ write_qxfer_response (char *buf, const gdb_byte *data, int len, int is_more)
     buf[0] = 'l';
 
   return remote_escape_output (data, len, 1, (unsigned char *) buf + 1,
-			       &out_len, PBUFSIZ - 2) + 1;
+			       &out_len, target_query_pbuf_size () - 2) + 1;
 }
 
 /* Handle btrace enabling in BTS format.  */
@@ -573,7 +577,7 @@ create_fetch_memtags_reply (char *reply, const gdb::byte_vector &tags)
   packet += bin2hex (tags.data (), tags.size ());
 
   /* Check if the reply is too big for the packet to handle.  */
-  if (PBUFSIZ < packet.size ())
+  if (target_query_pbuf_size () < packet.size ())
     return false;
 
   strcpy (reply, packet.c_str ());
@@ -2018,8 +2022,8 @@ handle_qxfer (char *own_buf, int packet_len, int *new_packet_len_p)
 
 	      /* Read one extra byte, as an indicator of whether there is
 		 more.  */
-	      if (len > PBUFSIZ - 2)
-		len = PBUFSIZ - 2;
+	      if (len > target_query_pbuf_size () - 2)
+		len = target_query_pbuf_size () - 2;
 	      data = (unsigned char *) malloc (len + 1);
 	      if (data == NULL)
 		{
@@ -2376,7 +2380,7 @@ handle_query (char *own_buf, int packet_len, int *new_packet_len_p)
 	       "QStartupWithShell+;QEnvironmentHexEncoded+;"
 	       "QEnvironmentReset+;QEnvironmentUnset+;"
 	       "QSetWorkingDir+",
-	       PBUFSIZ - 1);
+	       target_query_pbuf_size () - 1);
 
       if (target_supports_catch_syscall ())
 	strcat (own_buf, ";QCatchSyscalls+");
@@ -2578,7 +2582,7 @@ handle_query (char *own_buf, int packet_len, int *new_packet_len_p)
   /* Handle "monitor" commands.  */
   if (startswith (own_buf, "qRcmd,"))
     {
-      char *mon = (char *) malloc (PBUFSIZ);
+      char *mon = (char *) malloc (target_query_pbuf_size ());
       int len = strlen (own_buf + 6);
 
       if (mon == NULL)
@@ -3894,7 +3898,7 @@ captured_main (int argc, char *argv[])
   if (target_supports_tracepoints ())
     initialize_tracepoint ();
 
-  mem_buf = (unsigned char *) xmalloc (PBUFSIZ);
+  mem_buf = (unsigned char *) xmalloc (target_query_pbuf_size ());
 
   if (selftest)
     {
diff --git a/gdbserver/server.h b/gdbserver/server.h
index fde7dfe7060..2122c9e9429 100644
--- a/gdbserver/server.h
+++ b/gdbserver/server.h
@@ -101,11 +101,6 @@ extern int in_queued_stop_replies (ptid_t ptid);
    is chosen to fill up a packet (the headers account for the 32).  */
 #define MAXBUFBYTES(N) (((N)-32)/2)
 
-/* Buffer sizes for transferring memory, registers, etc.   Set to a constant
-   value to accommodate multiple register formats.  This value must be at least
-   as large as the largest register set supported by gdbserver.  */
-#define PBUFSIZ 18432
-
 /* Definition for an unknown syscall, used basically in error-cases.  */
 #define UNKNOWN_SYSCALL (-1)
 
@@ -129,9 +124,27 @@ extern unsigned long signal_pid;
 
 struct client_state
 {
-  client_state ():
-    own_buf ((char *) xmalloc (PBUFSIZ + 1)) 
-  {}
+  client_state () = default;
+
+  client_state (const client_state& other) = delete;
+  client_state (const client_state&& other) = delete;
+  client_state& operator=(const client_state& other) = delete;
+  client_state& operator=(const client_state&& other) noexcept = delete;
+
+  virtual ~client_state ()
+  {
+    xfree (own_buf);
+  }
+
+  /* Lazily allocate own_buf.  This buffer is allocated one time only as soon
+     as we can query the target for the size information.  */
+  void alloc_own_buf ()
+  {
+    /* We are asserting because we initially expect no allocation.  */
+    gdb_assert (own_buf == nullptr);
+    own_buf = (char *) xmalloc (target_query_pbuf_size () + 1);
+    gdb_assert (own_buf != nullptr);
+  }
 
   /* The thread set with an `Hc' packet.  `Hc' is deprecated in favor of
      `vCont'.  Note the multi-process extensions made `vCont' a
@@ -177,7 +190,7 @@ struct client_state
   struct target_waitstatus last_status;
   ptid_t last_ptid;
 
-  char *own_buf;
+  char *own_buf = nullptr;
 
   /* If true, then GDB has requested noack mode.  */
   int noack_mode = 0;
diff --git a/gdbserver/target.cc b/gdbserver/target.cc
index f8e592d20c3..1de513aca9c 100644
--- a/gdbserver/target.cc
+++ b/gdbserver/target.cc
@@ -832,3 +832,12 @@ process_stratum_target::get_ipa_tdesc_idx ()
 {
   return 0;
 }
+
+int
+process_stratum_target::query_pbuf_size ()
+{
+  /* Buffer sizes for transferring memory, registers, etc.  The target decides
+     how big this needs to be but this value must be at least as large as the
+     largest register set supported by gdbserver.  */
+  return PBUFSIZ;
+}
diff --git a/gdbserver/target.h b/gdbserver/target.h
index d993e361b76..7815be7f53c 100644
--- a/gdbserver/target.h
+++ b/gdbserver/target.h
@@ -508,6 +508,9 @@ class process_stratum_target
      Returns true if successful and false otherwise.  */
   virtual bool store_memtags (CORE_ADDR address, size_t len,
 			      const gdb::byte_vector &tags, int type);
+
+  /* Query packet buffer size.  */
+  virtual int query_pbuf_size ();
 };
 
 extern process_stratum_target *the_target;
@@ -699,6 +702,12 @@ target_thread_pending_child (thread_info *thread)
   return the_target->thread_pending_child (thread);
 }
 
+static inline int
+target_query_pbuf_size ()
+{
+  return the_target->query_pbuf_size ();
+}
+
 /* Read LEN bytes from MEMADDR in the buffer MYADDR.  Return 0 if the read
    is successful, otherwise, return a non-zero error code.  */
 
diff --git a/gdbserver/tdesc.cc b/gdbserver/tdesc.cc
index 2c7257c458f..0f48a43e643 100644
--- a/gdbserver/tdesc.cc
+++ b/gdbserver/tdesc.cc
@@ -16,6 +16,7 @@
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
 #include "server.h"
+#include "target.h"
 #include "tdesc.h"
 #include "regdef.h"
 
@@ -84,12 +85,16 @@ init_target_desc (struct target_desc *tdesc,
 
   tdesc->registers_size = offset / 8;
 
-  /* Make sure PBUFSIZ is large enough to hold a full register
+#ifndef IN_PROCESS_AGENT
+  /* Make sure target packet buffer  is large enough to hold a full register
      packet.  */
-  gdb_assert (2 * tdesc->registers_size + 32 <= PBUFSIZ);
+  gdb_assert (2 * tdesc->registers_size + 32 <= target_query_pbuf_size ());
 
-#ifndef IN_PROCESS_AGENT
   tdesc->expedite_regs = expedite_regs;
+#else
+  /* Make sure PBUFSIZ is large enough to hold a full register
+     packet.  */
+  gdb_assert (2 * tdesc->registers_size + 32 <= PBUFSIZ);
 #endif
 }
 
diff --git a/gdbserver/tracepoint.cc b/gdbserver/tracepoint.cc
index 609d49a87ef..b7ec9e1bf25 100644
--- a/gdbserver/tracepoint.cc
+++ b/gdbserver/tracepoint.cc
@@ -4018,8 +4018,8 @@ cmd_qtbuffer (char *own_buf)
     num = tot - offset;
 
   /* Trim to available packet size.  */
-  if (num >= (PBUFSIZ - 16) / 2 )
-    num = (PBUFSIZ - 16) / 2;
+  if (num >= (target_query_pbuf_size () - 16) / 2)
+    num = (target_query_pbuf_size () - 16) / 2;
 
   bin2hex (tbp, own_buf, num);
 }
diff --git a/gdbsupport/common-defs.h b/gdbsupport/common-defs.h
index 72c1c5144f3..462ce00ed04 100644
--- a/gdbsupport/common-defs.h
+++ b/gdbsupport/common-defs.h
@@ -230,4 +230,9 @@
 #define HAVE_USEFUL_SBRK 1
 #endif
 
+/* The default packet buffer size for transferring memory, registers, etc.  If a
+ * target needs a bigger (or smaller) buffer, it should implement the target op
+ * query_pbuf_size ().  */
+#define PBUFSIZ 18432
+
 #endif /* COMMON_COMMON_DEFS_H */
-- 
2.34.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928