[Patch] Enable GDB remote protocol to support zlib-compressed xml

[Patch] Enable GDB remote protocol to support zlib-compressed xml

[Terry Guo]

[-- Attachment #1: Type: text/plain, Size: 1530 bytes --]

Hi,

This patch intends to implement a new feature discussed in thread
http://sourceware.org/ml/gdb-patches/2012-06/msg00271.html. With this patch,
stub can return host gdb with zlib-compressed object which consumes less
space and communication overhead compared to its uncompressed format.

A minor change from our discussion is that "compressedXML" is replaced with
"compressedXfer" which I think is more representative because we may
transfer something other than XML file.

Please help to review and comment this patch. Thanks in advance.

BR,
Terry

gdb/doc/ChangeLog:

2012-08-07  Terry Guo  <terry.guo@arm.com>

	* gdb.texinfo (General Query Packets): Document new 
	compressedXfer stub feature.
	(General Query Packets): Document new zread packets.


gdb/ChangeLog:

2012-08-07  Terry Guo  <terry.guo@arm.com>

	* defs.h (gdb_zlib_error): Declare.
	* utils.c (gdb_zlib_error): New function.
	* remote.c (struct remote_state): New field has_cmprs_xfer.
	(enum packet_cmprs_xfer): New enum.
	(struct packet_config): New field xfer_status.
	(add_packet_config_cmd): Initialize the field xfer_status.
	(remote_packet_is_cmprs_xfer): New function.
	(remote_compressed_xfer_feature): New function.
	(remote_protocol_features): New feature compressedXfer.
	(remote_read_qxfer): Try zread and fallback to plain read when
	fails.
	* target.c (target_decompress_buf): New function.
	(target_read_stralloc): Decompress buffer first if any.
	* target.h (remote_packet_is_cmprs_xfer): Declare.

[-- Attachment #2: gdb-zlib-compressed-object-v3.patch --]
[-- Type: application/octet-stream, Size: 16903 bytes --]

diff --git a/gdb/defs.h b/gdb/defs.h
index 1c6fa79..1457624 100644
--- a/gdb/defs.h
+++ b/gdb/defs.h
@@ -291,6 +291,8 @@ extern void set_display_time (int);
 
 extern void set_display_space (int);
 
+extern void gdb_zlib_error (int);
+
 /* Cleanup utilities.  */
 
 #include "cleanups.h"
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index a92df86..c877710 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -36704,6 +36704,11 @@ These are the currently defined stub features and their properties:
 @tab @samp{-}
 @tab No
 
+@item @samp{compressedXfer}
+@tab No
+@tab @samp{-}
+@tab Yes
+
 @end multitable
 
 These are the currently defined stub features, in more detail:
@@ -36858,6 +36863,13 @@ See @ref{Bytecode Descriptions} for details about the bytecode.
 The remote stub supports running a breakpoint's command list itself,
 rather than reporting the hit to @value{GDBN}.
 
+@item compressedXfer
+The remote stub can reply GDB @samp{qXfer:object:zread:annex:offset,length}
+(@pxref{qXfer read}) request with contents compressed in zlib format rather
+than the plain format.  This doesn't mean all the replies will be in compressed
+format.  In the event of this feature, the host GDB will always try @samp{zread}
+first and fall back to plain @samp{read} when fails.
+
 @end table
 
 @item qSymbol::
@@ -36954,6 +36966,7 @@ packets.)
 @xref{Tracepoint Packets}.
 
 @item qXfer:@var{object}:read:@var{annex}:@var{offset},@var{length}
+@item qXfer:@var{object}:zread:@var{annex}:@var{offset},@var{length}
 @cindex read special object, remote request
 @cindex @samp{qXfer} packet
 @anchor{qXfer read}
@@ -36963,6 +36976,12 @@ starting at @var{offset} bytes into the data.  The content and
 encoding of @var{annex} is specific to @var{object}; it can supply
 additional details about what data to access.
 
+The @var{zread} works as the @var{read} except that the transferred objects
+should be zlib-compressed.  This can enable stub to save spaces by
+comprising pre-built contents in zlib format instead of plain format. The
+compressed format can also reduce the communication overhead.  The @var{zread}
+should be used only when the host gdb is built with appropriate zlib library.
+
 Here are the specific requests of this form defined so far.  All
 @samp{qXfer:@var{object}:read:@dots{}} requests use the same reply
 formats, listed below.
diff --git a/gdb/remote.c b/gdb/remote.c
index 1c9367d..53a5ed8 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -353,6 +353,9 @@ struct remote_state
   /* Nonzero if the user has pressed Ctrl-C, but the target hasn't
      responded to that.  */
   int ctrlc_pending_p;
+
+  /* Nonzero if the stub reports support for zlib-compressed transfer.  */
+  int has_cmprs_xfer;
 };
 
 /* Private data that we'll store in (struct thread_info)->private.  */
