gdb: remove packet size limit

Message ID 1440541673-31794-1-git-send-email-palves@redhat.com
State New, archived
Headers

Commit Message

Pedro Alves Aug. 25, 2015, 10:27 p.m. UTC
  The remote packet buffer size is currently capped to 16384 mostly for
historical reasons, related to use of alloca.  Stop using alloca and
remove the limitation.

Tested on x86_64 Fedora 20.

gdb/ChangeLog:
2015-08-25  Pedro Alves  <palves@redhat.com>

	* remote.c (DEFAULT_MAX_MEMORY_PACKET_SIZE)
	(MIN_MEMORY_PACKET_SIZE): New.
	(MAX_REMOTE_PACKET_SIZE, MIN_REMOTE_PACKET_SIZE): Delete.
	(get_memory_packet_size): Adjust.  No longer limit the max packet
	size.
	(set_memory_packet_size): Adjust, and remove dead code.
	(remote_check_symbols): Use xmalloc and a cleanup instead of
	alloca.
	(remote_packet_size): No longer cap the packet size.
	(putpkt_binary): Use xmalloc and a cleanup instead of alloca.
---
 gdb/remote.c | 69 ++++++++++++++++++++++++++++++------------------------------
 1 file changed, 35 insertions(+), 34 deletions(-)
  

Comments

Gary Benson Aug. 26, 2015, 8:43 a.m. UTC | #1
Pedro Alves wrote:
> The remote packet buffer size is currently capped to 16384 mostly for
> historical reasons, related to use of alloca.  Stop using alloca and
> remove the limitation.

Nice.

> diff --git a/gdb/remote.c b/gdb/remote.c
> index e019277..e00e67a 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -920,6 +920,16 @@ struct memory_packet_config
>    int fixed_p;
>  };
>  
> +/* The default max memory-write-packet-size.  The 16k is historical.
> +   (It came from older GDB's using alloca for buffers and the
> +   knowledge (folk law?) that some hosts don't cope very well with

It's "folklore".

Thanks,
Gary
  

Patch

diff --git a/gdb/remote.c b/gdb/remote.c
index e019277..e00e67a 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -920,6 +920,16 @@  struct memory_packet_config
   int fixed_p;
 };
 
+/* The default max memory-write-packet-size.  The 16k is historical.
+   (It came from older GDB's using alloca for buffers and the
+   knowledge (folk law?) that some hosts don't cope very well with
+   large alloca() calls.)  */
+#define DEFAULT_MAX_MEMORY_PACKET_SIZE 16384
+
+/* The minimum remote packet size for memory transfers.  Ensures we
+   can write at least one byte.  */
+#define MIN_MEMORY_PACKET_SIZE 20
+
 /* Compute the current size of a read/write packet.  Since this makes
    use of ``actual_register_packet_size'' the computation is dynamic.  */
 
@@ -929,23 +939,11 @@  get_memory_packet_size (struct memory_packet_config *config)
   struct remote_state *rs = get_remote_state ();
   struct remote_arch_state *rsa = get_remote_arch_state ();
 
-  /* NOTE: The somewhat arbitrary 16k comes from the knowledge (folk
-     law?) that some hosts don't cope very well with large alloca()
-     calls.  Eventually the alloca() code will be replaced by calls to
-     xmalloc() and make_cleanups() allowing this restriction to either
-     be lifted or removed.  */
-#ifndef MAX_REMOTE_PACKET_SIZE
-#define MAX_REMOTE_PACKET_SIZE 16384
-#endif
-  /* NOTE: 20 ensures we can write at least one byte.  */
-#ifndef MIN_REMOTE_PACKET_SIZE
-#define MIN_REMOTE_PACKET_SIZE 20
-#endif
   long what_they_get;
   if (config->fixed_p)
     {
       if (config->size <= 0)
-	what_they_get = MAX_REMOTE_PACKET_SIZE;
+	what_they_get = DEFAULT_MAX_MEMORY_PACKET_SIZE;
       else
 	what_they_get = config->size;
     }
@@ -964,10 +962,8 @@  get_memory_packet_size (struct memory_packet_config *config)
 	  && what_they_get > rsa->actual_register_packet_size)
 	what_they_get = rsa->actual_register_packet_size;
     }
