From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 23535 invoked by alias); 6 Feb 2014 14:44:48 -0000 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 Received: (qmail 23525 invoked by uid 89); 6 Feb 2014 14:44:47 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-2.0 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 06 Feb 2014 14:44:46 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s16Eig4L021857 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Thu, 6 Feb 2014 09:44:43 -0500 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s16EieEk005644; Thu, 6 Feb 2014 09:44:41 -0500 Message-ID: <52F39FD8.2070507@redhat.com> Date: Thu, 06 Feb 2014 14:44:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130625 Thunderbird/17.0.7 MIME-Version: 1.0 To: Yao Qi CC: gdb-patches@sourceware.org Subject: Re: [PATCH 5/6] Return target_xfer_status in to_xfer_partial References: <1391139325-2758-1-git-send-email-yao@codesourcery.com> <1391139325-2758-6-git-send-email-yao@codesourcery.com> In-Reply-To: <1391139325-2758-6-git-send-email-yao@codesourcery.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-SW-Source: 2014-02/txt/msg00095.txt.bz2 On 01/31/2014 03:35 AM, Yao Qi wrote: > This patch does the conversion of to_xfer_partial from > > LONGEST (*to_xfer_partial) (struct target_ops *ops, > enum target_object object, const char *annex, > gdb_byte *readbuf, const gdb_byte *writebuf, > ULONGEST offset, ULONGEST len); > > to > > enum target_xfer_status (*to_xfer_partial) (struct target_ops *ops, > enum target_object object, const char *annex, > gdb_byte *readbuf, const gdb_byte *writebuf, > ULONGEST offset, ULONGEST len, ULONGEST *xfered_len); > > It changes to_xfer_partial return the transfer status and the transfered > length by *XFERED_LEN. Generally, the return status has three stats, > > - TARGET_XFER_OK, > - TARGET_XFER_DONE, > - TARGET_XFER_E_XXXX, I was reading this and thinking "what's the difference between OK and DONE again?" ... > See the comments to them in 'enum target_xfer_status'. Note that > Pedro suggested not name TARGET_XFER_DONE, as it is confusing, > compared with "TARGET_XFER_OK". I still choose "done" because it has > been used for a while. Right. :-) This DONE status indicates that no further transfer is possible. How about we call it TARGET_XFER_END or TARGET_XFER_EOF ? EOF is pretty much universally understood I think. I believe that'd be much clearer. > size -= offset; > if (size > len) > size = len; > @@ -714,7 +714,13 @@ core_xfer_partial (struct target_ops *ops, enum target_object object, > return TARGET_XFER_E_IO; > } > > - return size; > + if (size == 0) > + return TARGET_XFER_DONE; > + else > + { > + *xfered_len = (ULONGEST) size; > + return TARGET_XFER_OK; > + } If we move this size == 0 check above, we simplify the code above too. Like: if (size > len) size = len; + if (size == 0) + return TARGET_XFER_DONE; - if (size > 0 - && !bfd_get_section_contents (core_bfd, section, readbuf, + if (!bfd_get_section_contents (core_bfd, section, readbuf, (file_ptr) offset, size)) { warning (_("Couldn't read NT_AUXV note in core file.")); return TARGET_XFER_E_IO; } - return size; + *xfered_len = (ULONGEST) size; + return TARGET_XFER_OK; > } > return TARGET_XFER_E_IO; > > @@ -746,7 +752,13 @@ core_xfer_partial (struct target_ops *ops, enum target_object object, > return TARGET_XFER_E_IO; > } > > - return size; > + if (size == 0) > + return TARGET_XFER_DONE; > + else > + { > + *xfered_len = (ULONGEST) size; > + return TARGET_XFER_OK; > + } > } Likewise. > > @@ -793,7 +822,7 @@ core_xfer_partial (struct target_ops *ops, enum target_object object, > > size = bfd_section_size (core_bfd, section); > if (offset >= size) > - return 0; > + return TARGET_XFER_DONE; > size -= offset; > if (size > len) > size = len; > @@ -805,7 +834,13 @@ core_xfer_partial (struct target_ops *ops, enum target_object object, > return TARGET_XFER_E_IO; > } > > - return size; > + if (size == 0) > + return TARGET_XFER_DONE; > + else > + { > + *xfered_len = (ULONGEST) size; > + return TARGET_XFER_OK; > + } Ditto. > -static LONGEST > +static enum target_xfer_status > linux_nat_xfer_osdata (struct target_ops *ops, enum target_object object, > const char *annex, gdb_byte *readbuf, > - const gdb_byte *writebuf, ULONGEST offset, ULONGEST len) > + const gdb_byte *writebuf, ULONGEST offset, ULONGEST len, > + ULONGEST *xfered_len) > { > gdb_assert (object == TARGET_OBJECT_OSDATA); > > - return linux_common_xfer_osdata (annex, readbuf, offset, len); > + *xfered_len = linux_common_xfer_osdata (annex, readbuf, offset, len); > + if (*xfered_len== 0) Missing space. > -static LONGEST > +static enum target_xfer_status > target_read_live_memory (enum target_object object, > - ULONGEST memaddr, gdb_byte *myaddr, ULONGEST len) > + ULONGEST memaddr, gdb_byte *myaddr, ULONGEST len, > + ULONGEST *xfered_len) > { > - LONGEST ret; > + enum target_xfer_status ret; > struct cleanup *cleanup; > > /* Switch momentarily out of tfind mode so to access live memory. > @@ -1326,8 +1327,8 @@ target_read_live_memory (enum target_object object, > cleanup = make_cleanup_restore_traceframe_number (); > set_traceframe_number (-1); > > - ret = target_read (current_target.beneath, object, NULL, > - myaddr, memaddr, len); > + ret = target_xfer_partial (current_target.beneath, object, NULL, > + myaddr, NULL, memaddr, len, xfered_len); This doesn't seem equivalent? > > if (readbuf) > myaddr = readbuf; > if (writebuf) > myaddr = writebuf; > - if (retval > 0 && myaddr != NULL) > + if (retval == TARGET_XFER_OK && myaddr != NULL) > { > int i; > > fputs_unfiltered (", bytes =", gdb_stdlog); > - for (i = 0; i < retval; i++) > + for (i = 0; i < *xfered_len; i++) > { > if ((((intptr_t) &(myaddr[i])) & 0xf) == 0) > { > @@ -1763,12 +1786,19 @@ target_xfer_partial (struct target_ops *ops, > > fputc_unfiltered ('\n', gdb_stdlog); > } > + > + /* Check implementations of to_xfer_partial update *XFERED_LEN > + properly. Do assertion after printing debug messages, so that we > + can find more clues on assertion failure from debugging messages. */ > + if (retval == TARGET_XFER_OK || retval == TARGET_XFER_E_UNAVAILABLE) > + gdb_assert (*xfered_len > 0); Indentation looks a little odd here. > /* Wrappers to perform the full transfer. */ > @@ -2058,16 +2098,22 @@ target_read (struct target_ops *ops, > > while (xfered < len) > { > - LONGEST xfer = target_read_partial (ops, object, annex, > - (gdb_byte *) buf + xfered, > - offset + xfered, len - xfered); > + ULONGEST xfered_len; > + enum target_xfer_status status; > + > + status = target_read_partial (ops, object, annex, > + (gdb_byte *) buf + xfered, > + offset + xfered, len - xfered, > + &xfered_len); > > /* Call an observer, notifying them of the xfer progress? */ > - if (xfer == 0) > + if (status == TARGET_XFER_DONE) > return xfered; > - if (xfer < 0) > + if (TARGET_XFER_STATUS_ERROR_P (status)) > return -1; > - xfered += xfer; > + > + gdb_assert (status == TARGET_XFER_OK); > + xfered += xfered_len; I think you can avoid all these TARGET_XFER_SATUS_ERROR_P uses by simply reversing the checks. E.g.,: if (status == TARGET_XFER_DONE) return xfered; else if (status == TARGET_XFER_OK) { xfered += xfered_len; QUIT; } else return -1; > } > return len; > @@ -2104,6 +2150,7 @@ read_whatever_is_readable (struct target_ops *ops, > ULONGEST current_end = end; > int forward; > memory_read_result_s r; > + ULONGEST xfered_len; > > /* If we previously failed to read 1 byte, nothing can be done here. */ > if (end - begin <= 1) > @@ -2116,13 +2163,16 @@ read_whatever_is_readable (struct target_ops *ops, > if not. This heuristic is meant to permit reading accessible memory > at the boundary of accessible region. */ > if (target_read_partial (ops, TARGET_OBJECT_MEMORY, NULL, > - buf, begin, 1) == 1) > + buf, begin, 1, &xfered_len) == TARGET_XFER_OK > + && xfered_len == 1) Didn't we assert that TARGET_XFER_OK transfers more than 0 ? Seems like this "xfered_len == 1" check is redundant. > { > forward = 1; > ++current_begin; > } > else if (target_read_partial (ops, TARGET_OBJECT_MEMORY, NULL, > - buf + (end-begin) - 1, end - 1, 1) == 1) > + buf + (end-begin) - 1, end - 1, 1, > + &xfered_len) == TARGET_XFER_OK > + && xfered_len == 1) Likewise. > > -/* Possible error codes returned by target_xfer_partial, etc. */ > +/* Possible values returned by target_xfer_partial, etc. */ > > -enum target_xfer_error > +enum target_xfer_status > { > + /* Some bytes are transferred. */ > + TARGET_XFER_OK = 1, > + > + /* No further transfer is possible. */ > + TARGET_XFER_DONE = 0, > + > /* Generic I/O error. Note that it's important that this is '-1', > as we still have target_xfer-related code returning hardcoded > '-1' on error. */ > @@ -219,9 +225,11 @@ enum target_xfer_error > /* Keep list in sync with target_xfer_error_to_string. */ > }; > > +#define TARGET_XFER_STATUS_ERROR_P(STATUS) (STATUS) < TARGET_XFER_DONE Wrap in ()'s: #define TARGET_XFER_STATUS_ERROR_P(STATUS) ((STATUS) < TARGET_XFER_DONE) > /* Request that OPS transfer up to LEN 8-bit bytes of the target's > OBJECT. The OFFSET, for a seekable object, specifies the > @@ -518,10 +527,11 @@ struct target_ops > starting point. The ANNEX can be used to provide additional > data-specific information to the target. > > - Return the number of bytes actually transfered, zero when no > - further transfer is possible, and a negative error code (really > - an 'enum target_xfer_error' value) when the transfer is not > - supported. Return of a positive value smaller than LEN does > + Return the transferred status, error or OK (an > + 'enum target_xfer_status' value). Save the number of bytes > + actually transferred in *XFERED_LEN if transfer is successful > + (TARGET_XFER_OK) or the requested data is unavailable > + (TARGET_XFER_E_UNAVAILABLE). *XFERED_LEN smaller than LEN does ... Save the number of bytes actually transferred in *XFERED_LEN if transfer is successful (TARGET_XFER_OK) or the number of unavailable bytes if the requested data is unavailable (TARGET_XFER_E_UNAVAILABLE). ... Otherwise looks good to me. Thanks a lot for doing this! -- Pedro Alves