@@ -1043,12 +1046,20 @@ enum packet_support
     PACKET_DISABLE
   };
 
+enum packet_cmprs_xfer
+  {
+    CMPRS_XFER_DISABLE = 0,
+    CMPRS_XFER_ENABLE,
+    CMPRS_XFER_UNKNOWN
+  };
+
 struct packet_config
   {
     const char *name;
     const char *title;
     enum auto_boolean detect;
     enum packet_support support;
+    enum packet_cmprs_xfer xfer_status;
   };
 
 /* Analyze a packet's return value and update the packet config
@@ -1122,6 +1133,7 @@ add_packet_config_cmd (struct packet_config *config, const char *name,
   config->title = title;
   config->detect = AUTO_BOOLEAN_AUTO;
   config->support = PACKET_SUPPORT_UNKNOWN;
+  config->xfer_status = CMPRS_XFER_UNKNOWN;
   set_doc = xstrprintf ("Set use of remote protocol `%s' (%s) packet",
 			name, title);
   show_doc = xstrprintf ("Show current use of remote "
@@ -1380,6 +1392,74 @@ show_remote_protocol_Z_packet_cmd (struct ui_file *file, int from_tty,
     }
 }
 
+/* Return to target whether the current obj is transferred in zlib-compressed
+   format.  */
+
+int
+remote_packet_is_cmprs_xfer (enum target_object obj, const char *annex,
+			     int type_read)
+{
+  switch (obj)
+    {
+    case TARGET_OBJECT_SPU:
+      if (type_read)
+	return (remote_protocol_packets[PACKET_qXfer_spu_read].xfer_status
+		== CMPRS_XFER_ENABLE);
+      else
+	return (remote_protocol_packets[PACKET_qXfer_spu_write].xfer_status
+		== CMPRS_XFER_ENABLE);
+    case TARGET_OBJECT_SIGNAL_INFO:
+      if (type_read)
+	return (remote_protocol_packets[PACKET_qXfer_siginfo_read].xfer_status
+		== CMPRS_XFER_ENABLE);
+      else
+	return (remote_protocol_packets[PACKET_qXfer_siginfo_write].xfer_status
+		== CMPRS_XFER_ENABLE);
+    case TARGET_OBJECT_STATIC_TRACE_DATA:
+      if (type_read)
+	return
+	  (remote_protocol_packets[PACKET_qXfer_statictrace_read].xfer_status
+	   == CMPRS_XFER_ENABLE);
+      else
+	return 0;
+    case TARGET_OBJECT_AUXV:
+      return (remote_protocol_packets[PACKET_qXfer_auxv].xfer_status
+	      == CMPRS_XFER_ENABLE);
+    case TARGET_OBJECT_AVAILABLE_FEATURES:
+      return (remote_protocol_packets[PACKET_qXfer_features].xfer_status
+	      == CMPRS_XFER_ENABLE);
+    case TARGET_OBJECT_LIBRARIES:
+      return (remote_protocol_packets[PACKET_qXfer_libraries].xfer_status
+	      == CMPRS_XFER_ENABLE);
+    case TARGET_OBJECT_LIBRARIES_SVR4:
+      return (remote_protocol_packets[PACKET_qXfer_libraries_svr4].xfer_status
+	      == CMPRS_XFER_ENABLE);
+    case TARGET_OBJECT_MEMORY_MAP:
+      return (remote_protocol_packets[PACKET_qXfer_memory_map].xfer_status
+	      == CMPRS_XFER_ENABLE);
+    case TARGET_OBJECT_OSDATA:
+      gdb_assert (remote_desc);
+      return (remote_protocol_packets[PACKET_qXfer_osdata].xfer_status
+	      == CMPRS_XFER_ENABLE);
+    case TARGET_OBJECT_THREADS:
+      gdb_assert (annex == NULL);
+      return (remote_protocol_packets[PACKET_qXfer_threads].xfer_status
+	      == CMPRS_XFER_ENABLE);
+    case TARGET_OBJECT_TRACEFRAME_INFO:
+      gdb_assert (annex == NULL);
+      return (remote_protocol_packets[PACKET_qXfer_traceframe_info].xfer_status
+	      == CMPRS_XFER_ENABLE);
+    case TARGET_OBJECT_FDPIC:
+      return (remote_protocol_packets[PACKET_qXfer_fdpic].xfer_status
+	      == CMPRS_XFER_ENABLE);
+    case TARGET_OBJECT_OPENVMS_UIB:
+      return (remote_protocol_packets[PACKET_qXfer_uib].xfer_status
+	      == CMPRS_XFER_ENABLE);
+    default:
+      return 0;
+    }
+};
+
 /* Should we try the 'ThreadInfo' query packet?
 
    This variable (NOT available to the user: auto-detect only!)
@@ -3877,6 +3957,16 @@ remote_string_tracing_feature (const struct protocol_feature *feature,
   rs->string_tracing = (support == PACKET_ENABLE);
 }
 
+static void
+remote_compressed_xfer_feature (const struct protocol_feature *feature,
+				enum packet_support support,
+				const char *value)
+{
+  struct remote_state *rs = get_remote_state ();
+
+  rs->has_cmprs_xfer = (support != PACKET_DISABLE);
+}
+
 static struct protocol_feature remote_protocol_features[] = {
   { "PacketSize", PACKET_DISABLE, remote_packet_size, -1 },
   { "qXfer:auxv:read", PACKET_DISABLE, remote_supported_packet,
@@ -3944,6 +4034,7 @@ static struct protocol_feature remote_protocol_features[] = {
   { "QAgent", PACKET_DISABLE, remote_supported_packet, PACKET_QAgent},
   { "tracenz", PACKET_DISABLE,
     remote_string_tracing_feature, -1 },
+  { "compressedXfer", PACKET_DISABLE, remote_compressed_xfer_feature, -1 },
 };
 
 static char *remote_support_xml;
@@ -8443,9 +8534,12 @@ remote_read_qxfer (struct target_ops *ops, const char *object_name,
   static char *finished_object;
   static char *finished_annex;
   static ULONGEST finished_offset;
+  int xfer_ok = 1;
+  struct packet_config zpacket = *packet;
+  char *zname = NULL;
 
   struct remote_state *rs = get_remote_state ();
-  LONGEST i, n, packet_len;
+  LONGEST i, n, packet_len = 0;
 
   if (packet->support == PACKET_DISABLE)
     return -1;
@@ -8472,18 +8566,79 @@ remote_read_qxfer (struct target_ops *ops, const char *object_name,
      the target is free to respond with slightly less data.  We subtract
      five to account for the response type and the protocol frame.  */
   n = min (get_remote_packet_size () - 5, len);
