[PATCH 0/1] replace PBUFSIZ with a target op

[PATCH 0/1] replace PBUFSIZ with a target op

[Klaus Gerlicher]

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

Hi all,

this is my first contribution.

PBUFSIZ is meant to define the size of the packet buffer for communication with a target. It needs to be at
least 2 * tdesc->registers_size + overhead.

This #define has been increased constantly to accomodate for targets with ever increasing register sizes and
this could therefore become a target-related setting.

The accompanying patch solves this by making this a target op that will be queried from the client on
startup. It will return the current PBUFSIZ as a default for targets not yet using the new target op. Going
forward it would be a good idea to upgrade each supported target with this mechanism, so constantly changing
the global PBUFSIZ define becomes unnecessary.

The patch was tested with the testsuite on Linux x86 and x86_64.

Regards
Klaus


Gerlicher, Klaus (1):
  gdb, gdbserver: replace PBUFSIZ with a target op

 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(-)

-- 
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

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

[Klaus Gerlicher]

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

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

[Simon Marchi]

On 9/19/23 01:45, Klaus Gerlicher via Gdb-patches wrote:
> 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.

Just throwing an idea out there... instead of trying to determine a
buffer size that is large enough, can we instead pass something like an
std::vector (gdb::char_vector or gdb::byte_vector) around, and whatever
builds the packets and replies makes sure there is enough space?

Simon

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

[Gerlicher, Klaus]

Hi Simon,

Thanks for the quick response.

At least the initial buffer size needs to be fixed since now most clients aren't aware of any dynamic behavior here and therefore we need at least something pre-allocated for these clients.

For the clients interested we could change the CS buffer to a re-allocatable type that is pre-allocated with the current PBUFSIZ. We could pass this buffer around with a target op. The target could resize at will instead of letting it return a fixed number.

Is this something in line that you would support?

Klaus

-----Original Message-----
From: Simon Marchi <simon.marchi@polymtl.ca> 
Sent: Tuesday, September 19, 2023 4:08 PM
To: Gerlicher, Klaus <klaus.gerlicher@intel.com>; gdb-patches@sourceware.org
Subject: Re: [PATCH 1/1] gdb, gdbserver: replace PBUFSIZ with a target op

On 9/19/23 01:45, Klaus Gerlicher via Gdb-patches wrote:
> 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.

Just throwing an idea out there... instead of trying to determine a buffer size that is large enough, can we instead pass something like an std::vector (gdb::char_vector or gdb::byte_vector) around, and whatever builds the packets and replies makes sure there is enough space?

Simon
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

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

[Andrew Burgess]

"Gerlicher, Klaus via Gdb-patches" <gdb-patches@sourceware.org> writes:

> Hi Simon,
>
> Thanks for the quick response.
>
> At least the initial buffer size needs to be fixed since now most
> clients aren't aware of any dynamic behavior here and therefore we
> need at least something pre-allocated for these clients.

I don't understand your concerns here.  For this patch we're only
talking about the gdbserver client, right?  And your patch (rightly)
doesn't change things on the GDB side.

GDB already uses a dynamic packet buffer size.  So the only initial
buffer I think you can be talking about here is the gdbserver buffer,
which I think could be made dynamic, just as GDB's is.

We could hard-code gdbserver to return some stupidly large number for
the PacketSize in the qSupported reply, say MAX_INT?  Or (MAX_INT / 4),
you pick, this could be anything really, just something huge.

Then on the gdbserver side we can use a dynamic packet size, and we just
take care to throw an error if GDB ever tries to grow it past our
(stupidly large) max packet size -- but this doesn't mean we actually
need to pre-allocate that large of a buffer.

Alternatively, we can always extend the remote protocol with a new
reply, say 'PacketSize=*' to indicate that the client supports variable
packet buffers -- but that would require GDB to first advertise that it
understands that syntax, then clients will need to deal with GDBs that
don't support it .... and it's a whole big thing.  Maybe easier just to
work within the current framework?  Though maybe not as clean.

Now for other clients (i.e. not gdbserver) they are free to keep using a
pre-allocated buffer, and they'll send back a much smaller PaketSize,
but that should be fine.  Nothing changes for them.

> For the clients interested we could change the CS buffer to a
> re-allocatable type that is pre-allocated with the current PBUFSIZ. We
> could pass this buffer around with a target op. The target could
> resize at will instead of letting it return a fixed number.

I didn't understand this paragraph.  I don't know what 'CS buffer'
means.  And I'm not sure why we're need a target op to pass the buffer
around -- but maybe that would become obvious if I looked at the code a
little more...

Thanks,
Andrew

