[remote protocol] support for disabling packet acknowledgement

[Pedro Alves <pedro at codesourcery dot com>]

Hi,

This patch adds support to the remote protocol to disable packet
acknowlegment.  This is useful when the transport being used is itself
reliable, e.g., TCP/IP.  In these cases, by removing the acking
we reduce the amount of roundtrips, and decrease the communication
latency.

While we were discussing implementing this, I noticed that Apple
already had something of the sort, so we ended up reusing Apple's
QStartNoAckMode packet name, and basing our implementation on
their version, modernizing it, adding qSupport support
to it, tweaking the user interface so it is integrated with
the "set remote xxx-packet" set of commands, and documenting it.

The docs changes below explain more in detail the approach taken.

I'm including in the patch GDB changes to support the feature,
and adding the support to gdbserver.  Regtesting against a native
gdbserver on x86_64-unknown-linux-gnu doesn't show any regression.

What do you think?

-- 
Pedro Alves

gdb/doc/
2008-07-10  Sandra Loosemore  <sandra@codesourcery.com>

	* gdb.texinfo (Remote Configuration): Document set remote noack-packet.
	(Remote Protocol): Add Packet Acknowledgement to menu.
	(Overview): Mention +/- can be disabled, and point to new section
	where this is discussed in detail.
	(General Query Packets): Document QStartNoAckMode packet, and
	corresponding qSupported reply.
	(Packet Acknowledgement): New section.

gdb/
2008-07-10  Pedro Alves  <pedro@codesourcery.com>

	Add no-ack mode to the remote protocol --- optionally stop ACKing
	packets and responses when we have a reliable communication
	medium.

	Based on Apple's GDB, by Jason Molenda <jmolenda@apple.com>

	* remote.c (struct remote_state): Add noack_mode field.
	(PACKET_QStartNoAckMode): New.
	(remote_start_remote): Don't any outstanding packet here.
	(remote_open_1): Clear noack_mode.  Ack any outstanding packet
	here.  Activate noack mode if requested.
	(remote_protocol_features): Add QStartNoAckMode.
	(remote_open_1):
	(putpkt_binary): Don't send ack in noack mode.
	(read_frame): Don't recompute the checksum in noack mode.
	(getpkt_sane): Skip sending ack if in noack mode.
	(_initialize_remote): Add set/show remote noack mode.

gdb/gdbserver/
2008-07-10  Pedro Alves  <pedro@codesourcery.com>

	* remote-utils.c (noack_mode, transport_is_reliable): New globals.
	(remote_open): Set or clear transport_is_reliable.
	(putpkt_binary): Don't expect acks in noack mode.
	(getpkt): Don't send ack/nac in noack mode.
	* server.c (handle_general_set): Handle QStartNoAckMode.
	(handle_query): If connected by tcp pass QStartNoAckMode+ in
	qSupported.
	(main): Reset noack_mode on every connection.
	* server.h (noack_mode): Declare.

---
 gdb/doc/gdb.texinfo          |   80 ++++++++++++++++++++++++++++++++++++++++++-
 gdb/gdbserver/remote-utils.c |   51 ++++++++++++++++++++++-----
 gdb/gdbserver/server.c       |   16 ++++++++
 gdb/gdbserver/server.h       |    2 +
 gdb/remote.c                 |   72 +++++++++++++++++++++++++++++++++++---
 5 files changed, 204 insertions(+), 17 deletions(-)

Index: src/gdb/doc/gdb.texinfo
===================================================================
--- src.orig/gdb/doc/gdb.texinfo	2008-07-07 19:02:34.000000000 +0100
+++ src/gdb/doc/gdb.texinfo	2008-07-10 17:38:44.000000000 +0100
@@ -13676,6 +13676,10 @@ are:
 @item @code{hostio-unlink-packet}
 @tab @code{vFile:unlink}
 @tab @code{remote delete}
+
+@item @code{noack-packet}
+@tab @code{QStartNoAckMode}
+@tab Packet acknowledgement
 @end multitable
 
 @node Remote Stub
@@ -23703,6 +23707,7 @@ Show the current setting of the target w
 * Tracepoint Packets::
 * Host I/O Packets::
 * Interrupts::
+* Packet Acknowledgement::
 * Examples::
 * File-I/O Remote Protocol Extension::
 * Library List Format::
@@ -23752,7 +23757,6 @@ That @var{sequence-id} was appended to t
 has never output @var{sequence-id}s.  Stubs that handle packets added
 since @value{GDBN} 5.0 must not accept @var{sequence-id}.
 