-  snprintf (rs->buf, get_remote_packet_size () - 4, "qXfer:%s:read:%s:%s,%s",
-	    object_name, annex ? annex : "",
-	    phex_nz (offset, sizeof offset),
-	    phex_nz (n, sizeof n));
-  i = putpkt (rs->buf);
-  if (i < 0)
-    return -1;
 
-  rs->buf[0] = '\0';
-  packet_len = getpkt_sane (&rs->buf, &rs->buf_size, 0);
-  if (packet_len < 0 || packet_ok (rs->buf, packet) != PACKET_OK)
-    return -1;
+  /* If compressed object is supported, try it first.  */
+  if (rs->has_cmprs_xfer && packet->xfer_status != CMPRS_XFER_DISABLE)
+    {
+      /* When start to transfer a new object, fake a zread packet to detect
+         whether the remote stub support zread for this object.  */
+      if (packet->xfer_status == CMPRS_XFER_UNKNOWN)
+	{
+	  int zname_len = strlen(object_name) + 13;
+	  zname = (char *)xcalloc (zname_len, 1);
+	  snprintf (zname, zname_len, "qXfer:%s:zread", object_name);
+	  zpacket.name = zname;
+	  zpacket.support = PACKET_SUPPORT_UNKNOWN;
+	}
+
+      snprintf (rs->buf, get_remote_packet_size () - 4, "qXfer:%s:zread:%s:%s,%s",
+		object_name, annex ? annex : "",
+		phex_nz (offset, sizeof offset),
+		phex_nz (n, sizeof n));
+      i = putpkt (rs->buf);
+      if (i >= 0)
+	{
+	  rs->buf[0] = '\0';
+	  packet_len = getpkt_sane (&rs->buf, &rs->buf_size, 0);
+	  if (packet_len < 0 || packet_ok (rs->buf, &zpacket) != PACKET_OK)
+	    xfer_ok = 0;
+	}
+      else
+	xfer_ok = 0;
+
+      if (zname)
+	xfree (zname);
+
+      if (packet->xfer_status == CMPRS_XFER_ENABLE && xfer_ok == 0)
+	{
+	  /* We are in the middle of transferring a large object,
+	     the fail of current transfer will invalid all finished
+	     transfer, so we don't fall back to plain format.  */
+	  return -1;
+	}
+      else if (packet->xfer_status == CMPRS_XFER_UNKNOWN && xfer_ok == 1)
+	{
+	  /* If successfully transferred the first chunk in compressed format,
+	     change xfer_status of packet to tell next transfer that we decided
+	     to use compressed format to transfer remaining chunks, so the next
+	     transfer won't fall back to use plain format when it fails.  */
+	  packet->xfer_status = CMPRS_XFER_ENABLE;
+	}
+      else if (packet->xfer_status == CMPRS_XFER_UNKNOWN && xfer_ok == 0)
+	{
+	  /* If the first attempt fails,
+	     just fall back to use plain format.  */
+	  packet->xfer_status = CMPRS_XFER_DISABLE;
+	}
+    }
+
+  /* The compressed object isn't supported, fall back to use plain format.  */
+  if (rs->has_cmprs_xfer == 0 || packet->xfer_status == CMPRS_XFER_DISABLE)
+    {
+      snprintf (rs->buf, get_remote_packet_size () - 4, "qXfer:%s:read:%s:%s,%s",
+		object_name, annex ? annex : "",
+		phex_nz (offset, sizeof offset),
+		phex_nz (n, sizeof n));
+
+      i = putpkt (rs->buf);
+      if (i < 0)
+	return -1;
+
+      rs->buf[0] = '\0';
+      packet_len = getpkt_sane (&rs->buf, &rs->buf_size, 0);
+      if (packet_len < 0 || packet_ok (rs->buf, packet) != PACKET_OK)
+	return -1;
+    }
 
   if (rs->buf[0] != 'l' && rs->buf[0] != 'm')
     error (_("Unknown remote qXfer reply: %s"), rs->buf);
