Re: [patch v4 18/24] record-btrace: extend unwinder

[Jan Kratochvil <jan.kratochvil@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4232 bytes --]

On Mon, 16 Sep 2013 13:21:29 +0200, Metzger, Markus T wrote:
> > > An assertion in get_frame_id at frame.c:340 requires that a frame
> > > provides a stack address.  The record-btrace unwinder can't provide
> > > this since the trace does not contain data.  I incorrectly set
> > > stack_addr_p to 1 to avoid the assertion.
> > 
> > Primarily record-btrace can provide the stack address.  You know $sp at the
> > end of the recoding and you can query .eh_frame/.debug_frame at any PC
> > address what is the difference between $sp and caller's $sp at that exact PC.
> > This assumes either all the involved binaries were built with -fasynchronous-
> > unwind-tables (for .eh_frame) or that debug info (for .debug_frame) is
> > present.  The former is true in Fedora / Red Hat distros, unaware how others.
> 
> This would only hold for functions that have not yet returned to their caller.
> If we go back far enough, the branch trace will also contain functions that
> have already returned to their caller for which we do not have any information.
> I would even argue that this is the majority of functions in the branch trace.

In many cases one can reconstruct $sp.  But for example if alloca() was in use
I see now $sp cannot be reconstructed.  So I agree now GDB has to handle cases
when $sp is not known for a frame_id.

BTW in many cases one realy can reconstruct all past $sp addresses from the
btrace buffer.  I have tried it for one backtrace of /usr/bin/gdb :

It will not work (at least) in two cases:

 * for -O0 code (not -O2) GCC does not produce DW_CFA_def_cfa_offset but it
   provides just:
     DW_CFA_def_cfa_register: r6 (rbp)
   As you describe we do not know $rbp that time anymore.

 * Even in -O2 code if a function uses alloca() GCC will produce again:
     DW_CFA_def_cfa_register: r6 (rbp)

I have no idea in which percentage of real world code the
DW_CFA_def_cfa_offset dependency would work, C++ code CFI may look differently
than GDB in plain C I tried below:

CIE has always:
  DW_CFA_def_cfa: r7 (rsp) ofs 8

#0  0x00007ffff5eef950 in __poll_nocancel
  DW_CFA_def_cfa: r7 (rsp) ofs 8
#1  0x000000000059bb63 in poll
  DW_CFA_def_cfa_offset: 80
#2  gdb_wait_for_event
#3  0x000000000059c2da in gdb_do_one_event
  DW_CFA_def_cfa_offset: 64
#4  0x000000000059c517 in start_event_loop
  DW_CFA_def_cfa_offset: 48
#5  0x00000000005953a3 in captured_command_loop
  DW_CFA_def_cfa_offset: 16
#6  0x00000000005934aa in catch_errors
  DW_CFA_def_cfa_offset: 112
#7  0x000000000059607e in captured_main
  DW_CFA_def_cfa_offset: 192
#8  0x00000000005934aa in catch_errors
  DW_CFA_def_cfa_offset: 112
#9  0x0000000000596c44 in gdb_main
  DW_CFA_def_cfa_offset: 16
#10 0x000000000045526e in main
  DW_CFA_def_cfa_offset: 64

As an obvious check $sp in #10 main 0x7fffffffd940 - $sp in #0 0x7fffffffd6b8:
(gdb) p 0x7fffffffd940 - 0x7fffffffd6b8
$1 = 648
8+80+64+48+16+112+192+112+16 = 648

This is just FYI, I do not ask to implement it.  I do not think knowing just
$sp is too important when it works only sometimes.


> > The current method of constant STACK_ADDR may have some problems with
> > frame_id_inner() but I did not investigate it more.
> 
> By looking at the code, frame_id_inner () should always fail since all btrace
> frames have stack_addr == 0.
> 
> On the other hand, frame_id_inner is only called for frames of type
> NORMAL_FRAME, whereas btrace frames have type BTRACE_FRAME.

OK, I agree now frame_id_inner() is not needed.


> This has meanwhile been resolved.  This had been a side-effect of throwing
> an error in to_fetch_registers.  When I just return, function arguments are
> correctly displayed as unavailable and the "can't compute CFA for this frame"
> message is gone.

With v6 patchset it is only sometimes gone, I still get it.
Tested with (results are the same):
	gcc (GCC) 4.8.2 20130927 (prerelease)
	gcc-4.8.1-10.fc21.x86_64

int f(int i) {
  return i;
}
int main(void) {
  f(1);
  return 0;
}

gcc -o test3 test3.c -Wall -g 
./gdb ./test3 -ex start -ex 'record btrace' -ex step -ex step -ex reverse-step -ex frame
#0  f (i=<error reading variable: can't compute CFA for this frame>) at test3.c:2
2	  return i;
(gdb) _

It gets fixed by the attached patch.


Thanks,
Jan

[-- Attachment #2: cfa.patch --]
[-- Type: text/plain, Size: 1824 bytes --]

diff --git a/gdb/dwarf2-frame.c b/gdb/dwarf2-frame.c
index 2aff23e..518b0b9 100644
--- a/gdb/dwarf2-frame.c
+++ b/gdb/dwarf2-frame.c
@@ -1495,9 +1495,13 @@ dwarf2_frame_base_sniffer (struct frame_info *this_frame)
 CORE_ADDR
 dwarf2_frame_cfa (struct frame_info *this_frame)
 {
+  extern const struct frame_unwind record_btrace_frame_unwind;
+  extern const struct frame_unwind record_btrace_tailcall_frame_unwind;
   while (get_frame_type (this_frame) == INLINE_FRAME)
     this_frame = get_prev_frame (this_frame);
-  if (get_frame_unwind_stop_reason (this_frame) == UNWIND_UNAVAILABLE)
+  if (get_frame_unwind_stop_reason (this_frame) == UNWIND_UNAVAILABLE
+      || frame_unwinder_is (this_frame, &record_btrace_frame_unwind)
+      || frame_unwinder_is (this_frame, &record_btrace_tailcall_frame_unwind))
     throw_error (NOT_AVAILABLE_ERROR,
                 _("can't compute CFA for this frame: "
                   "required registers or memory are unavailable"));
diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
index d634712..9a4287b 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -1217,7 +1217,7 @@ record_btrace_frame_dealloc_cache (struct frame_info *self, void *this_cache)
    Therefore this unwinder reports any possibly unwound registers as
    <unavailable>.  */
 
-static const struct frame_unwind record_btrace_frame_unwind =
+const struct frame_unwind record_btrace_frame_unwind =
 {
   BTRACE_FRAME,
   record_btrace_frame_unwind_stop_reason,
@@ -1228,7 +1228,7 @@ static const struct frame_unwind record_btrace_frame_unwind =
   record_btrace_frame_dealloc_cache
 };
 
-static const struct frame_unwind record_btrace_tailcall_frame_unwind =
+const struct frame_unwind record_btrace_tailcall_frame_unwind =
 {
   BTRACE_TAILCALL_FRAME,
   record_btrace_frame_unwind_stop_reason,