-@cindex acknowledgment, for @value{GDBN} remote
 When either the host or the target machine receives a packet, the first
 response expected is an acknowledgment: either @samp{+} (to indicate
 the package was received correctly) or @samp{-} (to request
@@ -23764,6 +23768,10 @@ retransmission):
 @end smallexample
 @noindent
 
+The @samp{+}/@samp{-} acknowledgements can be disabled
+once a connection is established.
+@xref{Packet Acknowledgement}, for details.
+
 The host (@value{GDBN}) sends @var{command}s, and the target (the
 debugging stub incorporated in your program) sends a @var{response}.  In
 the case of step and continue @var{command}s, the response is only sent
@@ -24818,6 +24826,23 @@ A badly formed request or an error was e
 An empty reply indicates that @samp{qSearch:memory} is not recognized.
 @end table
 
+@item QStartNoAckMode
+@cindex @samp{QStartNoAckMode} packet
+@anchor{QStartNoAckMode}
+Request that the remote stub disable the normal @samp{+}/@samp{-}
+protocol acknowledgments (@pxref{Packet Acknowledgement}).
+
+Reply:
+@table @samp
+@item OK
+The stub has switched to no-acknowledgement mode.
+@value{GDBN} acknowledges this reponse,
+but neither the stub nor @value{GDBN} shall send or expect further
+@samp{+}/@samp{-} acknowledgements in the current connection.
+@item
+An empty reply indicates that the stub does not support no-acknowledgment mode.
+@end table
+
 @item qSupported @r{[}:@var{gdbfeature} @r{[};@var{gdbfeature}@r{]}@dots{} @r{]}
 @cindex supported packets, remote query
 @cindex features of the remote protocol
@@ -24959,6 +24984,11 @@ These are the currently defined stub fea
 @tab @samp{-}
 @tab Yes
 
+@item @samp{QStartNoAckMode}
+@tab No
+@tab @samp{i}
+@tab Yes
+
 @end multitable
 
 These are the currently defined stub features, in more detail:
@@ -25003,6 +25033,10 @@ The remote stub understands the @samp{qX
 The remote stub understands the @samp{QPassSignals} packet
 (@pxref{QPassSignals}).
 
+@item QStartNoAckMode
+The remote stub understands the @samp{QStartNoAckMode} packet and
+prefers to operate in no-acknowledgement mode.  @xref{Packet Acknowledgement}.
+
 @end table
 
 @item qSymbol::
@@ -25547,6 +25581,50 @@ Reply Packets (@pxref{Stop Reply Packets
 of successfully stopping the program.  Interrupts received while the
 program is stopped will be discarded.
 
+@node Packet Acknowledgement
+@section Packet Acknowledgement
+
+@cindex acknowledgment, for @value{GDBN} remote
+@cindex packet acknowledgment, for @value{GDBN} remote
+By default, when either the host or the target machine receives a packet,
+the first response expected is an acknowledgment: either @samp{+} (to indicate
+the package was received correctly) or @samp{-} (to request retransmission).
+This mechanism allows the @value{GDBN} remote protocol to operate over
+unreliable transport mechanisms, such as a serial line.
+
+In cases where the transport mechanism is itself reliable (such as a pipe or
+TCP connection), the @samp{+}/@samp{-} acknowledgements are redundant.
+It may be desirable to disable them in that case to reduce communication
+overhead, or for other reasons.  This can be accomplished by means of the
+@samp{QStartNoAckMode} packet; @pxref{QStartNoAckMode}.
+
+When in no-acknowledgment mode, neither the stub nor GDB shall send or
+expect @samp{+}/@samp{-} protocol acknowledgements.  The packet
+and response format still includes the normal checksum, as described in
+@ref{Overview}, but the checksum may be ignored by the receiver.
+
+If the stub supports @samp{QStartNoAckMode} and prefers to operate in
+no-acknowledgement mode, it should report that to @value{GDBN}
+by including @samp{QStartNoAckMode+} in its response to @samp{qSupported};
+@pxref{qSupported}.
+If @value{GDBN} also supports @samp{QStartNoAckMode} and it has not been
+disabled via the @code{set remote noack-packet off} command
+(@pxref{Remote Configuration}),
+@value{GDBN} may then send a @samp{QStartNoAckMode} packet to the stub.
+Only then may the stub actually turn off packet acknowledgements.
+@value{GDBN} sends a final @samp{+} acknowledgement of the stub's @samp{OK}
+response, which can be safely ignored by the stub.
+
+Note that @code{set remote noack-packet} command only affects negotiation
+between @value{GDBN} and the stub when subsequent connections are made;
+it does not affect the protocol acknowledgement state for any current
+connection.
+Since @samp{+}/@samp{-} acknowledgements are enabled by default when a
+new connection is established,
+there is also no protocol request to re-enable the acknowledgements
+for the current connection, once disabled.
+
+
 @node Examples
 @section Examples
 
Index: src/gdb/remote.c
===================================================================
--- src.orig/gdb/remote.c	2008-07-10 17:37:11.000000000 +0100
+++ src/gdb/remote.c	2008-07-10 17:39:29.000000000 +0100
@@ -274,6 +274,11 @@ struct remote_state
      skip calling getpkt.  This flag is set when BUF contains a
      stop reply packet and the target is not waiting.  */
   int cached_wait_status;
+
+  /* True, if in no ack mode.  That is, neither GDB nor the stub will
+     expect acks from each other.  The connection is assumed to be
+     reliable.  */
+  int noack_mode;
 };
 
 /* This data could be associated with a target, but we do not always
@@ -960,6 +965,7 @@ enum {
   PACKET_qSearch_memory,
   PACKET_vAttach,
   PACKET_vRun,
+  PACKET_QStartNoAckMode,
   PACKET_MAX
 };
 
@@ -2291,9 +2297,6 @@ remote_start_remote (struct ui_out *uiou
 
   immediate_quit++;		/* Allow user to interrupt it.  */
 
-  /* Ack any packet which the remote side has already sent.  */
-  serial_write (remote_desc, "+", 1);
-
   /* Check whether the target is running now.  */
   putpkt ("?");
   getpkt (&rs->buf, &rs->buf_size, 0);
@@ -2551,6 +2554,8 @@ static struct protocol_feature remote_pr
     PACKET_qXfer_spu_write },
   { "QPassSignals", PACKET_DISABLE, remote_supported_packet,
     PACKET_QPassSignals },
+  { "QStartNoAckMode", PACKET_DISABLE, remote_supported_packet,
+    PACKET_QStartNoAckMode },
 };
 
 static void
