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

[Jonathan Larmour <jifl at eCosCentric dot com>]

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