Re: [PATCH] gdb: remove packet size limit

[Pedro Alves <palves at redhat dot com>]

On 08/26/2015 09:43 AM, Gary Benson wrote:
> 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, pushed with that change.

>From a5c0808e221c0e8600561c2c9664c920d808a887 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Thu, 27 Aug 2015 13:03:15 +0100
Subject: [PATCH] gdb: remove packet size limit

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-27  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/ChangeLog | 13 +++++++++++
 gdb/remote.c  | 71 ++++++++++++++++++++++++++++++-----------------------------
 2 files changed, 49 insertions(+), 35 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 0323b06..22005ac 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,16 @@
+2015-08-27  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.
+
 2015-08-26  Luis Machado  <lgustavo@codesourcery.com>
 
 	* compile/compile.c (compile_to_object): Mention language in
diff --git a/gdb/remote.c b/gdb/remote.c
index e11cb5f..3afcaf4 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -921,6 +921,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 (folklore?) 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.  */
 
@@ -930,23 +940,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;
     }
@@ -965,10 +963,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).  */
@@ -1005,15 +1001,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
+
+      /* Instead of explicitly capping the size of a packet to or
+	 disallowing 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)
     {
@@ -4008,6 +4005,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
@@ -4026,7 +4024,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.  */
 
@@ -4064,6 +4063,8 @@ remote_check_symbols (void)
       getpkt (&rs->buf, &rs->buf_size, 0);
       reply = rs->buf;
     }
+
+  do_cleanups (old_chain);
 }
 
 static struct serial *
@@ -4186,13 +4187,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;
 }
@@ -7784,7 +7778,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;
@@ -7877,6 +7872,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)
@@ -7885,7 +7881,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 '$':
 	      {
@@ -7972,6 +7971,8 @@ putpkt_binary (const char *buf, int cnt)
 	}
 #endif
     }
+
+  do_cleanups (old_chain);
   return 0;
 }
 
-- 
1.9.3