Refactor dbghelp synchronization

Author: aloucks

Cargo.toml

@@ -67,14 +67,14 @@ std = []
 # - unix-backtrace: this uses the backtrace(3) function to acquire a
 #   backtrace, but is not as reliable as libunwind. It is, however,
 #   generally found in more locations.
-# - dbghelp: on windows this enables usage of dbghelp.dll to find a
-#   backtrace at runtime
+# - dbghelp: on windows this enables usage of dbghelp.dll to acquire and
+#   resolve a backtrace at runtime
 # - kernel32: on windows this enables using RtlCaptureStackBackTrace as the
 #   function to acquire a backtrace
 libunwind = []
 unix-backtrace = []
-dbghelp = ["std"]
-kernel32 = ["dbghelp"]
+dbghelp = []
+kernel32 = []
 
 #=======================================
 # Methods of resolving symbols

ci/azure-test-all.yml

@@ -37,5 +37,9 @@ steps:
     displayName: "Test backtrace (-default + gimli-symbolize + std)"
   - bash: cargo test --no-default-features --features 'dbghelp std'
     displayName: "Test backtrace (-default + dbghelp + std)"
+  - bash: cargo test --no-default-features --features 'kernel32 std'
+    displayName: "Test backtrace (-default + kernel32 + std)"
+  - bash: cargo test --no-default-features --features 'kernel32 dbghelp std'
+    displayName: "Test backtrace (-default + kernel32 + dbghelp + std)"
   - bash: cd ./cpp_smoke_test && cargo test
     displayName: "Test cpp_smoke_test"

src/backtrace/dbghelp.rs

@@ -62,7 +62,7 @@ pub unsafe fn trace(cb: &mut FnMut(&super::Frame) -> bool) {
     };
     let image = init_frame(&mut frame.inner.inner, &context.0);
 
-    let cleanup_on_drop = CleanupOnDrop;
+    let _cleanup_on_drop = CleanupOnDrop;
 
     ::TRACE_CLEANUP.with(|trace_cleanup| {
         let mut trace_cleanup = trace_cleanup.borrow_mut();
@@ -120,8 +120,6 @@ pub unsafe fn trace(cb: &mut FnMut(&super::Frame) -> bool) {
                 break
             }
         }
-
-        drop(cleanup_on_drop);
     }
 }
 

src/capture.rs

