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

[Pedro Alves <palves@redhat.com>]

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...

-- 
Pedro Alves