diff --git a/gdb/target.c b/gdb/target.c
index bb8eae8..245cb9e 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -43,6 +43,9 @@
 #include "tracepoint.h"
 #include "gdb/fileio.h"
 #include "agent.h"
+#ifdef HAVE_ZLIB_H
+#include <zlib.h>
+#endif
 
 static void target_info (char *, int);
 
@@ -1998,6 +2001,80 @@ target_write_partial (struct target_ops *ops,
   return target_xfer_partial (ops, object, annex, NULL, buf, offset, len);
 }
 
+/* Decompress the zlib-compressed buffer buf_p.
+   Inside the buf_p, the length of received compressed data is data_size.
+   If decompress successfully, free the buffer pointed by buf_p and redirect
+   buf_p to the new decompressed data buffer.
+   The size of decompressed data will be returned.
+   Otherwise the buf_p won't be touched and simply return data_size.  */
+
+static unsigned int
+target_decompress_buf (gdb_byte **buf_p, size_t data_size)
+{
+#ifndef HAVE_ZLIB_H
+  error (_("Support for zlib-compressed data is disabled in this copy of GDB"));
+#else
+  size_t ret, uncompressed_size, compressed_size, i;
+  gdb_byte *buf;
+
+  if (remote_debug)
+    {
+      int i;
+
+      fprintf_unfiltered (gdb_stdlog,
+			  "Zlib-compressed packet received with size %d bytes: ",
+			  data_size);
+
+      for (i = 0; i < data_size; i++)
+	fprintf_unfiltered (gdb_stdlog, "%02x", (unsigned char)(*buf_p)[i]);
+
+      fprintf_unfiltered (gdb_stdlog, "\n");
+    }
+
+  /* For zlib-based compressed buffer, we assume that the first 8 bytes are
+     used to encode the length of uncompressed data, and the following 4 bytes
+     encode the zlib head, so the total length should be bigger than 12.  */
+  if (data_size <= 12)
+    error (_("Zlib Error: the data size %d is too short to decompress"),
+	   data_size);
+
+  uncompressed_size =  (*buf_p)[0];
+  uncompressed_size += (*buf_p)[1]<<4;
+  uncompressed_size += (*buf_p)[2]<<8;
+  uncompressed_size += (*buf_p)[3]<<12;
+  uncompressed_size += (*buf_p)[4]<<16;
+  uncompressed_size += (*buf_p)[5]<<20;
+  uncompressed_size += (*buf_p)[6]<<24;
+  uncompressed_size += (*buf_p)[7]<<28;
+
+  compressed_size = data_size - 8;
+
+  /* The extra 1 byte is to cope with the existing alloc strategy which always
+     alloc more spaces and have code like buffer[len] = 0.  */
+  buf = xmalloc (uncompressed_size + 1);
+
+  ret = uncompress ((Bytef *)buf, (uLongf *)&uncompressed_size,
+		    (const Bytef *)((*buf_p) + 8), (uLong)compressed_size);
+
+  if (ret == Z_OK)
+    {
+      if (remote_debug)
+	fprintf_unfiltered (gdb_stdlog, "Packet decompressed in size %d bytes as: %s\n",
+			    uncompressed_size, (char *)buf);
+
+      xfree (*buf_p);
+      *buf_p = buf;
+      return uncompressed_size;
+    }
+  else
+    {
+      xfree (buf);
+      gdb_zlib_error (ret);
+      return data_size;
+    }
+#endif
+}
+
 /* Wrappers to perform the full transfer.  */
 
 /* For docs on target_read see target.h.  */
