Re: [patch] Fix crash on lval_computed value_zero/value_one

[Pedro Alves <pedro at codesourcery dot com>]

On Sunday 24 July 2011 20:59:27, Jan Kratochvil wrote:
> Hi,
> 
> GDB can crash on computations with lval_computed values dereferencing NULL
> struct lval_funcs vector.
> 
> Sure it could be abstracted more, currently it for example does:
> (gdb) p a
> $1 = (struct S *) <synthetic pointer>
> (gdb) ptype &a[0]
> Attempt to take address of value not located in memory.
> 
> Instead of reassembling new <synthetic pointer>.  But I believe it would
> require struct lval_funcs extensions and this fix is good enough now.
> 
> No regressions on {x86_64,x86_64-m32,i686}-fedora16pre-linux-gnu.
> I will check it in in some time.
> 
> 
> Thanks,
> Jan
> 
> 
> gdb/
> 2011-07-24  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	Fix crash on lval_computed values.
> 	* valops.c (value_zero, value_one): Use not_lval for lval_computed.
> 
> gdb/testsuite/
> 2011-07-24  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	Fix crash on lval_computed values.
> 	* gdb.dwarf2/implptr.exp (print sizeof (j[0])): New test.
> 
> --- a/gdb/valops.c
> +++ b/gdb/valops.c
> @@ -860,7 +860,7 @@ value_zero (struct type *type, enum lval_type lv)
>  {
>    struct value *val = allocate_value (type);
>  
> -  VALUE_LVAL (val) = lv;
> +  VALUE_LVAL (val) = (lv == lval_computed ? not_lval : lv);
>    return val;
>  }

value_zero should really be named "value_that_looks_sufficiently_like_the_value_Id_get_if_I_were_fully_evaluating_the_expression_but_Im_avoiding_touching_the_target_so_I_dont_care_about_the_values_contents_in_end".
It's not meant to produce a real numerical 0.  In that perpective,
that looks good enough until we have lval_computed values
that allow taking the address on (*), I agree.  We can't just _always_
return not_lval, as the value would lose the "legal to take address on me"
property, which I think some bits of evaluation rely on.

(*) - e.g., if we made all values be implemented on top of
the lval_computed mechanism, even lval_memory, lval_register,
etc., ones.

>  
> @@ -911,7 +911,7 @@ value_one (struct type *type, enum lval_type lv)
>        error (_("Not a numeric type."));
>      }
>  
> -  VALUE_LVAL (val) = lv;
> +  VALUE_LVAL (val) = (lv == lval_computed ? not_lval : lv);
>    return val;
>  }

This one's plain odd though.  Contrary to value_zero,
value_one's intent is really to build a numerical "1".  It makes
no sense for a "1" to be anything other than not an lval, as
you can't assign to "1".  In fact, all callers of value_one pass
in not_lval...  We should just remove the `lv' parameter and
replace that VALUE_LVAL assignment by:

  gdb_assert (VALUE_LVAL (val) == not_lval);

(allocate_value creates values already as not_lval)

(I'd prefer that cleanup to be a separate patch.)

-- 
Pedro Alves