a972d47
http://sourceware.org/ml/gdb-patches/2014-07/msg00530.html
a972d47
Subject: [read_frame_arg patch] Handle partially optimized out values similarly to unavailable values (Re: [patchv2] Fix crash on optimized-out entry data values)
a972d47
a972d47
a972d47
--V88s5gaDVPzZ0KCq
a972d47
Content-Type: text/plain; charset=us-ascii
a972d47
Content-Disposition: inline
a972d47
a972d47
On Thu, 17 Jul 2014 14:23:06 +0200, Pedro Alves wrote:
a972d47
> On 07/16/2014 10:58 PM, Jan Kratochvil wrote:
a972d47
> > This patch is apparently not suitable for gdb-7.8 which is I guess often
a972d47
> > crashing on -O2 -g entry values so there could be some rather minimal crash
a972d47
> > avoiding patch instead.
a972d47
> 
a972d47
> Yeah.
a972d47
> 
a972d47
> So this was originally "caused" (more exposed) by 4f14910f:
a972d47
>     
a972d47
>         gdb/ChangeLog
a972d47
>         2013-11-26  Andrew Burgess  <aburgess@broadcom.com>
a972d47
>     
a972d47
>             * value.c (allocate_optimized_out_value): Mark value as non-lazy.
a972d47
> 
a972d47
> I tried a few approaches in value_available_contents_eq
a972d47
> today, and ended up thinking that the simplest should be to
a972d47
> just revert that patch until we have the fuller fix in place.
a972d47
a972d47
OK, that seems as the best solution for 7.8 to me.
a972d47
a972d47
a972d47
> While doing just that fixes the crash, it surprisingly causes
a972d47
> one of your new tests to FAIL:
a972d47
> 
a972d47
>  (gdb) frame
a972d47
>  #0  bar (ref=ref@entry=@0x7fffffffd184: 10) at gdb.arch/amd64-entry-value-paramref.cc:23
a972d47
>  23        vv++; /* break-here */
a972d47
>  (gdb) FAIL: gdb.arch/amd64-entry-value-paramref.exp: frame
a972d47
a972d47
There is a bug in that entry value code of mine, fix attached.
a972d47
The testcase then PASSes with the reverted optimization by Andrew Burgess.
a972d47
a972d47
For the attached fix - if you nitpick the missing conditional case:
a972d47
	value_optimized_out (val_deref) && value_optimized_out (entryval_deref)
a972d47
It is not detected there but that IMO does not matter much as
a972d47
 * It is for 7.8 only, for trunk it will get compared correctly thanks to the
a972d47
   new implementation of value_available_contents_eq()
a972d47
   called value_contents_eq().
a972d47
 * If the conditional
a972d47
                      if (val != val_deref
a972d47
                          && !value_optimized_out (val_deref)
a972d47
                          && !value_optimized_out (entryval_deref)
a972d47
                          && value_available_contents_eq (val_deref, 0,
a972d47
                                                          entryval_deref, 0,
a972d47
                                                      TYPE_LENGTH (type_deref)))
a972d47
                        val_equal = 1;
a972d47
   fails it may just print
a972d47
     bar (ref=@0x7fffffffd904: <optimized out>, ref@entry=@0x7fffffffd904: <optimized out>)
a972d47
   (or some variant with some partially optimized-out/unavailable parts)
a972d47
   instead of the more correct
a972d47
     bar (ref=ref@entry=@0x7fffffffd904: <optimized out>)
a972d47
   which is not much a bug.
a972d47
a972d47
The attached fix no longe makes sense after the new implementation
a972d47
of value_available_contents_eq() called value_contents_eq() gets applied as it
a972d47
handles all the optimized-out/unavailable values on its own, therefore the
a972d47
attached patch is really only for 7.8.
a972d47
a972d47
a972d47
> Turns out it's the code disabled in value_of_dwarf_reg_entry:
a972d47
> 
a972d47
>   target_val = dwarf_entry_parameter_to_value (parameter,
a972d47
> 					       TYPE_LENGTH (target_type),
a972d47
> 					       target_type, caller_frame,
a972d47
> 					       caller_per_cu);
a972d47
> 
a972d47
>   /* value_as_address dereferences TYPE_CODE_REF.  */
a972d47
>   addr = extract_typed_address (value_contents (outer_val), checked_type);
a972d47
> 
a972d47
>   /* The target entry value has artificial address of the entry value
a972d47
>      reference.  */
a972d47
>   VALUE_LVAL (target_val) = lval_memory;
a972d47
>   set_value_address (target_val, addr);
a972d47
> 
a972d47
> It looks quite wrong to me to just change a value's lval like that.
a972d47
> 
a972d47
> I ran the testsuite with that code disabled (like in the patch below),
a972d47
> and that caused no regressions.  I can't say I really understand the
a972d47
> intention here though.  What would we be missing if we removed that code?
a972d47
a972d47
I cannot reproduce any wrong case having the code above #if 0-ed.
a972d47
a972d47
I just do not find it correct to have it disabled.  But at the same time I do
a972d47
like much / I do not find correct the code myself.  It is a bit problematic to
a972d47
have struct value describing a memory content which is no longer present
a972d47
there.
a972d47
a972d47
What happens there:
a972d47
------------------------------------------------------------------------------
a972d47
volatile int vv;
a972d47
static __attribute__((noinline)) int
a972d47
bar (int &ref) {
a972d47
  ref = 20;
a972d47
  vv++; /* break-here */
a972d47
  return ref;
a972d47
}
a972d47
int main (void) {
a972d47
  int var = 10;
a972d47
  return bar (var);
a972d47
}
a972d47
------------------------------------------------------------------------------
a972d47
 <4><c7>: Abbrev Number: 13 (DW_TAG_GNU_call_site_parameter)