@@ -68,6 +68,8 @@ impl Backtrace {
     /// ```
     #[inline(never)] // want to make sure there's a frame here to remove
     pub fn new() -> Backtrace {
+        let _lock = ::lock::lock();
+
         // initialize dbghelp only once for both the trace and the resolve
         #[cfg(all(windows, feature = "dbghelp"))]
         let _c = unsafe { ::dbghelp_init() };
@@ -145,6 +147,8 @@ impl Backtrace {
     /// If this backtrace has been previously resolved or was created through
     /// `new`, this function does nothing.
     pub fn resolve(&mut self) {
+        let _lock = ::lock::lock();
+
         // initialize dbghelp only once for all frames
         #[cfg(all(windows, feature = "dbghelp"))]
         let _c = unsafe { ::dbghelp_init() };

src/lib.rs

@@ -180,29 +180,18 @@ mod lock {
 }
 
 #[cfg(all(windows, feature = "dbghelp"))]
-struct Cleanup {
-    handle: winapi::um::winnt::HANDLE,
-    opts: winapi::shared::minwindef::DWORD,
-}
+mod dbghelp {
+    use core::marker::PhantomData;
+    use winapi::um::dbghelp::{SymInitializeW, SymCleanup};
 
-#[cfg(all(windows, feature = "dbghelp"))]
-enum Trace {
-    Inside(Option<Cleanup>),
-    Outside,
-}
-
-thread_local! {
-    #[cfg(all(windows, feature = "dbghelp"))]
-    static TRACE_CLEANUP: std::cell::RefCell<Trace> = std::cell::RefCell::new(Trace::Outside);
-}
+    // Not sure why these are missing in winapi
 
-#[cfg(all(windows, feature = "dbghelp"))]
-unsafe fn dbghelp_init() -> Option<Cleanup> {
-    use winapi::shared::minwindef;
-    use winapi::um::{dbghelp, processthreadsapi};
+    const SYMOPT_DEFERRED_LOADS: winapi::shared::minwindef::DWORD = 0x00000004;
 
-    use std::sync::{Mutex, Once, ONCE_INIT};
-    use std::boxed::Box;
+    extern "system" {
+        fn SymGetOptions() -> winapi::shared::minwindef::DWORD;
+        fn SymSetOptions(options: winapi::shared::minwindef::DWORD);
+    }
 
     // Initializing symbols has significant overhead, but initializing only once
     // without cleanup causes problems for external sources. For example, the
@@ -214,68 +203,84 @@ unsafe fn dbghelp_init() -> Option<Cleanup> {
     // 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<usize> = 0 as *mut _;
-    static mut INIT: Once = ONCE_INIT;
+    static mut DBGHELP: DbgHelp = DbgHelp {
+        ref_count: 0,
+        opts: 0,
+        handle: core::ptr::null_mut()
+    };
+
+    struct DbgHelp {
+        ref_count: usize,
+        handle: winapi::um::winnt::HANDLE,
+        opts: winapi::shared::minwindef::DWORD,
+    }
 
-    INIT.call_once(|| {
-        REF_COUNT = Box::into_raw(Box::new(Mutex::new(0)));
-    });
+    pub struct Cleanup(PhantomData<*mut ()>);
 
-    // 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 Clone for Cleanup {
+        fn clone(&self) -> Cleanup {
+            unsafe {
+                DBGHELP.ref_count += 1;
+            }
+            Cleanup(PhantomData)
+        }
     }
 
     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);
+                DBGHELP.ref_count -= 1;
+                if DBGHELP.ref_count == 0 {
+                    SymCleanup(DBGHELP.handle);
+                    SymSetOptions(DBGHELP.opts);
                 }
             }
         }
     }
 
-    impl Clone for Cleanup {
-        fn clone(&self) -> Cleanup {
-            unsafe {
-                let mut ref_count_guard = (&*REF_COUNT).lock().unwrap();
-                *ref_count_guard += 1;
-            }
-            Cleanup {
-                opts: self.opts,
-                handle: self.handle
-            }
-        }
+    /// Tracks whether or not we have initialized dbghelp.dll inside of a call to `trace`.
+    pub enum Trace {
+        Inside(Option<Cleanup>),
+        Outside,
     }
 
-    let opts = SymGetOptions();
-    let handle = processthreadsapi::GetCurrentProcess();
+    thread_local! {
+        pub static TRACE_CLEANUP: core::cell::RefCell<Trace> = core::cell::RefCell::new(Trace::Outside);
+    }
 
-    let mut ref_count_guard = (&*REF_COUNT).lock().unwrap();
+    /// Requires external synchronization
+    pub unsafe fn init() -> Option<Cleanup> {
+        use winapi::shared::minwindef::TRUE;
+        use winapi::um::processthreadsapi::GetCurrentProcess;
 
-    if *ref_count_guard > 0 {
-        *ref_count_guard += 1;
-        return Some(Cleanup { handle, opts });
-    }
+        if DBGHELP.ref_count > 0 {
+            DBGHELP.ref_count += 1;
+            return Some(Cleanup(core::marker::PhantomData));
+        }
+
+        let opts = SymGetOptions();
+        let handle = GetCurrentProcess();
 
-    SymSetOptions(opts | SYMOPT_DEFERRED_LOADS);
+        SymSetOptions(opts | SYMOPT_DEFERRED_LOADS);
 
-    let ret = dbghelp::SymInitializeW(handle,
-                                      0 as *mut _,
-                                      minwindef::TRUE);
+        let ret = SymInitializeW(handle,  0 as *mut _,TRUE);
+        if ret != TRUE {
+            // Revert our changes
+            SymSetOptions(opts);
 
-    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 })
+            // Symbols may have been initialized by another library or an external debugger
+            None
+        } else {
+            DBGHELP.ref_count += 1;
+            DBGHELP.handle = handle;
+            DBGHELP.opts = opts;
+            Some(Cleanup(PhantomData))
+        }
     }
-}
+}
+
+#[cfg(all(windows, feature = "dbghelp"))]
+use dbghelp::init as dbghelp_init;
+
+#[cfg(all(windows, feature = "dbghelp"))]
+use dbghelp::{Trace, TRACE_CLEANUP};