From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 1612 invoked by alias); 5 Jul 2013 15:20:57 -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 1602 invoked by uid 89); 5 Jul 2013 15:20:57 -0000 X-Spam-SWARE-Status: No, score=-4.1 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_DNSWL_MED,RP_MATCHES_RCVD autolearn=ham version=3.3.1 Received: from mms3.broadcom.com (HELO mms3.broadcom.com) (216.31.210.19) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Fri, 05 Jul 2013 15:20:46 +0000 Received: from [10.9.208.55] by mms3.broadcom.com with ESMTP (Broadcom SMTP Relay (Email Firewall v6.5)); Fri, 05 Jul 2013 08:11:11 -0700 X-Server-Uuid: B86B6450-0931-4310-942E-F00ED04CA7AF Received: from IRVEXCHSMTP3.corp.ad.broadcom.com (10.9.207.53) by IRVEXCHCAS07.corp.ad.broadcom.com (10.9.208.55) with Microsoft SMTP Server (TLS) id 14.1.438.0; Fri, 5 Jul 2013 08:20:35 -0700 Received: from mail-irva-13.broadcom.com (10.10.10.20) by IRVEXCHSMTP3.corp.ad.broadcom.com (10.9.207.53) with Microsoft SMTP Server id 14.1.438.0; Fri, 5 Jul 2013 08:20:35 -0700 Received: from [10.177.73.66] (unknown [10.177.73.66]) by mail-irva-13.broadcom.com (Postfix) with ESMTP id 7E695F2D72; Fri, 5 Jul 2013 08:20:34 -0700 (PDT) Message-ID: <51D6E441.4040709@broadcom.com> Date: Fri, 05 Jul 2013 15:20:00 -0000 From: "Andrew Burgess" User-Agent: Mozilla/5.0 (Windows NT 6.1; rv:17.0) Gecko/20130620 Thunderbird/17.0.7 MIME-Version: 1.0 To: "Pedro Alves" cc: gdb-patches@sourceware.org Subject: Re: [COMMIT PATCH] value_bits_valid: Fix latent bug. References: <20130704160927.11801.10290.stgit@brno.lan> <51D6DB44.1000609@broadcom.com> <51D6E0EB.3040006@redhat.com> In-Reply-To: <51D6E0EB.3040006@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2013-07/txt/msg00201.txt.bz2 On 05/07/2013 4:06 PM, Pedro Alves wrote: > On 07/05/2013 03:42 PM, Andrew Burgess wrote: >> On 04/07/2013 5:09 PM, Pedro Alves wrote: >>> Doing something else, I factored out the bits of the value_bits_valid >>> function that actually handle the check_validity hook, and >>> surprisingly found out that the result was misbehaving. Turns out >>> value_bits_valid has a latent bug. If the value is not lval_computed, >>> or doesn't have a check_validity hook, then we should assume the value >>> is entirely valid, not invalid. This is currently masked by the >>> value->optimized_out check -- I ran the testsuite with a gdb_assert(0) >>> inserted in place of that return being touched by the patch, and it >>> never triggers. >>> >>> Tested on x86_64 Fedora 17. >>> >>> gdb/ >>> 2013-07-04 Pedro Alves >>> >>> * value.c (value_bits_valid): If the value is not lval_computed, >>> or doesn't have a check_validity hook, assume the value is entirely >>> valid. >>> --- >>> gdb/value.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/gdb/value.c b/gdb/value.c >>> index ce4b13a..353f62a 100644 >>> --- a/gdb/value.c >>> +++ b/gdb/value.c >>> @@ -1086,7 +1086,7 @@ value_bits_valid (const struct value *value, int offset, int length) >>> return 1; >>> if (value->lval != lval_computed >>> || !value->location.computed.funcs->check_validity) >>> - return 0; >>> + return 1; >>> return value->location.computed.funcs->check_validity (value, offset, >>> length); >>> } >>> >> >> There's a small issue with this patch, in the case of an optimized_out, >> non-computed value we now report that the bits are valid when they >> should be invalid. > > Whoops... > >> Patch below applies on top of the above and fixes both the original >> issue Pedro spotted, and fixes the non-computed issue. >> >> Ok to apply? >> >> Thanks, >> Andrew >> >> >> gdb/ChangeLog >> >> 2013-07-05 Andrew Burgess >> >> * value.c (value_bits_valid): If the value is not lval_computed >> then the answer is in the optimized_out flag, otherwise if we have >> no handler assume bits are valid, if there is a handler use that. >> >> >> >> diff --git a/gdb/value.c b/gdb/value.c >> index 353f62a..ca5463b 100644 >> --- a/gdb/value.c >> +++ b/gdb/value.c >> @@ -1082,13 +1082,18 @@ value_entirely_optimized_out (const struct value *value) >> int >> value_bits_valid (const struct value *value, int offset, int length) >> { >> - if (!value->optimized_out) >> - return 1; >> - if (value->lval != lval_computed >> - || !value->location.computed.funcs->check_validity) >> - return 1; >> - return value->location.computed.funcs->check_validity (value, offset, >> - length); >> + if (value->lval != lval_computed) >> + return !value->optimized_out; >> + else >> + { >> + /* Computed value, defer to handler if there is one. */ >> + if (!value->location.computed.funcs->check_validity) >> + return 1; > > Hmm, in this case we should look at the value->optimized_out > flag too, I think. Looks like my patch was bogus afterall, > and we should just revert it. Very sorry about that. > > The patch I was originally talking about that exposed the > issue was: > > https://github.com/palves/gdb/commit/7143cd119e18d568a5a224ac22f215a96f691624 > > but it looks like the value_check_validity function would have to > check value->optimized_out anyway, so the only difference to > value_bits_valid would be the assertions... You're right, except we could imagine a computed value that implements a fetch method, but not a check-validity method, instead it just sets the optimized_out flag. So, third time lucky, this time, - non computed values, and computed values with no check-validity handler just defer to the optimized_out flag. - computed values with a handler defer to the handler. This is a change from the original code, but I think it's a good change, what do you think? Andrew gdb/ChangeLog 2013-07-05 Andrew Burgess * value.c (value_bits_valid): If the value is not lval_computed or has no check validity handler then the answer is the optimized_out flag, otherwise defer to the handler. diff --git a/gdb/value.c b/gdb/value.c index 353f62a..1be1845 100644 --- a/gdb/value.c +++ b/gdb/value.c @@ -1082,13 +1082,12 @@ value_entirely_optimized_out (const struct value *value) int value_bits_valid (const struct value *value, int offset, int length) { - if (!value->optimized_out) - return 1; if (value->lval != lval_computed || !value->location.computed.funcs->check_validity) - return 1; - return value->location.computed.funcs->check_validity (value, offset, - length); + return !value->optimized_out; + else + return value->location.computed.funcs->check_validity (value, offset, + length); } int