a972d47
    <c8>   DW_AT_location    : 1 byte block: 55         (DW_OP_reg5 (rdi))
a972d47
    <ca>   DW_AT_GNU_call_site_value: 2 byte block: 91 74       (DW_OP_fbreg: -12)
a972d47
    <cd>   DW_AT_GNU_call_site_data_value: 1 byte block: 3a     (DW_OP_lit10)
a972d47
------------------------------------------------------------------------------
a972d47
gdb -ex 'b value_addr' -ex r --args ../gdb ./1 -ex 'watch vv' -ex r -ex 'p &ref@entry'
a972d47
->
a972d47
6    return ref;
a972d47
bar (ref=@0x7fffffffd944: 20, ref@entry=@0x7fffffffd944: 10) at 1.C:25
a972d47
------------------------------------------------------------------------------
a972d47
At /* break-here */ struct value variable 'ref' is TYPE_CODE_REF.
a972d47
a972d47
With FSF GDB HEAD:
a972d47
(gdb) x/gx arg1.contents
a972d47
0x6004000a4ad0: 0x00007fffffffd944
a972d47
(gdb) p ((struct value *)arg1.location.computed.closure).lval
a972d47
$1 = lval_memory
a972d47
(gdb) p/x ((struct value *)arg1.location.computed.closure).location.address
a972d47
$3 = 0x7fffffffd944
a972d47
a972d47
With your #if0-ed code:
a972d47
(gdb) x/gx arg1.contents
a972d47
0x6004000a4ad0: 0x00007fffffffd944
a972d47
(gdb) p ((struct value *)arg1.location.computed.closure).lval
a972d47
$8 = not_lval
a972d47
(gdb) p/x ((struct value *)arg1.location.computed.closure).location.address
a972d47
$9 = 0x0
a972d47
a972d47
I do not see how to access
a972d47
	((struct value *)arg1.location.computed.closure).location.address
a972d47
from GDB CLI.  Trying
a972d47
(gdb) p &ref@entry
a972d47
will invoke value_addr()'s:
a972d47
  if (TYPE_CODE (type) == TYPE_CODE_REF)
a972d47
      /* Copy the value, but change the type from (T&) to (T*).  We
a972d47
         keep the same location information, which is efficient, and
a972d47
         allows &(&X) to get the location containing the reference.  */
a972d47
and therefore the address gets fetched already from
a972d47
  arg1.contents
a972d47
and not from
a972d47
  ((struct value *)arg1.location.computed.closure).location.address
a972d47
.
a972d47
a972d47
And for any other type than TYPE_CODE_REF this code you #if 0-ed does not get
a972d47
executed at all.  This DW_AT_GNU_call_site_data_value DWARF was meant
a972d47
primarily for Fortran but with -O0 entry values do not get produced
a972d47
and with -Og and higher Fortran always optimizes out the passing by reference.
a972d47
a972d47
If you do not like the #if 0 code there I am OK with removing it as I do not
a972d47
know how to make it's use reproducible for user anyway.  In the worst case
a972d47
- if there really is some way how to exploit it - one should just get
a972d47
  Attempt to take address of value not located in memory.
a972d47
instead of some wrong value and it may be easy to fix then.
a972d47
a972d47
a972d47
Thanks for the analysis,
a972d47
Jan
a972d47
a972d47
--V88s5gaDVPzZ0KCq
a972d47
Content-Type: text/plain; charset=us-ascii
a972d47
Content-Disposition: inline; filename=1
a972d47
a972d47
gdb/
a972d47
2014-07-20  Jan Kratochvil  <jan.kratochvil@redhat.com>
a972d47
a972d47
	* stack.c (read_frame_arg): Verify value_optimized_out before calling
a972d47
	value_available_contents_eq.
a972d47
a972d47
diff --git a/gdb/stack.c b/gdb/stack.c
a972d47
index 0d6d8e7..4db5df5 100644
a972d47
--- a/gdb/stack.c
a972d47
+++ b/gdb/stack.c
a972d47
@@ -413,6 +413,8 @@ read_frame_arg (struct symbol *sym, struct frame_info *frame,
a972d47
 		      /* If the reference addresses match but dereferenced
a972d47
 			 content does not match print them.  */
a972d47
 		      if (val != val_deref
a972d47
+		          && !value_optimized_out (val_deref)
a972d47
+		          && !value_optimized_out (entryval_deref)
a972d47
 			  && value_available_contents_eq (val_deref, 0,
a972d47
 							  entryval_deref, 0,
a972d47
 						      TYPE_LENGTH (type_deref)))
a972d47
a972d47
--V88s5gaDVPzZ0KCq--
a972d47