>
> Is this something in line that you would support?
>
> Klaus
>
> -----Original Message-----
> From: Simon Marchi <simon.marchi@polymtl.ca> 
> Sent: Tuesday, September 19, 2023 4:08 PM
> To: Gerlicher, Klaus <klaus.gerlicher@intel.com>; gdb-patches@sourceware.org
> Subject: Re: [PATCH 1/1] gdb, gdbserver: replace PBUFSIZ with a target op
>
> On 9/19/23 01:45, Klaus Gerlicher via Gdb-patches wrote:
>> 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.
>
> Just throwing an idea out there... instead of trying to determine a buffer size that is large enough, can we instead pass something like an std::vector (gdb::char_vector or gdb::byte_vector) around, and whatever builds the packets and replies makes sure there is enough space?
>
> Simon
> 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

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

[Pedro Alves]

Hi!

On 2023-09-20 13:59, Andrew Burgess via Gdb-patches wrote:
> "Gerlicher, Klaus via Gdb-patches" <gdb-patches@sourceware.org> writes:
> 
>> Hi Simon,
>>
>> Thanks for the quick response.
>>
>> At least the initial buffer size needs to be fixed since now most
>> clients aren't aware of any dynamic behavior here and therefore we
>> need at least something pre-allocated for these clients.
> 
> I don't understand your concerns here.  For this patch we're only
> talking about the gdbserver client, right?  And your patch (rightly)
> doesn't change things on the GDB side.

The client is the GDB side, the server side is, well, gdbserver.  :-)

> 
> GDB already uses a dynamic packet buffer size.  

It is dynamic, but not in the sense that we just append to the buffer
with push_back and let the buffer grow unbounded.  Instead, GDB tries to guess a
sufficient packet size, but if the server tells it explicitly what packet size it
supports, then GDB will grow its buffer to that size, no questions asked.

  /* If we increased the packet size, make sure to increase the global
     buffer size also.  We delay this until after parsing the entire
     qSupported packet, because this is the same buffer we were
     parsing.  */
  if (rs->buf.size () < rs->explicit_packet_size)
    rs->buf.resize (rs->explicit_packet_size);


> So the only initial
> buffer I think you can be talking about here is the gdbserver buffer,
> which I think could be made dynamic, just as GDB's is.

I think he was really talking about the GDB side.  Or even other
clients, like LLDB, etc.

> 
> We could hard-code gdbserver to return some stupidly large number for
> the PacketSize in the qSupported reply, say MAX_INT?  Or (MAX_INT / 4),
> you pick, this could be anything really, just something huge.

I don't think it can, due to the immediate resize mentioned above.

I don't understand why the patch added a target method on the gdb side.

Also, do we really need the new target method on the gdbserver side?
We assert that the buffer size is bigger than the tdesc's register size plus
a slack, but how about flipping that around and make the buffer size be dependent
on the register size?  Maybe the packet size decision is done earlier than we
know which tdesc we are using, though, that's something to check.

Pedro Alves

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

[Gerlicher, Klaus]

Hi,

I'm unsure why I'm confusing you about what this does. This is a patch for remote target packet buffer, so for gdbserver.

From original gdbserver/server.h:

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

PBUFSIZ is defined as a constant( #define) and is used for allocating the buffer that gdbserver writes into for communication with GDB. 

This will remain the same size for targets not aware of the process_stratum_pbufsiz::query_pbuf_size override. I called this a target op but maybe that's the wrong term and that is the source of confusion?

Targets can override this to any size they see fit for what register size or memory transfer sizes they require. It doesn't make sense to have big allocation for targets that don't need that much but some newer accelerator/SIMD/GPU device (for a lack of a better term) targets need a much bigger buffer.

Thanks
Klaus

-----Original Message-----
From: Pedro Alves <pedro@palves.net> 
Sent: Wednesday, September 20, 2023 6:32 PM
To: Andrew Burgess <aburgess@redhat.com>; Gerlicher, Klaus <klaus.gerlicher@intel.com>; Simon Marchi <simon.marchi@polymtl.ca>; gdb-patches@sourceware.org
Subject: Re: [PATCH 1/1] gdb, gdbserver: replace PBUFSIZ with a target op

Hi!

On 2023-09-20 13:59, Andrew Burgess via Gdb-patches wrote:
> "Gerlicher, Klaus via Gdb-patches" <gdb-patches@sourceware.org> writes:
> 
>> Hi Simon,
>>
>> Thanks for the quick response.
>>
>> At least the initial buffer size needs to be fixed since now most 
>> clients aren't aware of any dynamic behavior here and therefore we 
>> need at least something pre-allocated for these clients.
> 
> I don't understand your concerns here.  For this patch we're only 
> talking about the gdbserver client, right?  And your patch (rightly) 
> doesn't change things on the GDB side.

The client is the GDB side, the server side is, well, gdbserver.  :-)

> 
> GDB already uses a dynamic packet buffer size.  