@@ -2372,6 +2449,13 @@ target_read_stralloc (struct target_ops *ops, enum target_object object,
   if (transferred == 0)
     return xstrdup ("");
 
+  /* If it is compressed object, need to decompress it.
+     The first 8 bytes encode the size of uncompressed data.
+     The following 4 bytes encode the head of zlib-based compressed
+     buffer.  */
+  if (transferred > 12 && remote_packet_is_cmprs_xfer (object, annex, 1))
+    transferred = target_decompress_buf (&buffer, transferred);
+
   buffer[transferred] = 0;
 
   /* Check for embedded NUL bytes; but allow trailing NULs.  */
diff --git a/gdb/target.h b/gdb/target.h
index 54c58d6..cf07f38 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -1861,6 +1861,10 @@ extern struct target_ops *find_target_beneath (struct target_ops *);
 
 extern char *target_get_osdata (const char *type);
 
+/* From remote.c */
+
+extern int remote_packet_is_cmprs_xfer (enum target_object, const char *, int);
+
 \f
 /* Stuff that should be shared among the various remote targets.  */
 
diff --git a/gdb/utils.c b/gdb/utils.c
index 5566149..3703607 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -78,6 +78,10 @@
 #include "interps.h"
 #include "gdb_regex.h"
 
+#ifdef HAVE_ZLIB_H
+#include <zlib.h>
+#endif
+
 #if !HAVE_DECL_MALLOC
 extern PTR malloc ();		/* ARI: PTR */
 #endif
@@ -3893,3 +3897,39 @@ _initialize_utils (void)
   add_internal_problem_command (&internal_error_problem);
   add_internal_problem_command (&internal_warning_problem);
 }
+
+/* Report zlib error message.  */
+
+void
+gdb_zlib_error (int ret_code)
+{
+#ifndef HAVE_ZLIB_H
+  error (_("Zlib support is disabled in this copy of GDB"));
+#else
+  switch (ret_code)
+    {
+    case Z_OK:
+      break;
+    case Z_ERRNO:
+      if (ferror(stdin))
+	error (_("Zlib Error: error reading stdin"));
+      if (ferror(stdout))
+	error (_("Zlib Error: error writing stdout"));
+      break;
+    case Z_STREAM_ERROR:
+      error (_("Zlib Error: invalid compression level"));
+      break;
+    case Z_DATA_ERROR:
+      error (_("Zlib Error: invalid or incomplete deflate data"));
+      break;
+    case Z_MEM_ERROR:
+      error (_("Zlib Error: out of memory"));
+      break;
+    case Z_VERSION_ERROR:
+      error (_("Zlib Error: zlib version mismatch"));
+      break;
+    default:
+      error (_("Zlib Error: unknown error code"));
+    }
+#endif
+}

