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#591
led to ac8c6d2 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.

Author: alexcrichton

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<ManuallyDrop<crate::dbghelp::Cleanup>>,
-    }
-
-    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::*;

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,75 +244,45 @@ 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.
 ///
 /// Note that the `Dbghelp` returned must also be dropped within the same
 /// lock.
 #[cfg(all(windows, feature = "dbghelp"))]
-pub unsafe fn init() -> Result<Cleanup, ()> {
-    // 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<Init, ()> {
+    // 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)
 }

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),
 ) {