From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 21206 invoked by alias); 9 Aug 2012 06:16:26 -0000 Received: (qmail 21196 invoked by uid 22791); 9 Aug 2012 06:16:24 -0000 X-SWARE-Spam-Status: No, hits=-2.8 required=5.0 tests=AWL,BAYES_00,KHOP_THREADED,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from hagrid.ecoscentric.com (HELO mail.ecoscentric.com) (212.13.207.197) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 09 Aug 2012 06:16:11 +0000 Received: from localhost (hagrid.ecoscentric.com [127.0.0.1]) by mail.ecoscentric.com (Postfix) with ESMTP id AA69C2F78007; Thu, 9 Aug 2012 07:16:09 +0100 (BST) Received: from mail.ecoscentric.com ([127.0.0.1]) by localhost (hagrid.ecoscentric.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id nuQYZ7Syd58d; Thu, 9 Aug 2012 07:16:06 +0100 (BST) Received: from lert.jifvik.org (jifvik.dyndns.org [85.158.45.40]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) (Authenticated sender: jlarmour@ecoscentric.com) by mail.ecoscentric.com (Postfix) with ESMTP id D39032F78005; Thu, 9 Aug 2012 07:16:03 +0100 (BST) Message-ID: <5023559B.1070709@eCosCentric.com> Date: Thu, 09 Aug 2012 06:16:00 -0000 From: Jonathan Larmour User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.15) Gecko/20101027 Fedora/3.0.10-1.fc12 Lightning/1.0b2pre Thunderbird/3.0.10 MIME-Version: 1.0 To: Terry Guo CC: gdb-patches@sourceware.org Subject: Re: [Patch] Enable GDB remote protocol to support zlib-compressed xml References: <000401cd7540$59679610$0c36c230$@guo@arm.com> In-Reply-To: <000401cd7540$59679610$0c36c230$@guo@arm.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2012-08/txt/msg00279.txt.bz2 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