From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 22275 invoked by alias); 5 Jul 2013 15:06:25 -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 22266 invoked by uid 89); 5 Jul 2013 15:06:25 -0000 X-Spam-SWARE-Status: No, score=-7.5 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; Fri, 05 Jul 2013 15:06:23 +0000 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r65F6Lof016905 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Fri, 5 Jul 2013 11:06:21 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id r65F6Jfb020163; Fri, 5 Jul 2013 11:06:20 -0400 Message-ID: <51D6E0EB.3040006@redhat.com> Date: Fri, 05 Jul 2013 15:06: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: Andrew Burgess 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> In-Reply-To: <51D6DB44.1000609@broadcom.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2013-07/txt/msg00200.txt.bz2 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... -- Pedro Alves