It is dynamic, but not in the sense that we just append to the buffer with push_back and let the buffer grow unbounded.  Instead, GDB tries to guess a sufficient packet size, but if the server tells it explicitly what packet size it supports, then GDB will grow its buffer to that size, no questions asked.

  /* If we increased the packet size, make sure to increase the global
     buffer size also.  We delay this until after parsing the entire
     qSupported packet, because this is the same buffer we were
     parsing.  */
  if (rs->buf.size () < rs->explicit_packet_size)
    rs->buf.resize (rs->explicit_packet_size);


> So the only initial
> buffer I think you can be talking about here is the gdbserver buffer, 
> which I think could be made dynamic, just as GDB's is.

I think he was really talking about the GDB side.  Or even other clients, like LLDB, etc.

> 
> We could hard-code gdbserver to return some stupidly large number for 
> the PacketSize in the qSupported reply, say MAX_INT?  Or (MAX_INT / 
> 4), you pick, this could be anything really, just something huge.

I don't think it can, due to the immediate resize mentioned above.

I don't understand why the patch added a target method on the gdb side.

Also, do we really need the new target method on the gdbserver side?
We assert that the buffer size is bigger than the tdesc's register size plus a slack, but how about flipping that around and make the buffer size be dependent on the register size?  Maybe the packet size decision is done earlier than we know which tdesc we are using, though, that's something to check.

Pedro Alves

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

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

[Andrew Burgess]

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

> Hi,
>
> I'm unsure why I'm confusing you about what this does. This is a patch for remote target packet buffer, so for gdbserver.
>
> From original gdbserver/server.h:
>
> /* 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.  */
>
> PBUFSIZ is defined as a constant( #define) and is used for allocating the buffer that gdbserver writes into for communication with GDB. 
>
> This will remain the same size for targets not aware of the
> process_stratum_pbufsiz::query_pbuf_size override. I called this a
> target op but maybe that's the wrong term and that is the source of
> confusion?

No target_ops is the correct name (so target ops is fine).  For me, I
think you should drop all of the changes in the gdb/ tree, leaving just
the gdbserver/ changes.

GDB should not need to know about PBUFSIZ, and indeed, there's just one
reference to it, in a comment, and this should be rewritten to indicate
that the maximum size being referenced is the one that the server passed
us in the qSupported packet.

> Targets can override this to any size they see fit for what register
> size or memory transfer sizes they require. It doesn't make sense to
> have big allocation for targets that don't need that much but some
> newer accelerator/SIMD/GPU device (for a lack of a better term)
> targets need a much bigger buffer.

Agreed.  It would be interesting to see if we could derive the required
size from the tdesc as Pedro suggested.

Thanks,
Andrew

>
> Thanks
> Klaus
>
> -----Original Message-----
> From: Pedro Alves <pedro@palves.net> 
> Sent: Wednesday, September 20, 2023 6:32 PM
> To: Andrew Burgess <aburgess@redhat.com>; Gerlicher, Klaus <klaus.gerlicher@intel.com>; Simon Marchi <simon.marchi@polymtl.ca>; gdb-patches@sourceware.org
> Subject: Re: [PATCH 1/1] gdb, gdbserver: replace PBUFSIZ with a target op
>
> Hi!
>
> On 2023-09-20 13:59, Andrew Burgess via Gdb-patches wrote:
>> "Gerlicher, Klaus via Gdb-patches" <gdb-patches@sourceware.org> writes:
>> 
>>> Hi Simon,
>>>
>>> Thanks for the quick response.
>>>
>>> At least the initial buffer size needs to be fixed since now most 
>>> clients aren't aware of any dynamic behavior here and therefore we 
>>> need at least something pre-allocated for these clients.
>> 
>> I don't understand your concerns here.  For this patch we're only 
>> talking about the gdbserver client, right?  And your patch (rightly) 
>> doesn't change things on the GDB side.
>
> The client is the GDB side, the server side is, well, gdbserver.  :-)
>
>> 
>> GDB already uses a dynamic packet buffer size.  
>
> It is dynamic, but not in the sense that we just append to the buffer with push_back and let the buffer grow unbounded.  Instead, GDB tries to guess a sufficient packet size, but if the server tells it explicitly what packet size it supports, then GDB will grow its buffer to that size, no questions asked.
>
>   /* If we increased the packet size, make sure to increase the global
>      buffer size also.  We delay this until after parsing the entire
>      qSupported packet, because this is the same buffer we were
>      parsing.  */
>   if (rs->buf.size () < rs->explicit_packet_size)
>     rs->buf.resize (rs->explicit_packet_size);
>
>
>> So the only initial
>> buffer I think you can be talking about here is the gdbserver buffer, 
>> which I think could be made dynamic, just as GDB's is.
>
> I think he was really talking about the GDB side.  Or even other clients, like LLDB, etc.
>
>> 
>> We could hard-code gdbserver to return some stupidly large number for 
>> the PacketSize in the qSupported reply, say MAX_INT?  Or (MAX_INT / 
>> 4), you pick, this could be anything really, just something huge.
>
> I don't think it can, due to the immediate resize mentioned above.
>
> I don't understand why the patch added a target method on the gdb side.
>
> Also, do we really need the new target method on the gdbserver side?
> We assert that the buffer size is bigger than the tdesc's register size plus a slack, but how about flipping that around and make the buffer size be dependent on the register size?  Maybe the packet size decision is done earlier than we know which tdesc we are using, though, that's something to check.
>
> Pedro Alves
>
> 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

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

