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

Pedro Alves
2013-07-04T17:08:31Z

On 07/04/2013 05:40 PM, Andrew Burgess wrote:
> 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.

Right.

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

Yep. I'm actually about to post a patch in this direction. I'm just
splitting it into smaller unrelated chunks.

--
Pedro Alves