@@ -2684,6 +2689,8 @@ static void
 remote_open_1 (char *name, int from_tty, struct target_ops *target, int extended_p)
 {
   struct remote_state *rs = get_remote_state ();
+  struct packet_config *noack_config;
+
   if (name == 0)
     error (_("To open a remote debug connection, you need to specify what\n"
 	   "serial device is attached to the remote system\n"
@@ -2765,6 +2772,7 @@ remote_open_1 (char *name, int from_tty,
      remote_query_supported or as they are needed.  */
   init_all_packet_configs ();
   rs->explicit_packet_size = 0;
+  rs->noack_mode = 0;
 
   general_thread = not_sent_ptid;
   continue_thread = not_sent_ptid;
@@ -2773,11 +2781,39 @@ remote_open_1 (char *name, int from_tty,
   use_threadinfo_query = 1;
   use_threadextra_query = 1;
 
+  /* Ack any packet which the remote side has already sent.  */
+  serial_write (remote_desc, "+", 1);
+
   /* The first packet we send to the target is the optional "supported
      packets" request.  If the target can answer this, it will tell us
      which later probes to skip.  */
   remote_query_supported ();
 
+  /* Next, we possibly activate noack mode.
+
+     If the QStartNoAckMode packet configuration is set to AUTO,
+     enable noack mode if the stub reported a wish for it with
+     qSupported.
+
+     If set to TRUE, then enable noack mode even if the stub didn't
+     report it in qSupported.  If the stub doesn't reply OK, the
+     session ends with an error.
+
+     If FALSE, then don't activate noack mode, regardless of what the
+     stub claimed should be the default with qSupported.  */
+
+  noack_config = &remote_protocol_packets[PACKET_QStartNoAckMode];
+
+  if (noack_config->detect == AUTO_BOOLEAN_TRUE
+      || (noack_config->detect == AUTO_BOOLEAN_AUTO
+	  && noack_config->support == PACKET_ENABLE))
+    {
+      putpkt ("QStartNoAckMode");
+      getpkt (&rs->buf, &rs->buf_size, 0);
+      if (packet_ok (rs->buf, noack_config) == PACKET_OK)
+	rs->noack_mode = 1;
+    }
+
   /* Next, if the target can specify a description, read it.  We do
      this before anything involving memory or registers.  */
   target_find_description ();
@@ -4790,6 +4826,11 @@ putpkt_binary (char *buf, int cnt)
       if (serial_write (remote_desc, buf2, p - buf2))
 	perror_with_name (_("putpkt: write failed"));
 
+      /* If this is a no acks version of the remote protocol, send the
+	 packet and move on.  */
+      if (rs->noack_mode)
+        break;
+
       /* Read until either a timeout occurs (-2) or '+' is read.  */
       while (1)
 	{
@@ -4866,6 +4907,7 @@ putpkt_binary (char *buf, int cnt)
 	}
 #endif
     }
+  return 0;
 }
 
 /* Come here after finding the start of a frame when we expected an
@@ -4921,6 +4963,7 @@ read_frame (char **buf_p,
   long bc;
   int c;
   char *buf = *buf_p;
+  struct remote_state *rs = get_remote_state ();
 
   csum = 0;
   bc = 0;
@@ -4966,6 +5009,12 @@ read_frame (char **buf_p,
 		return -1;
 	      }
 
+	    /* Don't recompute the checksum; with no ack packets we
+	       don't have any way to indicate a packet retransmission
+	       is necessary.  */
+	    if (rs->noack_mode)
+	      return bc;
+
 	    pktcsum = (fromhex (check_0) << 4) | fromhex (check_1);
 	    if (csum == pktcsum)
               return bc;
@@ -5124,20 +5173,28 @@ getpkt_sane (char **buf, long *sizeof_bu
 	      fputstrn_unfiltered (*buf, val, 0, gdb_stdlog);
 	      fprintf_unfiltered (gdb_stdlog, "\n");
 	    }
-	  serial_write (remote_desc, "+", 1);
+
+	  /* Skip the ack char if we're in no-ack mode.  */
+	  if (!rs->noack_mode)
+	    serial_write (remote_desc, "+", 1);
 	  return val;
 	}
 
       /* Try the whole thing again.  */
     retry:
-      serial_write (remote_desc, "-", 1);
+      /* Skip the nack char if we're in no-ack mode.  */
+      if (!rs->noack_mode)
+	serial_write (remote_desc, "-", 1);
     }
 
   /* We have tried hard enough, and just can't receive the packet.
      Give up.  */
 
   printf_unfiltered (_("Ignoring packet error, continuing...\n"));
-  serial_write (remote_desc, "+", 1);
+
+  /* Skip the ack char if we're in no-ack mode.  */
+  if (!rs->noack_mode)
+    serial_write (remote_desc, "+", 1);
   return -1;
 }
 
