From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 13858 invoked by alias); 19 Jun 2013 15:50:42 -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 13791 invoked by uid 89); 19 Jun 2013 15:50:37 -0000 X-Spam-SWARE-Status: No, score=-8.1 required=5.0 tests=AWL,BAYES_00,KHOP_THREADED,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.1 Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Wed, 19 Jun 2013 15:50:36 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r5JFoXuw020354 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Wed, 19 Jun 2013 11:50:33 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r5JFoWEw007053; Wed, 19 Jun 2013 11:50:32 -0400 Message-ID: <51C1D347.3020906@redhat.com> Date: Wed, 19 Jun 2013 15:55:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130514 Thunderbird/17.0.6 MIME-Version: 1.0 To: Andrew Burgess CC: "gdb-patches@sourceware.org" Subject: Re: [PATCH] value_optimized_out and value_fetch_lazy References: <51B5A95F.7090400@broadcom.com> In-Reply-To: <51B5A95F.7090400@broadcom.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-SW-Source: 2013-06/txt/msg00497.txt.bz2 On 06/10/2013 11:24 AM, Andrew Burgess wrote: > Hi, > > I ran into an issue with gdb that appears to be caused by an incorrect > use of value_optimized_out. > > Currently, if value_optimized_out returns true, this means we know the > value is optimized out, if value_optimized_out returns false this > means the value might, or might not, be optimized out; if the value > is currently lazy then we don't know it has been optimized out, and > so the answer from value_optimized_out is false. Looks like the main culprit would be frame_register_unwind. > > I believe that currently, not every caller of value_optimized_out > follows these rules, and some cases if value_optimized_out returns > false, they treat the value as though it is NOT optimized out. > > The patch below changes value_optimized_out so that if the value > is lazy it will be loaded, this should ensure that by the time > value_optimized_out returns, the value will be correct. I've fixed > the one place I could see where we were obviously taking care to > resolve lazy values and I've also made a few tweaks to some of the > other value code in order to prevent recursion while loading lazy > values, and to try and keep values lazy as much as possible. > > Feedback welcome, or ok to commit? I agree with this. It might be possible to factor out the bits of value_fetch_lazy that handle optimized out values, and use it from value_optimized_out, avoiding fetching the value's contents, but given the case I believe where it could most matter, backtracing, in frame_register_unwind, already does: *optimizedp = value_optimized_out (value); *unavailablep = !value_entirely_available (value); and value_entirely_available does already: int value_entirely_available (struct value *value) { /* We can only tell whether the whole value is available when we try to read it. */ if (value->lazy) value_fetch_lazy (value); ... it's super fine with me to make the code correct first, and consider optimizing value_optimized_out's internals at some other point. (while at it, it seems value_entirely_available doesn't really need to call value_fetch_lazy for optimized out values.) I'm finding the patch a bit hard to read though. Could you split it up? At least the move of value_fetch_lazy to value.c would be better done in its own. Pretty hard to reason about or even tell which were the value_fetch_lazy changes from the diff. Can you explain a little better the value_primitive_field changes? Is that code motion mainly an optimization that could be done separately, or is it necessary for correctness? IOW, is that something that could be split out too? Please add copyright headers to the tests. I also noticed the patch adds spurious trailing whitespace: $ git am /tmp/mbox Applying: value_optimized_out and value_fetch_lazy /home/pedro/gdb/mygit/src/.git/rebase-apply/patch:609: trailing whitespace. .uleb128 0x7 /* ULEB128 register */ /home/pedro/gdb/mygit/src/.git/rebase-apply/patch:1032: trailing whitespace. int /home/pedro/gdb/mygit/src/.git/rebase-apply/patch:1036: trailing whitespace. /home/pedro/gdb/mygit/src/.git/rebase-apply/patch:1104: new blank line at EOF. + warning: 4 lines add whitespace errors. -- Pedro Alves