[Pedro Alves]

Hi!

I am not sure who you are replying to.  It would be great if you please avoided top posting.

On 2023-09-21 07:02, Gerlicher, Klaus wrote:
> Hi,
> 
> I'm unsure why I'm confusing you about what this does. This is a patch for remote target packet buffer, so for gdbserver.

I had said:

  > I don't understand why the patch added a target method on the gdb side.

Is this what you are replying to?  You are describing this as a gdbserver patch, but the
patch touches the gdb side as well.  That's what I was asking about.  Why are these changes
needed:

 gdb/remote.h             |  8 +++---
 gdb/target-delegates.c   | 31 +++++++++++++++++++++--
 gdb/target.h             |  7 ++++++

I then had another separate question.

Pedro Alves

> 
> From original gdbserver/server.h:
> 
> /* 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.  */
> 
> PBUFSIZ is defined as a constant( #define) and is used for allocating the buffer that gdbserver writes into for communication with GDB. 
> 
> This will remain the same size for targets not aware of the process_stratum_pbufsiz::query_pbuf_size override. I called this a target op but maybe that's the wrong term and that is the source of confusion?
> 
> Targets can override this to any size they see fit for what register size or memory transfer sizes they require. It doesn't make sense to have big allocation for targets that don't need that much but some newer accelerator/SIMD/GPU device (for a lack of a better term) targets need a much bigger buffer.
> 
> Thanks
> Klaus
> 
> -----Original Message-----
> From: Pedro Alves <pedro@palves.net> 
> Sent: Wednesday, September 20, 2023 6:32 PM
> To: Andrew Burgess <aburgess@redhat.com>; Gerlicher, Klaus <klaus.gerlicher@intel.com>; Simon Marchi <simon.marchi@polymtl.ca>; gdb-patches@sourceware.org
> Subject: Re: [PATCH 1/1] gdb, gdbserver: replace PBUFSIZ with a target op
> 
> Hi!
> 
> On 2023-09-20 13:59, Andrew Burgess via Gdb-patches wrote:
>> "Gerlicher, Klaus via Gdb-patches" <gdb-patches@sourceware.org> writes:
>>
>>> Hi Simon,
>>>
>>> Thanks for the quick response.
>>>
>>> At least the initial buffer size needs to be fixed since now most 
>>> clients aren't aware of any dynamic behavior here and therefore we 
>>> need at least something pre-allocated for these clients.
>>
>> I don't understand your concerns here.  For this patch we're only 
>> talking about the gdbserver client, right?  And your patch (rightly) 
>> doesn't change things on the GDB side.
> 
> The client is the GDB side, the server side is, well, gdbserver.  :-)
> 
>>
>> GDB already uses a dynamic packet buffer size.  
> 
> It is dynamic, but not in the sense that we just append to the buffer with push_back and let the buffer grow unbounded.  Instead, GDB tries to guess a sufficient packet size, but if the server tells it explicitly what packet size it supports, then GDB will grow its buffer to that size, no questions asked.
> 
>   /* If we increased the packet size, make sure to increase the global
>      buffer size also.  We delay this until after parsing the entire
>      qSupported packet, because this is the same buffer we were
>      parsing.  */
>   if (rs->buf.size () < rs->explicit_packet_size)
>     rs->buf.resize (rs->explicit_packet_size);
> 
> 
>> So the only initial
>> buffer I think you can be talking about here is the gdbserver buffer, 
>> which I think could be made dynamic, just as GDB's is.
> 
> I think he was really talking about the GDB side.  Or even other clients, like LLDB, etc.
> 
>>
>> We could hard-code gdbserver to return some stupidly large number for 
>> the PacketSize in the qSupported reply, say MAX_INT?  Or (MAX_INT / 
>> 4), you pick, this could be anything really, just something huge.
> 
> I don't think it can, due to the immediate resize mentioned above.
> 
> I don't understand why the patch added a target method on the gdb side.
> 
> Also, do we really need the new target method on the gdbserver side?
> We assert that the buffer size is bigger than the tdesc's register size plus a slack, but how about flipping that around and make the buffer size be dependent on the register size?  Maybe the packet size decision is done earlier than we know which tdesc we are using, though, that's something to check.
> 
> Pedro Alves
> 
> 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