Re: [3/3] [PATCH] value_optimized_out and value_fetch_lazy

["Andrew Burgess" <aburgess@broadcom.com>]

On 03/07/2013 8:57 PM, Pedro Alves wrote:
> On 07/01/2013 07:06 PM, Andrew Burgess wrote:
>> On 19/06/2013 4:50 PM, Pedro Alves wrote:
>>> On 06/10/2013 11:24 AM, Andrew Burgess wrote:
>>>
>>>> I ran into an issue with gdb that appears to be caused by an incorrect
>>>> use of value_optimized_out.
>>>>
>>>
>>> I'm finding the patch a bit hard to read though.  Could you
>>> split it up?  
>>
>> This tiny patch notices that when we mark a value as optimized out
>> we can also mark the value as no longer lazy.
>> This patch is not required, but felt like a good thing to me, not
>> sure if everyone will agree though.
>>
>> Should I apply?
>>
>> Thanks
>> Andrew
>>
>>
>>
>>
>> gdb/ChangeLog
>>
>> 2013-07-01  Andrew Burgess  <aburgess@broadcom.com>
>>
>> 	* value.c (set_value_optimized_out): A value that is optimized out
>> 	is no longer lazy.
> 
> Hmm.
> 
> I tried going a step further.  If the value is optimized
> out, then we don't really need its contents buffer.
> 
> That is:
> 
>  gdb/value.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/gdb/value.c b/gdb/value.c
> index 970fb66..50f8889 100644
> --- a/gdb/value.c
> +++ b/gdb/value.c
> @@ -690,6 +690,8 @@ allocate_value_lazy (struct type *type)
>  void
>  allocate_value_contents (struct value *val)
>  {
> +  gdb_assert (!val->optimized_out);
> +
>    if (!val->contents)
>      val->contents = (gdb_byte *) xzalloc (TYPE_LENGTH (val->enclosing_type));
>  }
> @@ -1064,6 +1066,13 @@ void
>  set_value_optimized_out (struct value *value, int val)
>  {
>    value->optimized_out = val;
> +
> +  if (val)
> +    {
> +      set_value_lazy (value, 0);
> +      xfree (value->contents);
> +      value->contents = NULL;
> +    }
>  }
> 
> and then ran the testsuite.  Sure enough, that catches a gdb crash.
> In gdb.dwarf2/dw2-op-out-param.exp:
> 
> #0  0x000000000056bed0 in extract_signed_integer (addr=0x0, len=8, byte_order=BFD_ENDIAN_LITTLE) at ../../src/gdb/findvar.c:78
> #1  0x000000000057e761 in unpack_long (type=0x1793f80, valaddr=0x0) at ../../src/gdb/value.c:2453
> #2  0x000000000059baae in print_scalar_formatted (valaddr=0x0, type=0x1793f80, options=0x7fffdb2376c0, size=0, stream=0x1929a00) at ../../src/gdb/printcmd.c:404
> #3  0x000000000059763b in val_print_scalar_formatted (type=0x1793f80, valaddr=0x0, embedded_offset=0, val=0x19dfcd0, options=0x7fffdb2376c0, size=0, stream=0x1929a00)
>     at ../../src/gdb/valprint.c:970
> #4  0x00000000006d1f2f in c_val_print (type=0x1793f80, valaddr=0x0, embedded_offset=0, address=9225056, stream=0x1929a00, recurse=3, original_value=0x19dfcd0, options=
>     0x7fffdb237860) at ../../src/gdb/c-valprint.c:407
> #5  0x0000000000596f4a in val_print (type=0x1793f80, valaddr=0x0, embedded_offset=0, address=9225056, stream=0x1929a00, recurse=3, val=0x19dfcd0, options=0x7fffdb237940,
>     language=0x8d34a0) at ../../src/gdb/valprint.c:778
> #6  0x00000000006d3491 in cp_print_value_fields (type=0x1796110, real_type=0x1796110, valaddr=0x0, offset=0, address=9225056, stream=0x1929a00, recurse=2, val=0x19dfcd0,
>     options=0x7fffdb237d40, dont_print_vb=0x0, dont_print_statmem=0) at ../../src/gdb/cp-valprint.c:363
> #7  0x00000000006d37ef in cp_print_value_fields_rtti (type=0x1796110, valaddr=0x0, offset=0, address=9225056, stream=0x1929a00, recurse=2, val=0x19dfcd0, options=
>     0x7fffdb237d40, dont_print_vb=0x0, dont_print_statmem=0) at ../../src/gdb/cp-valprint.c:458
> #8  0x00000000006d1e45 in c_val_print (type=0x1796110, valaddr=0x0, embedded_offset=0, address=9225056, stream=0x1929a00, recurse=2, original_value=0x19dfcd0, options=
>     0x7fffdb237d40) at ../../src/gdb/c-valprint.c:393
> #9  0x0000000000596f4a in val_print (type=0x1796110, valaddr=0x0, embedded_offset=0, address=9225056, stream=0x1929a00, recurse=2, val=0x19dfcd0, options=0x7fffdb237e60,
>     language=0x8d34a0) at ../../src/gdb/valprint.c:778
> #10 0x0000000000597157 in common_val_print (val=0x19dfcd0, stream=0x1929a00, recurse=2, options=0x7fffdb237e60, language=0x8d34a0) at ../../src/gdb/valprint.c:840
> #11 0x00000000005d9ea6 in print_frame_arg (arg=0x7fffdb237f30) at ../../src/gdb/stack.c:284
> #12 0x00000000005daa49 in print_frame_args (func=0x1796af0, frame=0x1b5b800, num=-1, stream=0x168fd10) at ../../src/gdb/stack.c:645
> #13 0x00000000005db9df in print_frame (frame=0x1b5b800, print_level=1, print_what=LOCATION, print_args=1, sal=...) at ../../src/gdb/stack.c:1172
> #14 0x00000000005daf95 in print_frame_info (frame=0x1b5b800, print_level=1, print_what=LOCATION, print_args=1) at ../../src/gdb/stack.c:824
> #15 0x00000000005dcdc5 in backtrace_command_1 (count_exp=0x0, show_locals=0, no_filters=0, from_tty=1) at ../../src/gdb/stack.c:1763
> #16 0x00000000005dd1b6 in backtrace_command (arg=0x0, from_tty=1) at ../../src/gdb/stack.c:1860
> #17 0x00000000004dc53f in do_cfunc (c=0x151bec0, args=0x0, from_tty=1) at ../../src/gdb/cli/cli-decode.c:113
> #18 0x00000000004df5d4 in cmd_func (cmd=0x151bec0, args=0x0, from_tty=1) at ../../src/gdb/cli/cli-decode.c:1888
> #19 0x00000000006e43d1 in execute_command (p=0x14623a2 "", from_tty=1) at ../../src/gdb/top.c:478
> 
> Odd that this bit of val_print_scalar_formatted:
> 
>   /* A scalar object that does not have all bits available can't be
>      printed, because all bits contribute to its representation.  */
>   if (!value_bits_valid (val, TARGET_CHAR_BIT * embedded_offset,
> 			      TARGET_CHAR_BIT * TYPE_LENGTH (type)))
>     val_print_optimized_out (stream);
> 
> didn't catch this.  I then added this further.
> 
> --- c/gdb/value.c
> +++ w/gdb/value.c
> @@ -1078,6 +1078,8 @@ set_value_optimized_out (struct value *value, int val)
>  int
>  value_entirely_optimized_out (const struct value *value)
>  {
> +  gdb_assert (value->optimized_out || !value->lazy);
> +
>    if (!value->optimized_out)
>      return 0;
>    if (value->lval != lval_computed
> @@ -1089,6 +1091,8 @@ value_entirely_optimized_out (const struct value *value)
>  int
>  value_bits_valid (const struct value *value, int offset, int length)
>  {
> +  gdb_assert (value->optimized_out || !value->lazy);
> +
>    if (!value->optimized_out)
>      return 1;
>    if (value->lval != lval_computed
> 
> And I see:
> 
> Breakpoint 2, 0x000000000040058f in breakpt ()
> (gdb) PASS: gdb.dwarf2/dw2-op-out-param.exp: continue to breakpoint: Stop in breakpt for struct_param_two_reg_pieces
> bt
> #0  0x000000000040058f in breakpt ()
> #1  0x00000000004005c2 in struct_param_two_reg_pieces (operand0=../../src/gdb/value.c:1081: internal-error: value_entirely_optimized_out: Assertion `value->optimized_out || !value->lazy' failed.
> A problem internal to GDB has been detected,
> further debugging may prove unreliable.
> Quit this debugging session? (y or n) FAIL: gdb.dwarf2/dw2-op-out-param.exp: Backtrace for test struct_param_two_reg_pieces (GDB internal error)
> Resyncing due to internal error.
> 
> ... and then I ran out of time.  Gotta run an errand.  :-)
> 

The issue here is that optimized_out does not mean "the value is
optimized out", but instead (in the general case) means this value is at
least partially optimized out.

The problem case are computed values, where if part of the value is
missing then the optimized_out flag is set, but we still rely on being
able to access the contents of the value, with the result that your
patch above, freeing the contents, causes problems.

I don't include the patch here, but as an experiment I made a quick hack
where all the calls to set_value_optimized_out inside dwarf2loc.c call a
new method set_value_partially_optimized_out, this new method sets the
optimized_out flag but does not free the contents buffer, all the tests
now pass.  I know this is a hack, we create computed values in other
situations, but the case of computed value and partially optimized out
values only appear to be tested for the dwarf case.

So... I wonder, there appears to be some overlap between the value
unavailable data and the value optimized_out flag.  I believe I
understand the difference:
 * unavailable - data was not collected at a tracepoint.
 * optimized_out - insufficient debug information, or the debug info
specifically states the value is gone.

However, at a higher level there does seem to be some overlap.  Imagine
if we collapsed together the unavailable and optimized_out properties of
a value, when handling a value gdb would then walk the following path:

  is-value-available? ----- yes -----> process-as-normal
    |
    '-- no --- > why-not? --- optimized_out ---> opt-out-case
                    |
                    '-------- unavailable ---> unavailable-case

The benefit however would be all the code we currently have for
propagating unavailable / optimized_out status between values would be
unified, we would gain the ability to support partially optimized_out
values, and from a quick look at the code I believe there are a couple
of places where we handle optimized_out but not unavailable, this would
be fixed.

What do people think?

Cheers,
Andrew