From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 29419 invoked by alias); 25 Apr 2014 08:51:24 -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 29402 invoked by uid 89); 25 Apr 2014 08:51:22 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.6 required=5.0 tests=AWL,BAYES_00 autolearn=ham version=3.3.2 X-HELO: relay1.mentorg.com Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 25 Apr 2014 08:51:21 +0000 Received: from svr-orw-exc-10.mgc.mentorg.com ([147.34.98.58]) by relay1.mentorg.com with esmtp id 1Wdbqn-0000Du-JT from Yao_Qi@mentor.com ; Fri, 25 Apr 2014 01:51:17 -0700 Received: from SVR-ORW-FEM-02.mgc.mentorg.com ([147.34.96.206]) by SVR-ORW-EXC-10.mgc.mentorg.com with Microsoft SMTPSVC(6.0.3790.4675); Fri, 25 Apr 2014 01:51:17 -0700 Received: from qiyao.dyndns.org (147.34.91.1) by svr-orw-fem-02.mgc.mentorg.com (147.34.96.168) with Microsoft SMTP Server id 14.2.247.3; Fri, 25 Apr 2014 01:51:16 -0700 Message-ID: <535A2169.7020905@codesourcery.com> Date: Fri, 25 Apr 2014 08:51:00 -0000 From: Yao Qi User-Agent: Mozilla/5.0 (X11; Linux i686; rv:24.0) Gecko/20100101 Thunderbird/24.4.0 MIME-Version: 1.0 To: Pedro Alves CC: Subject: Re: [PATCH] Partially available/unavailable data in requested range References: <1397998086-750-1-git-send-email-yao@codesourcery.com> <53593996.2020806@redhat.com> In-Reply-To: <53593996.2020806@redhat.com> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2014-04/txt/msg00527.txt.bz2 On 04/25/2014 12:19 AM, Pedro Alves wrote: > It'd be best if unavailable.cc/unavailable.exp covers this scenario > as well. It's meant to be complete in this sort of partial > collection stuff. Could you do that, please ? > Sure. I didn't add a test case for it because mi-available-children-only.exp can cover it, but looks the review to "available-children-only" series is slow-moving. I'll add test to unavailable.exp. unavailable.exp test is only for live target, while the bug this patch tries to fix is about trace file targets. I'll factor unavailable.exp out for tfile and ctf targets first. >> > + /* There should be at least one block within desired range, and >> > + range [OFFSET, MIN_ADDR_AVAILABLE) is unavailable. Tell >> > + caller about it and caller will request memory from >> > + MIN_ADDR_AVAILABLE. */ >> > + if (offset < min_addr_available) >> > + { >> > + *xfered_len = min_addr_available - offset; >> > + return TARGET_XFER_UNAVAILABLE; >> > + } > I find the comment above confusing and hard to grok. :-/ > > - "There should be" sounds like an assertion, which this is not. > > - Comments that assume the condition is true are better place > _within_ the then block. Comments that appear before > the condition usually are more naturally of the > > /* if $human-understandable-version-of-the-condition then > do something. */ > > form. > > - A few "the"'s are missing. > > So I think this would be much clearer: > > if (offset < min_addr_available) > { > /* There's at least one block containing the desired range > but the range [OFFSET, MIN_ADDR_AVAILABLE) is > unavailable. Return that and GDB will re-request memory > starting at MIN_ADDR_AVAILABLE. */ > *xfered_len = min_addr_available - offset; > return TARGET_XFER_UNAVAILABLE; > } > > > But, looking deeper, I don't think the patch is correct, actually. > > Even if the range [OFFSET, MIN_ADDR_AVAILABLE) is > not unavailable in the context of traceframes, that > range might fall within a read-only section, so we should > still try falling back to reading from the executable file. > I thought of this when I wrote this patch. I was unable to figure out your trim-LEN-up trick, and get the code complicated. Given "requesting memory across sections" is a rare case, I didn't go on this track further. > So it seems to me what we need to do is trim LEN up to > the first available address. Then if reading from the > executable still yields nothing, the > > else > { > /* No use trying further, we know some memory starting > at MEMADDR isn't available. */ > *xfered_len = len; > return TARGET_XFER_UNAVAILABLE; > } > > part returns the corrected LEN. That is, I think the below > would be both simpler, and more correct. (I also think Yes, that is much simpler. > first_addr_available is clearer than min_addr_available"). I don't think "first" is clearer than "min". There are multiple 'M' blocks in a traceframe, and the address of some of them are within the desired range [OFFSET, OFFSET + LEN). We are looking for the minimal address within the range, instead of the first address within the range. For example, supposing we have three 'M' blocks, M1 (0x01 0x02), M2 (0x07, 0x08) and M3 (0x4, 0x05), and the requested range is [0x03, 0x09), the first 'M' block within this range is M2, while the minimal address of 'M' block is M3. M3 is what we are looking for. > > Completely untested, but should give you the idea. > > -------------- > diff --git c/gdb/tracefile-tfile.c w/gdb/tracefile-tfile.c > index efa69b2..e570b10 100644 > --- c/gdb/tracefile-tfile.c > +++ w/gdb/tracefile-tfile.c > @@ -853,6 +853,8 @@ tfile_xfer_partial (struct target_ops *ops, enum target_object object, > { > int pos = 0; > enum target_xfer_status res; > + /* Records the first available address of all blocks. */ > + ULONGEST first_addr_available = 0; > > /* Iterate through the traceframe's blocks, looking for > memory. */ > @@ -886,13 +888,18 @@ tfile_xfer_partial (struct target_ops *ops, enum target_object object, > return TARGET_XFER_OK; > } > > + if (first_addr_available == 0 || maddr < first_addr_available) > + first_addr_available = maddr; > + In my patch, there is one more condition check if (offset < maddr && maddr < (offset + len)) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ if (min_addr_available == 0 || min_addr_available > maddr) min_addr_available = maddr; to avoid recoding minimal address *outside* of requested range. For example, we have three 'M' blocks, M1 (0x01 0x02), M2 (0x07, 0x08) and M3 (0x04, 0x05), and the requested range is [0x03, 0x09). OFFSET |--- requested ---| |-M1-| |-M2-| |-M3-| The MIN_ADDR_AVAILABLE is expected to be 0x04 instead of 0x01. I'll post a patch soon. -- Yao (齐尧)