diff --git a/src/capture.rs b/src/capture.rs index e9659d79f..59a461e50 100644 --- a/src/capture.rs +++ b/src/capture.rs @@ -124,7 +124,6 @@ impl Backtrace { /// enabled, and the `std` feature is enabled by default. #[inline(never)] // want to make sure there's a frame here to remove pub fn new() -> Backtrace { - let _guard = lock_and_platform_init(); let mut bt = Self::create(Self::new as usize); bt.resolve(); bt @@ -155,7 +154,6 @@ impl Backtrace { /// enabled, and the `std` feature is enabled by default. #[inline(never)] // want to make sure there's a frame here to remove pub fn new_unresolved() -> Backtrace { - let _guard = lock_and_platform_init(); Self::create(Self::new_unresolved as usize) } @@ -205,7 +203,6 @@ impl Backtrace { /// This function requires the `std` feature of the `backtrace` crate to be /// enabled, and the `std` feature is enabled by default. pub fn resolve(&mut self) { - let _guard = lock_and_platform_init(); for frame in self.frames.iter_mut().filter(|f| f.symbols.is_none()) { let mut symbols = Vec::new(); { @@ -418,51 +415,6 @@ impl fmt::Debug for BacktraceSymbol { } } -// When using `dbghelp` on Windows this is a performance optimization. If -// we don't do this then `SymInitializeW` is called once per trace and once per -// frame during resolution. That function, however, takes quite some time! To -// help speed it up this function can amortize the calls necessary by ensuring -// that the scope this is called in only initializes when this is called and -// doesn't reinitialize for the rest of the scope. -#[cfg(all(windows, feature = "dbghelp"))] -fn lock_and_platform_init() -> impl Drop { - use std::mem::ManuallyDrop; - - struct Cleanup { - _lock: crate::lock::LockGuard, - - // Need to make sure this is cleaned up before `_lock` - dbghelp_cleanup: Option>, - } - - impl Drop for Cleanup { - fn drop(&mut self) { - if let Some(cleanup) = self.dbghelp_cleanup.as_mut() { - // Unsafety here should be ok since we're only dropping this in - // `Drop` to ensure it's dropped before the lock, and `Drop` - // should only be called once. - unsafe { - ManuallyDrop::drop(cleanup); - } - } - } - } - - // Unsafety here should be ok because we only acquire the `dbghelp` - // initialization (the unsafe part) after acquiring the global lock for this - // crate. Note that we're also careful to drop it before the lock is - // dropped. - unsafe { - Cleanup { - _lock: crate::lock::lock(), - dbghelp_cleanup: crate::dbghelp::init().ok().map(ManuallyDrop::new), - } - } -} - -#[cfg(not(all(windows, feature = "dbghelp")))] -fn lock_and_platform_init() {} - #[cfg(feature = "serialize-rustc")] mod rustc_serialize_impls { use super::*; diff --git a/src/dbghelp.rs b/src/dbghelp.rs index 256244287..8db4e7637 100644 --- a/src/dbghelp.rs +++ b/src/dbghelp.rs @@ -154,7 +154,7 @@ macro_rules! dbghelp { // Convenience proxy to use the cleanup locks to reference dbghelp // functions. #[allow(dead_code)] - impl Cleanup { + impl Init { $(pub fn $name(&self) -> $name { unsafe { DBGHELP.$name().unwrap() @@ -244,14 +244,7 @@ dbghelp! { } } -pub struct Cleanup; - -// Number of times `init` has been called on this thread. This is externally -// synchronized and doesn't use internal synchronization on our behalf. -static mut COUNT: usize = 0; - -// Used to restore `SymSetOptions` and `SymGetOptions` values. -static mut OPTS_ORIG: DWORD = 0; +pub struct Init; /// Unsafe because this requires external synchronization, must be done /// inside of the same lock as all other backtrace operations. @@ -259,60 +252,37 @@ static mut OPTS_ORIG: DWORD = 0; /// Note that the `Dbghelp` returned must also be dropped within the same /// lock. #[cfg(all(windows, feature = "dbghelp"))] -pub unsafe fn init() -> Result { - // 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. - if COUNT > 0 { - COUNT += 1; - return Ok(Cleanup); +pub unsafe fn init() -> Result { + // Calling `SymInitializeW` is quite expensive, so we only do so once per + // process. + static mut INITIALIZED: bool = false; + if INITIALIZED { + return Ok(Init); } // Actually load `dbghelp.dll` into the process here, returning an error if // that fails. DBGHELP.ensure_open()?; - OPTS_ORIG = DBGHELP.SymGetOptions().unwrap()(); + let orig = DBGHELP.SymGetOptions().unwrap()(); // Ensure that the `SYMOPT_DEFERRED_LOADS` flag is set, because // according to MSVC's own docs about this: "This is the fastest, most // efficient way to use the symbol handler.", so let's do that! - DBGHELP.SymSetOptions().unwrap()(OPTS_ORIG | SYMOPT_DEFERRED_LOADS); - - let ret = DBGHELP.SymInitializeW().unwrap()(GetCurrentProcess(), ptr::null_mut(), TRUE); - if ret != TRUE { - // Symbols may have been initialized by another library or an - // external debugger - DBGHELP.SymSetOptions().unwrap()(OPTS_ORIG); - Err(()) - } else { - COUNT += 1; - Ok(Cleanup) - } -} + DBGHELP.SymSetOptions().unwrap()(orig | SYMOPT_DEFERRED_LOADS); -impl Drop for Cleanup { - fn drop(&mut self) { - unsafe { - COUNT -= 1; - if COUNT != 0 { - return; - } - - // Clean up after ourselves by cleaning up symbols and restoring the - // symbol options to their original value. This is currently - // required to cooperate with libstd as libstd's backtracing will - // assert symbol initialization succeeds and will clean up after the - // backtrace is finished. - DBGHELP.SymCleanup().unwrap()(GetCurrentProcess()); - DBGHELP.SymSetOptions().unwrap()(OPTS_ORIG); - } - } + // Actually initialize symbols with MSVC. Note that this can fail, but we + // ignore it. There's not a ton of prior art for this per se, but LLVM + // internally seems to ignore the return value here and one of the + // sanitizer libraries in LLVM prints a scary warning if this fails but + // basically ignores it in the long run. + // + // One case this comes up a lot for Rust is that the standard library and + // this crate on crates.io both want to compete for `SymInitializeW`. The + // standard library historically wanted to initialize then cleanup most of + // the time, but now that it's using this crate it means that someone will + // get to initialization first and the other will pick up that + // initialization. + DBGHELP.SymInitializeW().unwrap()(GetCurrentProcess(), ptr::null_mut(), TRUE); + Ok(Init) } diff --git a/src/symbolize/dbghelp.rs b/src/symbolize/dbghelp.rs index 6f2461fcb..7afcee736 100644 --- a/src/symbolize/dbghelp.rs +++ b/src/symbolize/dbghelp.rs @@ -98,7 +98,7 @@ pub unsafe fn resolve(what: ResolveWhat, cb: &mut FnMut(&super::Symbol)) { } unsafe fn resolve_with_inline( - dbghelp: &dbghelp::Cleanup, + dbghelp: &dbghelp::Init, frame: &STACKFRAME_EX, cb: &mut FnMut(&super::Symbol), ) { @@ -127,7 +127,7 @@ unsafe fn resolve_with_inline( } unsafe fn resolve_without_inline( - dbghelp: &dbghelp::Cleanup, + dbghelp: &dbghelp::Init, addr: *mut c_void, cb: &mut FnMut(&super::Symbol), ) {