RFA: patch to remote.c for larger download packet support (part 1)

["Frank Ch. Eigler" <fche at cygnus dot com>]

Hi -

The following patches are one step in loosening the remote protocols'
packet size limiting code, so that downloads to versatile targets across
reliable channels may be quicker.  This is done by making remote_write_bytes()
and putpkt() be able to accept packets up to "remotewritesize" bytes in length.

The next step would be generalizing generic_load() to supply larger packets
when desired.

The first patch is against gdb proper.  The second is a new test case family
that checks the effect of changing the parameter over several repeated
downloads.


[gdb/ChangeLog]
1999-10-04  Frank Ch. Eigler  <fche@cygnus.com>

	* remote.c (hexnumnstr): New function.  Allow setting of width.
	(hexnumstr): Call the above.
	(MAXIMUM_REMOTE_WRITE_SIZE): New macro.
	(remote_write_bytes): Allocate packet buffer with configurable size.
	Don't limit it to PBUFSIZ.  Fill in X-protocol address field more
	reliably.
	(putpkt_binary): Allocate outgoing packet buffer based on incoming
	buffer length.  Remove unnecessary sanity check.

Index: remote.c
===================================================================
RCS file: /cvs/cvsfiles/devo/gdb/remote.c,v
retrieving revision 1.247
diff -p -p -u -r1.247 remote.c
--- remote.c	1999/10/04 18:47:15	1.247
+++ remote.c	1999/10/05 19:36:59
@@ -166,6 +166,8 @@ static int remote_query PARAMS ((int /*c
 
 static int hexnumstr PARAMS ((char *, ULONGEST));
 
+static int hexnumnstr PARAMS ((char *, ULONGEST, int));
+
 static CORE_ADDR remote_address_masked PARAMS ((CORE_ADDR));
 
 static void print_packet PARAMS ((char *));
@@ -284,10 +286,11 @@ static int cisco_kernel_mode = 0;
 #define MAXBUFBYTES(N) (((N)-32)/2)
 
 /* Having this larger than 400 causes us to be incompatible with m68k-stub.c
-   and i386-stub.c.  Normally, no one would notice because it only matters
-   for writing large chunks of memory (e.g. in downloads).  Also, this needs
-   to be more than 400 if required to hold the registers (see below, where
-   we round it up based on REGISTER_BYTES).  */
+   and i386-stub.c.  Normally, no one would notice because it only
+   matters for writing large chunks of memory (e.g. in downloads), and
+   that doesn't use this limit.  Also, this needs to be more than 400
+   if required to hold the registers (see below, where we round it up
+   based on REGISTER_BYTES).  */
 /* Round up PBUFSIZ to hold all the registers, at least.  */
 #define	PBUFSIZ ((REGISTER_BYTES > MAXBUFBYTES (400)) \
 		 ? (REGISTER_BYTES * 2 + 32) \
@@ -301,6 +304,14 @@ static int cisco_kernel_mode = 0;
 
 static int remote_write_size;
 
+/* Define a maximum safe value for this, considering that buffers
+   of that size are allocated with alloca().  This limit might as
+   well equal the maximum load-chunk from symfile.c, since that number
+   is an upper limit on individual calls to remote_write_bytes(). */
+
+#define MAXIMUM_REMOTE_WRITE_SIZE (256*1024)
+
+
 /* This variable sets the number of bits in an address that are to be
    sent in a memory ("M" or "m") packet.  Normally, after stripping
    leading zeros, the entire address would be sent. This variable
@@ -2962,25 +2973,36 @@ hexnumlen (num)
   return max (i, 1);
 }
 
-/* Set BUF to the hex digits representing NUM.  */
+/* Set BUF to the minimum number of hex digits representing NUM.  */
 
 static int
 hexnumstr (buf, num)
      char *buf;
      ULONGEST num;
 {
-  int i;
   int len = hexnumlen (num);
+  return hexnumnstr (buf, num, len);
+}
 
-  buf[len] = '\0';
 
-  for (i = len - 1; i >= 0; i--)
+/* Set BUF to the hex digits representing NUM, padded to WIDTH characters.  */
+
+static int
+hexnumnstr (buf, num, width)
+     char *buf;
+     ULONGEST num;
+     int width;
+{
+  int i;
+  buf[width] = '\0';
+
+  for (i = width - 1; i >= 0; i--)
     {
       buf[i] = "0123456789abcdef"[(num & 0xf)];
       num >>= 4;
     }
 
-  return len;
+  return width;
 }
 
 /* Mask all but the least significant REMOTE_ADDRESS_SIZE bits. */
@@ -3055,9 +3077,11 @@ check_binary_download (addr)
       }
     }
 }
