From cc2c4f8595f6ca898e4c8056351e2a5c72a5bdc7 Mon Sep 17 00:00:00 2001 From: Aaron Loucks Date: Fri, 3 May 2019 15:38:38 -0400 Subject: [PATCH] Cleanup dbghelp so that stdlib can print backtraces on panic again Fixes #165 --- Cargo.toml | 2 +- benches/benchmarks.rs | 96 +++++++++++++++++++++++++++++++++++++++++++ src/capture.rs | 8 ++++ src/lib.rs | 79 +++++++++++++++++++++++++++++++---- tests/long_fn_name.rs | 1 - 5 files changed, 175 insertions(+), 11 deletions(-) create mode 100644 benches/benchmarks.rs diff --git a/Cargo.toml b/Cargo.toml index 63772f210..4e0673f98 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -73,7 +73,7 @@ std = [] # function to acquire a backtrace libunwind = [] unix-backtrace = [] -dbghelp = [] +dbghelp = ["std"] kernel32 = [] #======================================= diff --git a/benches/benchmarks.rs b/benches/benchmarks.rs new file mode 100644 index 000000000..81cfe484a --- /dev/null +++ b/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); +} \ No newline at end of file diff --git a/src/capture.rs b/src/capture.rs index bb180883e..6c1da2773 100644 --- a/src/capture.rs +++ b/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| { diff --git a/src/lib.rs b/src/lib.rs index f69a873b2..376e634bd 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -179,18 +179,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 { 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; + + // 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 = 0 as *mut _; + static mut INIT: Once = ONCE_INIT; - if !INITIALIZED { - dbghelp::SymInitializeW(processthreadsapi::GetCurrentProcess(), - 0 as *mut _, - minwindef::TRUE); - INITIALIZED = true; + 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 }) + } +} \ No newline at end of file diff --git a/tests/long_fn_name.rs b/tests/long_fn_name.rs index 862b62e70..8b53dabe1 100644 --- a/tests/long_fn_name.rs +++ b/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;