Re: [read_frame_arg patch] Handle partially optimized out values similarly to unavailable values (Re: [patchv2] Fix crash on optimized-out entry data values)

[Pedro Alves <palves@redhat.com>]

On 07/20/2014 04:07 PM, Jan Kratochvil wrote:
> On Thu, 17 Jul 2014 14:23:06 +0200, Pedro Alves wrote:
>> On 07/16/2014 10:58 PM, Jan Kratochvil wrote:
>>> This patch is apparently not suitable for gdb-7.8 which is I guess often
>>> crashing on -O2 -g entry values so there could be some rather minimal crash
>>> avoiding patch instead.
>>
>> Yeah.
>>
>> So this was originally "caused" (more exposed) by 4f14910f:
>>     
>>         gdb/ChangeLog
>>         2013-11-26  Andrew Burgess  <aburgess@broadcom.com>
>>     
>>             * value.c (allocate_optimized_out_value): Mark value as non-lazy.
>>
>> I tried a few approaches in value_available_contents_eq
>> today, and ended up thinking that the simplest should be to
>> just revert that patch until we have the fuller fix in place.
> 
> OK, that seems as the best solution for 7.8 to me.
> 
> 
>> While doing just that fixes the crash, it surprisingly causes
>> one of your new tests to FAIL:
>>
>>  (gdb) frame
>>  #0  bar (ref=ref@entry=@0x7fffffffd184: 10) at gdb.arch/amd64-entry-value-paramref.cc:23
>>  23        vv++; /* break-here */
>>  (gdb) FAIL: gdb.arch/amd64-entry-value-paramref.exp: frame
> 
> There is a bug in that entry value code of mine, fix attached.
> The testcase then PASSes with the reverted optimization by Andrew Burgess.
> 

OK, I've pushed the reversion (without the #if 0 bit) to both master
and 7.8.

> For the attached fix - if you nitpick the missing conditional case:
> 	value_optimized_out (val_deref) && value_optimized_out (entryval_deref)
> It is not detected there but that IMO does not matter much as
>  * It is for 7.8 only, for trunk it will get compared correctly thanks to the
>    new implementation of value_available_contents_eq()
>    called value_contents_eq().
>  * If the conditional
>                       if (val != val_deref
>                           && !value_optimized_out (val_deref)
>                           && !value_optimized_out (entryval_deref)
>                           && value_available_contents_eq (val_deref, 0,
>                                                           entryval_deref, 0,
>                                                       TYPE_LENGTH (type_deref)))
>                         val_equal = 1;
>    fails it may just print
>      bar (ref=@0x7fffffffd904: <optimized out>, ref@entry=@0x7fffffffd904: <optimized out>)
>    (or some variant with some partially optimized-out/unavailable parts)
>    instead of the more correct
>      bar (ref=ref@entry=@0x7fffffffd904: <optimized out>)
>    which is not much a bug.

That's fine with me.

> The attached fix no longe makes sense after the new implementation
> of value_available_contents_eq() called value_contents_eq() gets applied as it
> handles all the optimized-out/unavailable values on its own, therefore the
> attached patch is really only for 7.8.

As it's best not to get ourselves in a situation where we have a fix in
the branch but not in mainline, and avoid putting pressure on the
better fix, it's better to put your patch in mainline too.

> I do not see how to access
> 	((struct value *)arg1.location.computed.closure).location.address
> from GDB CLI.  Trying
> (gdb) p &ref@entry
> will invoke value_addr()'s:
>   if (TYPE_CODE (type) == TYPE_CODE_REF)
>       /* Copy the value, but change the type from (T&) to (T*).  We
>          keep the same location information, which is efficient, and
>          allows &(&X) to get the location containing the reference.  */
> and therefore the address gets fetched already from
>   arg1.contents
> and not from
>   ((struct value *)arg1.location.computed.closure).location.address
> .
> 

Yeah.

> And for any other type than TYPE_CODE_REF this code you #if 0-ed does not get
> executed at all.  This DW_AT_GNU_call_site_data_value DWARF was meant
> primarily for Fortran but with -O0 entry values do not get produced
> and with -Og and higher Fortran always optimizes out the passing by reference.
> 
> If you do not like the #if 0 code there I am OK with removing it as I do not
> know how to make it's use reproducible for user anyway.  In the worst case
> - if there really is some way how to exploit it - one should just get
>   Attempt to take address of value not located in memory.
> instead of some wrong value and it may be easy to fix then.

Thanks Jan.  Indeed I'd much prefer removing it.
It's fine with me to still leave it in 7.8 in case we missed
something.

-- 
Pedro Alves