+
+/* Write memory data directly to the remote machine.  Use packets as
+   large as remote_write_size.  This does not inform the data cache; 
+   the data cache may use this.
 
-/* Write memory data directly to the remote machine.
-   This does not inform the data cache; the data cache uses this.
    MEMADDR is the address in the remote memory space.
    MYADDR is the address of the buffer in our space.
    LEN is the number of bytes.
@@ -3070,17 +3094,17 @@ remote_write_bytes (memaddr, myaddr, len
      char *myaddr;
      int len;
 {
-  unsigned char *buf = alloca (PBUFSIZ);
+  unsigned char *buf = alloca (min (remote_write_size, MAXIMUM_REMOTE_WRITE_SIZE));
   int max_buf_size;		/* Max size of packet output buffer */
   int origlen;
 
   /* Verify that the target can support a binary download */
   check_binary_download (memaddr);
 
-  /* Chop the transfer down if necessary */
-
-  max_buf_size = min (remote_write_size, PBUFSIZ);
-  if (remote_register_buf_size != 0)
+  /* Chop the transfer down if necessary, but not if the user has
+     overridden remote_write_size to be something large. */
+  max_buf_size = remote_write_size;
+  if (remote_register_buf_size != 0 && remote_write_size <= PBUFSIZ)
     max_buf_size = min (max_buf_size, remote_register_buf_size);
 
   /* Subtract header overhead from max payload size -  $M<memaddr>,<len>:#nn */
