Cleanup dbghelp so that stdlib can print backtraces on panic again

Fixes rust-lang#165

Cargo.toml

@@ -71,7 +71,7 @@ std = []
 #   function to acquire a backtrace
 libunwind = []
 unix-backtrace = []
-dbghelp = []
+dbghelp = ["std"]
 kernel32 = []
 
 #=======================================

benches/benchmarks.rs

@@ -0,0 +1,96 @@
+#![feature(test)]
+
+extern crate test;
+
+extern crate backtrace;
+
+#[cfg(feature = "std")]
+use backtrace::Backtrace;
+
+#[bench]
+#[cfg(feature = "std")]
+fn trace(b: &mut test::Bencher) {
+    #[inline(never)]
+    fn the_function() {
+        backtrace::trace(|frame| {
+            let ip = frame.ip();
+            test::black_box(ip);
+            true
+        });
+    }
+    b.iter(the_function);
+}
+
+#[bench]
+#[cfg(feature = "std")]
+fn trace_and_resolve_callback(b: &mut test::Bencher) {
+    #[inline(never)]
+    fn the_function() {
+        backtrace::trace(|frame| {
+            backtrace::resolve(frame.ip(), |symbol| {
+                let addr = symbol.addr();
+                test::black_box(addr);
+            });
+            true
+        });
+    }
+    b.iter(the_function);
+}
+
+
+
+#[bench]
+#[cfg(feature = "std")]
+fn trace_and_resolve_separate(b: &mut test::Bencher) {
+    #[inline(never)]
+    fn the_function(frames: &mut Vec<*mut std::ffi::c_void>) {
+        backtrace::trace(|frame| {
+            frames.push(frame.ip());
+            true
+        });
+        frames.iter().for_each(|frame_ip| {
+            backtrace::resolve(*frame_ip, |symbol| {
+                test::black_box(symbol);
+            });
+        });
+    }
+    let mut frames = Vec::with_capacity(1024);
+    b.iter(|| {
+        the_function(&mut frames);
+        frames.clear();
+    });
+}
+
+#[bench]
+#[cfg(feature = "std")]
+fn new_unresolved(b: &mut test::Bencher) {
+    #[inline(never)]
+    fn the_function() {
+        let bt = Backtrace::new_unresolved();
+        test::black_box(bt);
+    }
+    b.iter(the_function);
+}
+
+#[bench]
+#[cfg(feature = "std")]
+fn new(b: &mut test::Bencher) {
+    #[inline(never)]
+    fn the_function() {
+        let bt = Backtrace::new();
+        test::black_box(bt);
+    }
+    b.iter(the_function);
+}
+
+#[bench]
+#[cfg(feature = "std")]
+fn new_unresolved_and_resolve_separate(b: &mut test::Bencher) {
+    #[inline(never)]
+    fn the_function() {
+        let mut bt = Backtrace::new_unresolved();
+        bt.resolve();
+        test::black_box(bt);
+    }
+    b.iter(the_function);
+}

src/capture.rs

@@ -68,6 +68,10 @@ impl Backtrace {
     /// ```
     #[inline(never)] // want to make sure there's a frame here to remove
     pub fn new() -> Backtrace {
+        // initialize dbghelp only once for both the trace and the resolve
+        #[cfg(all(windows, feature = "dbghelp"))]
+        let _c = unsafe { ::dbghelp_init() };
+
         let mut bt = Self::create(Self::new as usize);
         bt.resolve();
         bt
@@ -141,6 +145,10 @@ impl Backtrace {
     /// If this backtrace has been previously resolved or was created through
     /// `new`, this function does nothing.
     pub fn resolve(&mut self) {
+        // initialize dbghelp only once for all frames
+        #[cfg(all(windows, feature = "dbghelp"))]
+        let _c = unsafe { ::dbghelp_init() };
+
         for frame in self.frames.iter_mut().filter(|f| f.symbols.is_none()) {
             let mut symbols = Vec::new();
             resolve(frame.ip as *mut _, |symbol| {

src/lib.rs

@@ -177,18 +177,79 @@ mod lock {
     }
 }
 
-// requires external synchronization
 #[cfg(all(windows, feature = "dbghelp"))]
-unsafe fn dbghelp_init() {
+struct Cleanup {
+    handle: winapi::um::winnt::HANDLE,
+    opts: winapi::shared::minwindef::DWORD,
+}
+
+#[cfg(all(windows, feature = "dbghelp"))]
+unsafe fn dbghelp_init() -> Option<Cleanup> {
     use winapi::shared::minwindef;
     use winapi::um::{dbghelp, processthreadsapi};
 
-    static mut INITIALIZED: bool = false;
+    use std::sync::{Mutex, Once, ONCE_INIT};
+    use std::boxed::Box;
 
-    if !INITIALIZED {
-        dbghelp::SymInitializeW(processthreadsapi::GetCurrentProcess(),
-                                0 as *mut _,
-                                minwindef::TRUE);
-        INITIALIZED = true;
+    // Initializing symbols has significant overhead, but initializing only once
+    // without cleanup causes problems for external sources. For example, the
+    // standard library checks the result of SymInitializeW (which returns an
+    // error if attempting to initialize twice) and in the event of an error,
+    // will not print a backtrace on panic. Presumably, external debuggers may
+    // have similar issues.
+    //
+    // As a compromise, we'll keep track of the number of internal initialization
+    // requests within a single API call in order to minimize the number of
+    // init/cleanup cycles.
+    static mut REF_COUNT: *mut Mutex<usize> = 0 as *mut _;
+    static mut INIT: Once = ONCE_INIT;
+
+    INIT.call_once(|| {
+        REF_COUNT = Box::into_raw(Box::new(Mutex::new(0)));
+    });
+
+    // Not sure why these are missing in winapi
+    const SYMOPT_DEFERRED_LOADS: minwindef::DWORD = 0x00000004;
+    extern "system" {
+        fn SymGetOptions() -> minwindef::DWORD;
+        fn SymSetOptions(options: minwindef::DWORD);
     }
-}
+
+    impl Drop for Cleanup {
+        fn drop(&mut self) {
+            unsafe {
+                let mut ref_count_guard = (&*REF_COUNT).lock().unwrap();
+                *ref_count_guard -= 1;
+
+                if *ref_count_guard == 0 {
+                    dbghelp::SymCleanup(self.handle);
+                    SymSetOptions(self.opts);
+                }
+            }
+        }
+    }
+
+    let opts = SymGetOptions();
+    let handle = processthreadsapi::GetCurrentProcess();
+
+    let mut ref_count_guard = (&*REF_COUNT).lock().unwrap();
+
+    if *ref_count_guard > 0 {
+        *ref_count_guard += 1;
+        return Some(Cleanup { handle, opts });
+    }
+
+    SymSetOptions(opts | SYMOPT_DEFERRED_LOADS);
+
+    let ret = dbghelp::SymInitializeW(handle,
+                                      0 as *mut _,
+                                      minwindef::TRUE);
+
+    if ret != minwindef::TRUE {
+        // Symbols may have been initialized by another library or an external debugger
+        None
+    } else {
+        *ref_count_guard += 1;
+        Some(Cleanup { handle, opts })
+    }
+}

tests/long_fn_name.rs

@@ -23,7 +23,6 @@ mod _234567890_234567890_234567890_234567890_234567890 {
 #[test]
 #[cfg(all(windows, feature = "dbghelp", target_env = "msvc"))]
 fn test_long_fn_name() {
-    use winapi::um::dbghelp;
     use _234567890_234567890_234567890_234567890_234567890::
         _234567890_234567890_234567890_234567890_234567890 as S;