@@ -7532,6 +7589,9 @@ Show the maximum size of the address (in
   add_packet_config_cmd (&remote_protocol_packets[PACKET_vRun],
 			 "vRun", "run", 0);
 
+  add_packet_config_cmd (&remote_protocol_packets[PACKET_QStartNoAckMode],
+			 "QStartNoAckMode", "noack", 0);
+
   /* Keep the old ``set remote Z-packet ...'' working.  Each individual
      Z sub-packet has its own set and show commands, but users may
      have sets to this variable in their .gdbinit files (or in their
Index: src/gdb/gdbserver/remote-utils.c
===================================================================
--- src.orig/gdb/gdbserver/remote-utils.c	2008-07-10 17:37:11.000000000 +0100
+++ src/gdb/gdbserver/remote-utils.c	2008-07-10 17:38:08.000000000 +0100
@@ -90,7 +90,7 @@ static struct sym_cache *symbol_cache;
    failures.  */
 int all_symbols_looked_up;
 
-int remote_debug = 0;
+int remote_debug = 1;
 struct ui_file *gdb_stdlog;
 
 static int remote_desc = INVALID_DESCRIPTOR;
@@ -99,6 +99,11 @@ static int remote_desc = INVALID_DESCRIP
 extern int using_threads;
 extern int debug_threads;
 
+/* If true, then GDB has requested noack mode.  */
+int noack_mode = 0;
+/* If true, then we tell GDB to use noack mode by default.  */
+int transport_is_reliable = 0;
+
 #ifdef USE_WIN32API
 # define read(fd, buf, len) recv (fd, (char *) buf, len, 0)
 # define write(fd, buf, len) send (fd, (char *) buf, len, 0)
@@ -181,6 +186,8 @@ remote_open (char *name)
 
       fprintf (stderr, "Remote debugging using %s\n", name);
 #endif /* USE_WIN32API */
+
+      transport_is_reliable = 0;
     }
   else
     {
@@ -267,6 +274,8 @@ remote_open (char *name)
       /* Convert IP address to string.  */
       fprintf (stderr, "Remote debugging from host %s\n", 
          inet_ntoa (sockaddr.sin_addr));
+
+      transport_is_reliable = 1;
     }
 
 #if defined(F_SETFL) && defined (FASYNC)
