This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [COMMIT PATCH] value_bits_valid: Fix latent bug.
- From: "Andrew Burgess" <aburgess at broadcom dot com>
- To: "Pedro Alves" <palves at redhat dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Fri, 5 Jul 2013 16:20:33 +0100
- Subject: Re: [COMMIT PATCH] value_bits_valid: Fix latent bug.
- References: <20130704160927 dot 11801 dot 10290 dot stgit at brno dot lan> <51D6DB44 dot 1000609 at broadcom dot com> <51D6E0EB dot 3040006 at redhat dot com>
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 <palves@redhat.com>
>>>
>>> * 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 <aburgess@broadcom.com>
>>
>> * 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 <aburgess@broadcom.com>
* 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