@@ -3090,7 +3114,7 @@ remote_write_bytes (memaddr, myaddr, len
   while (len > 0)
     {
       unsigned char *p, *plen;
-      int todo;
+      int todo, plenlen;
       int i;
 
       /* construct "M"<memaddr>","<len>":" */
@@ -3115,7 +3139,8 @@ remote_write_bytes (memaddr, myaddr, len
       *p++ = ',';
 
       plen = p;			/* remember where len field goes */
-      p += hexnumstr (p, (ULONGEST) todo);
+      plenlen = hexnumstr (p, (ULONGEST) todo);
+      p += plenlen;
       *p++ = ':';
       *p = '\0';
 
@@ -3151,13 +3176,11 @@ remote_write_bytes (memaddr, myaddr, len
 	      {
 		/* Escape chars have filled up the buffer prematurely, 
 		   and we have actually sent fewer bytes than planned.
-		   Fix-up the length field of the packet.  */
+		   Fix-up the length field of the packet.  Use the same
+                   number of characters as before. */
 		
-		/* FIXME: will fail if new len is a shorter string than 
-		   old len.  */
-		
-		plen += hexnumstr (plen, (ULONGEST) i);
-		*plen++ = ':';
+		plen += hexnumnstr (plen, (ULONGEST) i, plenlen);
+		*plen = ':'; /* overwrite \0 from hexnumnstr() */
 	      }
 	    break;
 	  }
@@ -3460,8 +3483,8 @@ putpkt_binary (buf, cnt)
 {
   int i;
   unsigned char csum = 0;
-  char *buf2 = alloca (PBUFSIZ);
-  char *junkbuf = alloca (PBUFSIZ);
+  char *buf2 = alloca (cnt + 5);
+  char *junkbuf = alloca (PBUFSIZ);  /* used to swallow unexpected replies */
 
   int ch;
   int tcount = 0;
@@ -3469,9 +3492,6 @@ putpkt_binary (buf, cnt)
 
   /* Copy the packet into buffer BUF2, encapsulating it
      and giving it a checksum.  */
-
-  if (cnt > BUFSIZ - 5)		/* Prosanity check */
-    abort ();
 
   p = buf2;
   *p++ = '$';




[gdb/testsuite/ChangeLog]
1999-10-05  Frank Ch. Eigler  <fche@cygnus.com>

	* gdb.base/remote.exp: New test for remote downloading settings.
	* gdb.base/remote.c: New file with large .data.

Index: testsuite/gdb.base/remote.exp
===================================================================
RCS file: testsuite/gdb.base/remote.exp
diff -N remote.exp
--- /dev/null	Mon Dec 31 20:00:00 1979
+++ remote.exp	Tue Oct  5 12:35:03 1999
@@ -0,0 +1,83 @@
+#   Copyright (C) 1999 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+# 
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+# 
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.  
+
+# Please email any bugs, comments, and/or additions to this file to:
+# bug-gdb@prep.ai.mit.edu
+
+if $tracelevel then {
+    strace $tracelevel
+}
+
+set prms_id 0
+set bug_id 0
+
+
+# test only on a remote target board
+if {! [is_remote target]} {
+    return
+}
+
+
+set testfile "remote"
+set srcfile ${testfile}.c
+set binfile ${objdir}/${subdir}/${testfile}
+
+
+proc gdb_load_timed {executable writesize} {
+    global test gdb_prompt
+    set test "timed download `[file tail $executable]' ($writesize)"
+
+    if {$writesize != ""} then {
+	send_gdb "set remotewritesize $writesize\n"
+	gdb_expect 5 {
+	    -re ".*$gdb_prompt $" { }
+	    timeout { fail "$test - setting remotewritesize" ; return }
+	}
+    }
+
+    set load_begin_time [clock clicks]
+    set result [gdb_load $executable]
+    set load_end_time [clock clicks]
+    if {$result < 0} then { fail "$test - loading executable"; return }
+    verbose "$test - time [expr ($load_end_time - $load_begin_time) / 1000] ms"
+    pass $test
+}
+
+
+
+# tests
+
+gdb_start
+
+set result [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}]
+if {$result != "" } then {
+    gdb_suppress_entire_file "Testcase compile failed, so all tests in this file will automatically fail."
+}
+
+gdb_load_timed $binfile {}
+gdb_load_timed $binfile 50
+gdb_load_timed $binfile 100
+gdb_load_timed $binfile 200
+gdb_load_timed $binfile 400
+
+# extra tests for capable targets
+if {[target_info gdb,big_rx_buffers] != ""} then {
+    gdb_load_timed $binfile 800
+    gdb_load_timed $binfile 8000
+    gdb_load_timed $binfile 80000
+}
+
+gdb_exit
Index: testsuite/gdb.base/remote.c
===================================================================
RCS file: testsuite/remote.c
diff -N remote.c
--- /dev/null	Mon Dec 31 20:00:00 1979
+++ remote.c	Tue Oct  5 12:35:03 1999
@@ -0,0 +1,26 @@
+#include <stdio.h>
+#include <stdlib.h>
+
+/**************************************************************************
+ * TESTS :
+ *   -- downloading of a rather large executable
+ ***************************************************************************/
+
+
+/* A large array in .data.  If RLE compression becomes available during
+   downloads, this would have to become a bunch of real random data.  
+   Here's a quick way of generating such a bunch:
+
+yes | awk '{printf ("%4d,", rand()*1000);}' | fold -w80 -s | head -4096
+
+*/
+
+unsigned long random_data[65536] = { 1 };
+
+int
+main() 
+{
+  printf ("%lu\n", random_data [rand() % 
+			       (sizeof (random_data) /
+			        sizeof (random_data [0]))]);
+}