From e6ac926b7511e97a26468bee221b00f6e973f9a2 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Wed, 31 Jul 2019 08:53:56 -0700 Subject: [PATCH] Ignore return of `SymInitializeW` on Windows This commit updates the behavior of backtraces on Windows to execute `SymInitializeW` globally once-per-process and ignore the return value as to whether it succeeded or not. This undoes previous work in this crate to specifically check the return value, and this is a behavior update for the standard library when this goes into the standard library. The `SymInitializeW` function is required to be called on MSVC before any backtraces can be captured or before any addresses can be symbolized. This function is quite slow. It can only be called once-per-process and there's a corresponding `SymCleanup` to undo the work of `SymInitializeW`. The behavior of what to do with `SymInitializeW` has changed a lot over time in this crate. The very first implementation for Windows paired with `SymCleanup`. Then reports of slowness at rust-lang/rustup.rs#591 led to ac8c6d23db where `SymCleanup` was removed. This led to #165 where because the standard library still checked `SymInitializeW`'s return value and called `SymCleanup` generating a backtrace with this crate would break `RUST_BACKTRACE=1`. Finally (and expectedly) the performance report has come back as #229 for this crate. I'm proposing that the final nail is put in this coffin at this point where this crate will ignore the return value of `SymInitializeW`, initializing symbols once per process globally. This update will go into the standard library and get updated on the stable channel eventually, meaning both libstd and this crate will be able to work with one another and only initialize the process's symbols once globally. This does mean that there will be a period of "breakage" where we will continue to make `RUST_BACKTRACE=1` not useful if this crate is used on MSVC, but that's sort of the extension of the status quo as of a month or so ago. This will eventually be fixed once the stable channel of Rust is updated. --- src/capture.rs | 48 ------------------------- src/dbghelp.rs | 78 +++++++++++++--------------------------- src/symbolize/dbghelp.rs | 4 +-- 3 files changed, 26 insertions(+), 104 deletions(-) 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), ) {