Re: [COMMIT PATCH] value_bits_valid: Fix latent bug.

["Andrew Burgess" <aburgess@broadcom.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