Re: [Patch] Enable GDB remote protocol to support zlib-compressed xml

[Jonathan Larmour]

Hi Terry,

On 08/08/12 09:32, Terry Guo wrote:
> Hi,
> 
> This patch intends to implement a new feature discussed in thread
> http://sourceware.org/ml/gdb-patches/2012-06/msg00271.html. With this patch,
> stub can return host gdb with zlib-compressed object which consumes less
> space and communication overhead compared to its uncompressed format.

Thanks for your work on this. I have some observations...

In the documentation, may I suggest some slightly clearer wording:

-=-=-=-=-=-=-=-=-
@item compressedXfer
The remote stub understands the @samp{qXfer:object:zread:annex:offset,length}
packet, which is similar to @samp{qXfer:object:read:annex:offset:length}
(@pxref{qXfer read}) but with the object contents in the response packet
compressed as a zlib deflated stream, prepended with the uncompressed length
of the whole object.  The length in the @samp{qXfer:zread} request refers to
the maximum allowed packet size.  The uncompressed length is represented as a
64-bit value in little-endian byte order (that is, with the first byte being
the least significant byte of the value, and the eighth byte being the most
significant byte of the value).

It is not compulsory for a remote stub to allow compressed responses for all
responses: if this feature is set, the host GDB will always try @samp{zread}
first and fall back to plain @samp{read} if a null response is received from
the remote stub.
-=-=-=-=-=-=-=-=-
and later:
-=-=-=-=-=-=-=-=-
@var{zread} works in a similar way to @var{read} except that the transferred
objects must be compressed as a zlib deflated stream, prepended by the
uncompressed length of the object.  This can allow remote stubs to reduce
their memory footprint by returning pre-generated zlib-compressed data, as
well as reducing communications overhead.  @var{zread} can only be used when
the host gdb is built with Zlib support.
-=-=-=-=-=-=-=-=-

Although thinking about the uncompressed length, every other value in GDB's
remote protocol which is endian specific always uses big endian, not little
endian. So I recommend changing that (and if so, also my suggested replacement
documentation wording above).

For the code, I don't think you can call from target.c into remote.c for
remote_packet_is_comprs_xfer(). remote is only one target vector. I think the
GDB maintainers would consider this a layering violation, but hopefully one of
them will be able to confirm for definite. I think the only sensible
alternative is to perform the decompression within remote_read_qxfer.

The code currently assumes that once we know whether compressed data is or is
not supported for a particular packet, that setting is used for all objects
used with that packet. Is that a good assumption? Just because one object is
compressed, must we insist that all objects are compressed? Or vice versa.
This is somewhat theoretical given that initially we are only concerned with
features.xml, but if this is to be a general extension, this requirement for
the stub should be clear (and documented).

What about remote_write_qxfer()?

In gdb_zlib_error() I don't think zlib is likely to be using stdin/stdout, so
I doubt the handling of Z_ERRNO is correct. And in any case, there should have
been an 'else' clause to handle errors which aren't ferrors for stdin/stdout
anyway.

You also need to handle Z_BUF_ERROR.

Hope these comments are useful.

Jifl
-- 
eCosCentric Limited      http://www.eCosCentric.com/     The eCos experts
Barnwell House, Barnwell Drive, Cambridge, UK.       Tel: +44 1223 245571
Registered in England and Wales: Reg No 4422071.
------["Si fractum non sit, noli id reficere"]------       Opinions==mine

RE: [Patch] Enable GDB remote protocol to support zlib-compressed xml

[Terry Guo]

Hi Jonathan,

Your comments are great helpful. Thanks very much.
 
