Merge pull request #231 from rust-lang/test-init-once-windows

Ignore return of `SymInitializeW` on Windows

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