From f6c2e006b1b4d3f082d8ee137c54af214d669151 Mon Sep 17 00:00:00 2001
From: Alex Crichton <alex@alexcrichton.com>
Date: Wed, 15 May 2019 07:25:56 -0700
Subject: [PATCH] Always call backtrace_syminfo with libbacktrace

Turns out if we favor backtrace_pcinfo then if there's no debuginfo
in the binary (e.g. `strip -g`) then symbol information comes out! It
also looks to have more accurate symbol names in terms of loading it
from the symbol table rather than debuginfo where debuginfo tends to
typically be a bit more terse in naming.
---
 src/symbolize/libbacktrace.rs | 108 +++++++++++++++++++++++-----------
 1 file changed, 74 insertions(+), 34 deletions(-)

diff --git a/src/symbolize/libbacktrace.rs b/src/symbolize/libbacktrace.rs
index fe7b86e3d..d1b95b6d9 100644
--- a/src/symbolize/libbacktrace.rs
+++ b/src/symbolize/libbacktrace.rs
@@ -114,6 +114,12 @@ extern "C" fn error_cb(_data: *mut c_void, _msg: *const c_char, _errnum: c_int)
     // do nothing for now
 }
 
+/// Type of the `data` pointer passed into `syminfo_cb`
+struct SyminfoState<'a> {
+    cb: &'a mut (FnMut(&super::Symbol) + 'a),
+    pc: usize,
+}
+
 extern "C" fn syminfo_cb(
     data: *mut c_void,
     pc: uintptr_t,
@@ -121,19 +127,47 @@ extern "C" fn syminfo_cb(
     _symval: uintptr_t,
     _symsize: uintptr_t,
 ) {
+    // Once this callback is invoked from `backtrace_syminfo` when we start
+    // resolving we go further to call `backtrace_pcinfo`. The
+    // `backtrace_pcinfo` function will consult debug information and attemp tto
+    // do things like recover file/line information as well as inlined frames.
+    // Note though that `backtrace_pcinfo` can fail or not do much if there's
+    // not debug info, so if that happens we're sure to call the callback with
+    // at least one symbol from the `syminfo_cb`.
     unsafe {
-        call(
-            data,
-            &super::Symbol {
+        let syminfo_state = &mut *(data as *mut SyminfoState);
+        let mut pcinfo_state = PcinfoState {
+            symname,
+            called: false,
+            cb: syminfo_state.cb,
+        };
+        bt::backtrace_pcinfo(
+            init_state(),
+            syminfo_state.pc as uintptr_t,
+            pcinfo_cb,
+            error_cb,
+            &mut pcinfo_state as *mut _ as *mut _,
+        );
+        if !pcinfo_state.called {
+            let mut bomb = ::Bomb { enabled: true };
+            (pcinfo_state.cb)(&super::Symbol {
                 inner: Symbol::Syminfo {
                     pc: pc,
                     symname: symname,
                 },
-            },
-        );
+            });
+            bomb.enabled = false;
+        }
     }
 }
 
+/// Type of the `data` pointer passed into `pcinfo_cb`
+struct PcinfoState<'a> {
+    cb: &'a mut (FnMut(&super::Symbol) + 'a),
+    symname: *const c_char,
+    called: bool,
+}
+
 extern "C" fn pcinfo_cb(
     data: *mut c_void,
     pc: uintptr_t,
@@ -145,28 +179,33 @@ extern "C" fn pcinfo_cb(
         if filename.is_null() || function.is_null() {
             return -1;
         }
-        call(
-            data,
-            &super::Symbol {
-                inner: Symbol::Pcinfo {
-                    pc: pc,
-                    filename: filename,
-                    lineno: lineno,
-                    function: function,
+        let state = &mut *(data as *mut PcinfoState);
+        let mut bomb = ::Bomb { enabled: true };
+        state.called = true;
+        (state.cb)(&super::Symbol {
+            inner: Symbol::Pcinfo {
+                pc: pc,
+                filename: filename,
+                lineno: lineno,
+
+                // Favor the function name going into the `syminfo_cb`. That's
+                // typically from the symbol table and is the full symbol name
+                // whereas the `function` above is coming from debuginfo which
+                // isn't always as descriptive when considering the whole
+                // program.
+                function: if !state.symname.is_null() {
+                    state.symname
+                } else {
+                    function
                 },
             },
-        );
+        });
+        bomb.enabled = false;
+
         return 0;
     }
 }
 
-unsafe fn call(data: *mut c_void, sym: &super::Symbol) {
-    let cb = data as *mut &mut FnMut(&super::Symbol);
-    let mut bomb = ::Bomb { enabled: true };
-    (*cb)(sym);
-    bomb.enabled = false;
-}
-
 // The libbacktrace API supports creating a state, but it does not
 // support destroying a state. I personally take this to mean that a
 // state is meant to be created and then live forever.
@@ -334,7 +373,7 @@ unsafe fn init_state() -> *mut bt::backtrace_state {
     }
 }
 
-pub unsafe fn resolve(what: ResolveWhat, mut cb: &mut FnMut(&super::Symbol)) {
+pub unsafe fn resolve(what: ResolveWhat, cb: &mut FnMut(&super::Symbol)) {
     let symaddr = what.address_or_ip();
 
     // backtrace errors are currently swept under the rug
@@ -343,20 +382,21 @@ pub unsafe fn resolve(what: ResolveWhat, mut cb: &mut FnMut(&super::Symbol)) {
         return;
     }
 
-    let ret = bt::backtrace_pcinfo(
+    // Call the `backtrace_syminfo` API first. This is (from reading the code)
+    // guaranteed to call `syminfo_cb` exactly once (or fail with an error
+    // presumably). We then handle more within the `syminfo_cb`.
+    //
+    // Note that we do this since `syminfo` will consult the symbol table,
+    // finding symbol names even if there's no debug information in the binary.
+    let mut syminfo_state = SyminfoState {
+        pc: symaddr as usize,
+        cb: cb,
+    };
+    bt::backtrace_syminfo(
         state,
         symaddr as uintptr_t,
-        pcinfo_cb,
+        syminfo_cb,
         error_cb,
-        &mut cb as *mut _ as *mut _,
+        &mut syminfo_state as *mut _ as *mut _,
     );
-    if ret != 0 {
-        bt::backtrace_syminfo(
-            state,
-            symaddr as uintptr_t,
-            syminfo_cb,
-            error_cb,
-            &mut cb as *mut _ as *mut _,
-        );
-    }
 }