> 
> In the documentation, may I suggest some slightly clearer wording:
> 
> -=-=-=-=-=-=-=-=-
> @item compressedXfer
> The remote stub understands the
> @samp{qXfer:object:zread:annex:offset,length}
> packet, which is similar to @samp{qXfer:object:read:annex:offset:length}
> (@pxref{qXfer read}) but with the object contents in the response
> packet
> compressed as a zlib deflated stream, prepended with the uncompressed
> length
> of the whole object.  The length in the @samp{qXfer:zread} request
> refers to
> the maximum allowed packet size.  The uncompressed length is
> represented as a
> 64-bit value in little-endian byte order (that is, with the first byte
> being
> the least significant byte of the value, and the eighth byte being the
> most
> significant byte of the value).
> 
> It is not compulsory for a remote stub to allow compressed responses
> for all
> responses: if this feature is set, the host GDB will always try
> @samp{zread}
> first and fall back to plain @samp{read} if a null response is received
> from
> the remote stub.
> -=-=-=-=-=-=-=-=-
> and later:
> -=-=-=-=-=-=-=-=-
> @var{zread} works in a similar way to @var{read} except that the
> transferred
> objects must be compressed as a zlib deflated stream, prepended by the
> uncompressed length of the object.  This can allow remote stubs to
> reduce
> their memory footprint by returning pre-generated zlib-compressed data,
> as
> well as reducing communications overhead.  @var{zread} can only be used
> when
> the host gdb is built with Zlib support.
> -=-=-=-=-=-=-=-=-
> 

Much better than mine. I will use them.

> Although thinking about the uncompressed length, every other value in
> GDB's
> remote protocol which is endian specific always uses big endian, not
> little
> endian. So I recommend changing that (and if so, also my suggested
> replacement
> documentation wording above).
> 

OK. Will go with big endian assumption.

> For the code, I don't think you can call from target.c into remote.c
> for
> remote_packet_is_comprs_xfer(). remote is only one target vector. I
> think the
> GDB maintainers would consider this a layering violation, but hopefully
> one of
> them will be able to confirm for definite. I think the only sensible
> alternative is to perform the decompression within remote_read_qxfer.
> 

I also think it is a layering violation and want to avoid it. But I couldn't
find a way. 
Suppose we have a very large target.xml file which needs multiple transfers,
the stub may has two options to go:
a) compress it as a whole and then transfer it piece by piece 
or 
b) split and compress each pieces and then transfer them one by one.

If we go with option a), the remote_read_qxfer can't help because each
received packets are just part of a big zlib-compressed object. Only
function target_read_alloc_1 knows the end of the transfer of this big file.

If we go with option b), the remote_read_qxfer can help because it knows
each received packets are zlib-compressed.

> The code currently assumes that once we know whether compressed data is
> or is
> not supported for a particular packet, that setting is used for all
> objects
> used with that packet. Is that a good assumption? Just because one
> object is
> compressed, must we insist that all objects are compressed? Or vice
> versa.
> This is somewhat theoretical given that initially we are only concerned
> with
> features.xml, but if this is to be a general extension, this
> requirement for
> the stub should be clear (and documented).
> 

Maybe my code isn't so clear. But my intentions are:

a). The compressedXfer is a general stub feature and not specific to any
kind of gdb packets. Suppose the object for PACKET_qXfer_osdata will be
transferred in zlib-compressed format while the object for
PACKET_qXfer_libraries is still in plain format, once I know the status of
PACKET_qXfer_osdata, I won't assume same status for PACKET_qXfer_libraries,
I will still try zread first to detect its actual status.

b). If the object for PACKET_qXfer_libraries is very big and needs multiple
transfer, I will assume the remaining parts will be transferred in
compressed format once I know the first part is transferred in compressed
format.

> What about remote_write_qxfer()?
> 
I think the remote_wirte_qxfer means host gdb transfers zlib-compressed data
to gdb stub. This will need stub to spare more memory for zlib functions.
I can start to do it once the solution for read is accepted and there are
stubs that support decompress function. Otherwise I can't get a stub to
debug my code.

> In gdb_zlib_error() I don't think zlib is likely to be using
> stdin/stdout, so
> I doubt the handling of Z_ERRNO is correct. And in any case, there
> should have
> been an 'else' clause to handle errors which aren't ferrors for
> stdin/stdout
> anyway.
> 
> You also need to handle Z_BUF_ERROR.
> 

OK. I will update the way to handle Z_ERRNO.

BR,
Terry