@@ -551,6 +560,17 @@ putpkt_binary (char *buf, int cnt)
 	  return -1;
 	}
 
+      if (noack_mode)
+	{
+	  /* Don't expect an ack then.  */
+	  if (remote_debug)
+	    {
+	      fprintf (stderr, "putpkt (\"%s\"); [noack mode]\n", buf2);
+	      fflush (stderr);
+	    }
+	  break;
+	}
+
       if (remote_debug)
 	{
 	  fprintf (stderr, "putpkt (\"%s\"); [looking for ack]\n", buf2);
@@ -774,23 +794,34 @@ getpkt (char *buf)
       if (csum == (c1 << 4) + c2)
 	break;
 
+      if (noack_mode)
+	{
+	  fprintf (stderr, "Bad checksum, sentsum=0x%x, csum=0x%x, buf=%s [no-ack-mode, Bad medium?]\n",
+		   (c1 << 4) + c2, csum, buf);
+	  /* Not much we can do, GDB wasn't expecting an ack/nac.  */
+	  break;
+	}
+
       fprintf (stderr, "Bad checksum, sentsum=0x%x, csum=0x%x, buf=%s\n",
 	       (c1 << 4) + c2, csum, buf);
       write (remote_desc, "-", 1);
     }
 
-  if (remote_debug)
+  if (!noack_mode)
     {
-      fprintf (stderr, "getpkt (\"%s\");  [sending ack] \n", buf);
-      fflush (stderr);
-    }
+      if (remote_debug)
+	{
+	  fprintf (stderr, "getpkt (\"%s\");  [sending ack] \n", buf);
+	  fflush (stderr);
+	}
 
-  write (remote_desc, "+", 1);
+      write (remote_desc, "+", 1);
 
-  if (remote_debug)
-    {
-      fprintf (stderr, "[sent ack]\n");
-      fflush (stderr);
+      if (remote_debug)
+	{
+	  fprintf (stderr, "[sent ack]\n");
+	  fflush (stderr);
+	}
     }
 
   return bp - buf;
Index: src/gdb/gdbserver/server.c
===================================================================
--- src.orig/gdb/gdbserver/server.c	2008-07-10 17:37:11.000000000 +0100
+++ src/gdb/gdbserver/server.c	2008-07-10 17:38:08.000000000 +0100
@@ -267,6 +267,19 @@ handle_general_set (char *own_buf)
       return;
     }
 
+  if (strcmp (own_buf, "QStartNoAckMode") == 0)
+    {
+      if (remote_debug)
+	{
+	  fprintf (stderr, "[noack mode enabled]\n");
+	  fflush (stderr);
+	}
+
+      noack_mode = 1;
+      write_ok (own_buf);
+      return;
+    }
+
   /* Otherwise we didn't know what packet it was.  Say we didn't
      understand it.  */
   own_buf[0] = 0;
@@ -774,6 +787,8 @@ handle_query (char *own_buf, int packet_
 	 qXfer:feature:read at all, we will never be re-queried.  */
       strcat (own_buf, ";qXfer:features:read+");
 
+      if (transport_is_reliable)
+	strcat (own_buf, ";QStartNoAckMode+");
       return;
     }
 
@@ -1444,6 +1459,7 @@ main (int argc, char *argv[])
 
   while (1)
     {
+      noack_mode = 0;
       remote_open (port);
 
     restart:
Index: src/gdb/gdbserver/server.h
===================================================================
--- src.orig/gdb/gdbserver/server.h	2008-07-10 17:37:11.000000000 +0100
+++ src/gdb/gdbserver/server.h	2008-07-10 17:38:08.000000000 +0100
@@ -171,6 +171,8 @@ extern void hostio_last_error_from_errno
 
 extern int remote_debug;
 extern int all_symbols_looked_up;
+extern int noack_mode;
+extern int transport_is_reliable;
 
 int putpkt (char *buf);
 int putpkt_binary (char *buf, int len);