From ac45e727be2fd1d025824fb11ffc71bec25d9dd1 Mon Sep 17 00:00:00 2001
From: Alex Crichton <alex@alexcrichton.com>
Date: Thu, 16 May 2019 14:41:25 -0700
Subject: [PATCH] Favor debuginfo function names instead of symbol table

This commit fixes an accidental regression with #185 where libbacktrace
was updated to match the standard library by always returning the symbol
table symbol. Turns out though this library was one-upping the standard
library by consulting debuginfo (what it was previously doing) because
debuginfo can have more accurate symbol information for inline
functions, for example.

The fix here is to always prefer the debuginfo function name, if
present, and otherwise fall back to the symbol table symbol if
necessary.

Closes #186
---
 src/symbolize/libbacktrace.rs | 54 ++++++++++++++++++++---------------
 1 file changed, 31 insertions(+), 23 deletions(-)

diff --git a/src/symbolize/libbacktrace.rs b/src/symbolize/libbacktrace.rs
index d1b95b6d9..691fe3138 100644
--- a/src/symbolize/libbacktrace.rs
+++ b/src/symbolize/libbacktrace.rs
@@ -31,24 +31,41 @@ pub enum Symbol {
         filename: *const c_char,
         lineno: c_int,
         function: *const c_char,
+        symname: *const c_char,
     },
 }
 
 impl Symbol {
     pub fn name(&self) -> Option<SymbolName> {
-        let ptr = match *self {
-            Symbol::Syminfo { symname, .. } => symname,
-            Symbol::Pcinfo { function, .. } => function,
-        };
-        if ptr.is_null() {
-            None
-        } else {
+        let symbol = |ptr: *const c_char| {
             unsafe {
-                let len = libc::strlen(ptr);
-                Some(SymbolName::new(slice::from_raw_parts(
-                    ptr as *const u8,
-                    len,
-                )))
+                if ptr.is_null() {
+                    None
+                } else {
+                    let len = libc::strlen(ptr);
+                    Some(SymbolName::new(slice::from_raw_parts(
+                        ptr as *const u8,
+                        len,
+                    )))
+                }
+            }
+        };
+        match *self {
+            Symbol::Syminfo { symname, .. } => symbol(symname),
+            Symbol::Pcinfo { function, symname, .. } => {
+                // If possible prefer the `function` name which comes from
+                // debuginfo and can typically be more accurate for inline
+                // frames for example. If that's not present though fall back to
+                // the symbol table name specified in `symname`.
+                //
+                // Note that sometimes `function` can feel somewhat less
+                // accurate, for example being listed as `try<i32,closure>`
+                // isntead of `std::panicking::try::do_call`. It's not really
+                // clear why, but overall the `function` name seems more accurate.
+                if let Some(sym) = symbol(function) {
+                    return Some(sym)
+                }
+                symbol(symname)
             }
         }
     }
@@ -187,17 +204,8 @@ extern "C" fn pcinfo_cb(
                 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
-                },
+                symname: state.symname,
+                function,
             },
         });
         bomb.enabled = false;