From patchwork Thu Apr 24 16:19:34 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 669 Return-Path: X-Original-To: siddhesh@wilcox.dreamhost.com Delivered-To: siddhesh@wilcox.dreamhost.com Received: from homiemail-mx22.g.dreamhost.com (mx2.sub5.homie.mail.dreamhost.com [208.113.200.128]) by wilcox.dreamhost.com (Postfix) with ESMTP id 26F43361336 for ; Thu, 24 Apr 2014 09:19:44 -0700 (PDT) Received: by homiemail-mx22.g.dreamhost.com (Postfix, from userid 14314964) id C3BD74EDFF06; Thu, 24 Apr 2014 09:19:43 -0700 (PDT) X-Original-To: gdb@patchwork.siddhesh.in Delivered-To: x14314964@homiemail-mx22.g.dreamhost.com Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by homiemail-mx22.g.dreamhost.com (Postfix) with ESMTPS id 862334EDFF03 for ; Thu, 24 Apr 2014 09:19:43 -0700 (PDT) DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:message-id:date:from:mime-version:to:cc :subject:references:in-reply-to:content-type :content-transfer-encoding; q=dns; s=default; b=USFVj564vRPM4v6B b3dPERMZv8S2mzmHYqW554o2KsVW/QV4VDexWIRAhu04bnvmJsqsWK/wTY/LGfhq 7fCLoP9kbbLu7sq6SqFov1hrE7ANORlLpUBejHtKCB5I0Qu/4MCKSmeUkFRqZ3FO xRZ0Jpf4Q0t9KfYLgcClXKA4bIE= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:message-id:date:from:mime-version:to:cc :subject:references:in-reply-to:content-type :content-transfer-encoding; s=default; bh=/DUjaCfPHIBKJmr0RSigo/ 66JMI=; b=R7HD0txwyWuBDyfSZzNfEWt7jyFtbhhCo3ywUbd44g+JUeEIrDMnWp RDiNVWjt1n/YiGt/FNhG7m8naiCNbv6fani3SydNhRf36x482rF6pLL/6BLVFmge C1d7qmjQwRtysK30LpC7rgV16iRkP4utKhjOSBgnczn3q8loGpna8= Received: (qmail 25318 invoked by alias); 24 Apr 2014 16:19:42 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 25304 invoked by uid 89); 24 Apr 2014 16:19:41 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.1 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, 24 Apr 2014 16:19:40 +0000 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s3OGJaDf006409 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Thu, 24 Apr 2014 12:19:37 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx01.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id s3OGJZIG030979; Thu, 24 Apr 2014 12:19:35 -0400 Message-ID: <53593996.2020806@redhat.com> Date: Thu, 24 Apr 2014 17:19:34 +0100 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] Partially available/unavailable data in requested range References: <1397998086-750-1-git-send-email-yao@codesourcery.com> In-Reply-To: <1397998086-750-1-git-send-email-yao@codesourcery.com> X-DH-Original-To: gdb@patchwork.siddhesh.in On 04/20/2014 01:48 PM, Yao Qi wrote: > (gdb) p s > $1 = {a = 0, b = , s1 = {s3 = {g = 0, h = }, d = 0}, s2 = {e = , f = 0}} > > I don't add a test case for it because mi-available-children-only.exp > will cover it. 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 ? > + /* 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. 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 first_addr_available is clearer than min_addr_available"). 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; + /* Skip over this block. */ pos += (8 + 2 + mlen); } /* Requested memory is unavailable in the context of traceframes, and this address falls within a read-only section, fallback - to reading from executable. */ + to reading from executable, up to FIRST_ADDR_AVAILABLE. */ + if (offset < first_addr_available) + len = min (len, first_addr_available - offset); res = exec_read_partial_read_only (readbuf, offset, len, xfered_len); if (res == TARGET_XFER_OK)