-  if (what_they_get > MAX_REMOTE_PACKET_SIZE)
-    what_they_get = MAX_REMOTE_PACKET_SIZE;
-  if (what_they_get < MIN_REMOTE_PACKET_SIZE)
-    what_they_get = MIN_REMOTE_PACKET_SIZE;
+  if (what_they_get < MIN_MEMORY_PACKET_SIZE)
+    what_they_get = MIN_MEMORY_PACKET_SIZE;
 
   /* Make sure there is room in the global buffer for this packet
      (including its trailing NUL byte).  */
@@ -1004,15 +1000,16 @@  set_memory_packet_size (char *args, struct memory_packet_config *config)
       size = strtoul (args, &end, 0);
       if (args == end)
 	error (_("Invalid %s (bad syntax)."), config->name);
-#if 0
       /* Instead of explicitly capping the size of a packet to
-         MAX_REMOTE_PACKET_SIZE or dissallowing it, the user is
-         instead allowed to set the size to something arbitrarily
-         large.  */
-      if (size > MAX_REMOTE_PACKET_SIZE)
-	error (_("Invalid %s (too large)."), config->name);
-#endif
+	 DEFAULT_MAX_MEMORY_PACKET_SIZE or dissallowing it, the user
+	 is allowed to set the size to something arbitrarily
+	 large.  */
     }
+
+  /* So that the query shows the correct value.  */
+  if (size <= 0)
+    size = DEFAULT_MAX_MEMORY_PACKET_SIZE;
+
   /* Extra checks?  */
   if (fixed_p && !config->fixed_p)
     {
@@ -4007,6 +4004,7 @@  remote_check_symbols (void)
   char *msg, *reply, *tmp;
   struct bound_minimal_symbol sym;
   int end;
+  struct cleanup *old_chain;
 
   /* The remote side has no concept of inferiors that aren't running
      yet, it only knows about running processes.  If we're connected
@@ -4025,7 +4023,8 @@  remote_check_symbols (void)
 
   /* Allocate a message buffer.  We can't reuse the input buffer in RS,
      because we need both at the same time.  */
-  msg = alloca (get_remote_packet_size ());
+  msg = xmalloc (get_remote_packet_size ());
+  old_chain = make_cleanup (xfree, msg);
 
   /* Invite target to request symbol lookups.  */
 
@@ -4063,6 +4062,8 @@  remote_check_symbols (void)
       getpkt (&rs->buf, &rs->buf_size, 0);
       reply = rs->buf;
     }
+
+  do_cleanups (old_chain);
 }
 
 static struct serial *
@@ -4185,13 +4186,6 @@  remote_packet_size (const struct protocol_feature *feature,
       return;
     }
 
-  if (packet_size > MAX_REMOTE_PACKET_SIZE)
-    {
-      warning (_("limiting remote suggested packet size (%d bytes) to %d"),
-	       packet_size, MAX_REMOTE_PACKET_SIZE);
-      packet_size = MAX_REMOTE_PACKET_SIZE;
-    }
-
   /* Record the new maximum packet size.  */
   rs->explicit_packet_size = packet_size;
 }
@@ -7783,7 +7777,8 @@  putpkt_binary (const char *buf, int cnt)
   struct remote_state *rs = get_remote_state ();
   int i;
   unsigned char csum = 0;
-  char *buf2 = alloca (cnt + 6);
+  char *buf2 = xmalloc (cnt + 6);
+  struct cleanup *old_chain = make_cleanup (xfree, buf2);
 
   int ch;
   int tcount = 0;
@@ -7876,6 +7871,7 @@  putpkt_binary (const char *buf, int cnt)
 	    case '+':
 	      if (remote_debug)
 		fprintf_unfiltered (gdb_stdlog, "Ack\n");
+	      do_cleanups (old_chain);
 	      return 1;
 	    case '-':
 	      if (remote_debug)
@@ -7884,7 +7880,10 @@  putpkt_binary (const char *buf, int cnt)
 	    case SERIAL_TIMEOUT:
 	      tcount++;
 	      if (tcount > 3)
-		return 0;
+		{
+		  do_cleanups (old_chain);
+		  return 0;
+		}
 	      break;		/* Retransmit buffer.  */
 	    case '$':
 	      {
@@ -7971,6 +7970,8 @@  putpkt_binary (const char *buf, int cnt)
 	}
 #endif
     }
+
+  do_cleanups (old_chain);
   return 0;
 }