From bf7aaedfe0cea23d4bb36a07b779314ae3045ad0 Mon Sep 17 00:00:00 2001 From: Tim Balsfulland Date: Tue, 24 Sep 2024 23:47:03 +0200 Subject: [PATCH 1/5] Synchronize access to `dbghelp.dll` between Tracy and Rust on Windows --- tracy-client-sys/Cargo.toml | 3 ++ tracy-client-sys/build.rs | 7 ++++ tracy-client-sys/src/dbghelp.rs | 64 +++++++++++++++++++++++++++++++++ tracy-client-sys/src/lib.rs | 3 ++ 4 files changed, 77 insertions(+) create mode 100644 tracy-client-sys/src/dbghelp.rs diff --git a/tracy-client-sys/Cargo.toml b/tracy-client-sys/Cargo.toml index 5c69d82..3a0e530 100644 --- a/tracy-client-sys/Cargo.toml +++ b/tracy-client-sys/Cargo.toml @@ -22,6 +22,9 @@ required-features = ["fibers"] [dependencies] +[target."cfg(windows)".dependencies] +windows = { version = "0.58.0", features = ["Win32_Security", "Win32_System_Threading"] } + [build-dependencies] cc = { version = "1.0.83", default-features = false } diff --git a/tracy-client-sys/build.rs b/tracy-client-sys/build.rs index 9dff0e4..ad44adb 100644 --- a/tracy-client-sys/build.rs +++ b/tracy-client-sys/build.rs @@ -86,6 +86,13 @@ fn set_feature_defines(mut c: cc::Build) -> cc::Build { fn build_tracy_client() { if std::env::var_os("CARGO_FEATURE_ENABLE").is_some() { let mut builder = set_feature_defines(cc::Build::new()); + + if std::env::var("CARGO_CFG_TARGET_OS").as_deref() == Ok("windows") { + // Used for synchronizing access to the `dbghelp.dll` symbol helper. + // See the `dbghelp` module for more information. + builder.define("TRACY_DBGHELP_LOCK", "RustBacktraceMutex"); + } + let _ = builder .file("tracy/TracyClient.cpp") .warnings(false) diff --git a/tracy-client-sys/src/dbghelp.rs b/tracy-client-sys/src/dbghelp.rs new file mode 100644 index 0000000..23489c0 --- /dev/null +++ b/tracy-client-sys/src/dbghelp.rs @@ -0,0 +1,64 @@ +//! On Windows, both Tracy and Rust use the `dbghelp.dll` symbol helper to resolve symbols for stack traces. +//! `dbghelp.dll` is single threaded and requires synchronization to call any of its functions. +//! +//! The Rust standard library includes the `backtrace-rs` crate for capturing and resolving backtraces. +//! When both the standard library and the `backtrace-rs` crate are used in the same program +//! they need to synchronize their access to `dbghelp.dll`. +//! They use a shared named Windows mutex for that, which we will use as well. +//! +//! Users of Tracy (like this crate) can define the `TRACY_DBGHELP_LOCK` variable for synchronizing access to `dbghelp.dll`. +//! We set `TRACY_DBGHELP_LOCK=RustBacktraceMutex` in the build script. +//! Tracy will call [`RustBacktraceMutexInit`], [`RustBacktraceMutexLock`], and [`RustBacktraceMutexUnlock`]. +//! In those functions a handle to the shared named mutex is created, the mutex is locked, and unlocked respectively. + +use std::io::Write; +use std::sync::atomic::{AtomicPtr, Ordering}; +use windows::core::PCSTR; +use windows::Win32::Foundation::{FALSE, HANDLE}; +use windows::Win32::System::Threading::{ + CreateMutexA, GetCurrentProcessId, ReleaseMutex, WaitForSingleObject, INFINITE, +}; + +/// Handle to the shared named Windows mutex that synchronizes access to the `dbghelp.dll` symbol helper, +/// with the standard library and `backtrace-rs`. +/// Gets initialized by [`RustBacktraceMutexInit`], +/// and because there is no cleanup function, the handle is leaked. +static RUST_BACKTRACE_MUTEX: AtomicPtr = AtomicPtr::new(std::ptr::null_mut()); + +#[no_mangle] +extern "C" fn RustBacktraceMutexInit() { + unsafe { + // The name is the same one that the standard library and `backtrace-rs` use + let mut name = [0; 33]; + let id = GetCurrentProcessId(); + write!(&mut name[..], "Local\\RustBacktraceMutex{id:08X}\0").unwrap(); + let name = PCSTR::from_raw(name.as_ptr()); + + // Creates a named mutex that is shared with the standard library and `backtrace-rs` + // to synchronize access to `dbghelp.dll` functions, which are single threaded. + let mutex = CreateMutexA(None, FALSE, name).unwrap(); + assert!(!mutex.is_invalid()); + + // The old value is ignored because this function is only called once, + // and normally the handle to the mutex is leaked anyway. + RUST_BACKTRACE_MUTEX.store(mutex.0, Ordering::Release); + } +} + +#[no_mangle] +extern "C" fn RustBacktraceMutexLock() { + unsafe { + let mutex = HANDLE(RUST_BACKTRACE_MUTEX.load(Ordering::Acquire)); + assert!(!mutex.is_invalid()); + WaitForSingleObject(mutex, INFINITE); + } +} + +#[no_mangle] +extern "C" fn RustBacktraceMutexUnlock() { + unsafe { + let mutex = HANDLE(RUST_BACKTRACE_MUTEX.load(Ordering::Acquire)); + assert!(!mutex.is_invalid()); + ReleaseMutex(mutex).unwrap(); + } +} diff --git a/tracy-client-sys/src/lib.rs b/tracy-client-sys/src/lib.rs index 7951ab2..89fefc6 100644 --- a/tracy-client-sys/src/lib.rs +++ b/tracy-client-sys/src/lib.rs @@ -50,3 +50,6 @@ pub use generated_manual_lifetime::*; mod generated_fibers; #[cfg(all(feature = "enable", feature = "fibers"))] pub use generated_fibers::{___tracy_fiber_enter, ___tracy_fiber_leave}; + +#[cfg(all(feature = "enable", target_os = "windows"))] +mod dbghelp; From 3fab387d3bd80d9b9c406a4130f3ef8340f3c9f2 Mon Sep 17 00:00:00 2001 From: Tim Balsfulland Date: Tue, 24 Sep 2024 23:49:32 +0200 Subject: [PATCH 2/5] Fix missing stack traces when not running in the target directory on Windows --- tracy-client-sys/src/dbghelp.rs | 28 ++++++++++++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-) diff --git a/tracy-client-sys/src/dbghelp.rs b/tracy-client-sys/src/dbghelp.rs index 23489c0..f65ac9b 100644 --- a/tracy-client-sys/src/dbghelp.rs +++ b/tracy-client-sys/src/dbghelp.rs @@ -10,11 +10,25 @@ //! We set `TRACY_DBGHELP_LOCK=RustBacktraceMutex` in the build script. //! Tracy will call [`RustBacktraceMutexInit`], [`RustBacktraceMutexLock`], and [`RustBacktraceMutexUnlock`]. //! In those functions a handle to the shared named mutex is created, the mutex is locked, and unlocked respectively. +//! +//! There is also an issue with initialization between Tracy and `backtrace-rs`. +//! In particular, the `SymInitialize` function should only be called once per process +//! and will return an error on subsequent calls. +//! Both Tracy and `backtrace-rs` ignore errors of the `SymInitialize` function, +//! so calling it multiple times is not an issue. +//! But `backtrace-rs` adds `SYMOPT_DEFERRED_LOADS` to the symbol options before initialization, +//! and adds the directory of all loaded modules (executable and DLLs) to the symbol search path. +//! That causes the symbols for Rust modules to be found even when the working directory isn't the Cargo target directory. +//! Tracy doesn't add the `SYMOPT_DEFERRED_LOADS` option and manually loads all modules. +//! Note that changing the symbol search path doesn't affect modules that were already loaded. +//! +//! Therefore, we want `backtrace-rs` to initialize and modify the symbol search path before Tracy. +//! To do that, a standard library backtrace is captured and resolved in [`RustBacktraceMutexInit`]. -use std::io::Write; +use std::io::{sink, Write}; use std::sync::atomic::{AtomicPtr, Ordering}; use windows::core::PCSTR; -use windows::Win32::Foundation::{FALSE, HANDLE}; +use windows::Win32::Foundation::{GetLastError, ERROR_ALREADY_EXISTS, FALSE, HANDLE}; use windows::Win32::System::Threading::{ CreateMutexA, GetCurrentProcessId, ReleaseMutex, WaitForSingleObject, INFINITE, }; @@ -28,6 +42,11 @@ static RUST_BACKTRACE_MUTEX: AtomicPtr = AtomicPtr::new(std:: #[no_mangle] extern "C" fn RustBacktraceMutexInit() { unsafe { + // Initialize the `dbghelp.dll` symbol helper by capturing and resolving a backtrace using the standard library. + // Since symbol resolution is lazy, the backtrace is written to `sink`, which forces symbol resolution. + // Refer to the module documentation on why the standard library should do the initialization instead of Tracy. + write!(sink(), "{:?}", std::backtrace::Backtrace::force_capture()).unwrap(); + // The name is the same one that the standard library and `backtrace-rs` use let mut name = [0; 33]; let id = GetCurrentProcessId(); @@ -39,6 +58,11 @@ extern "C" fn RustBacktraceMutexInit() { let mutex = CreateMutexA(None, FALSE, name).unwrap(); assert!(!mutex.is_invalid()); + // Initialization of the `dbghelp.dll` symbol helper should have already happened + // through the standard library backtrace above. + // Therefore, the shared named mutex should already have existed. + assert_eq!(GetLastError(), ERROR_ALREADY_EXISTS); + // The old value is ignored because this function is only called once, // and normally the handle to the mutex is leaked anyway. RUST_BACKTRACE_MUTEX.store(mutex.0, Ordering::Release); From 06fa38bd16acc1042d72e9a34bb0239f0205d354 Mon Sep 17 00:00:00 2001 From: Tim Balsfulland Date: Thu, 26 Sep 2024 19:18:52 +0200 Subject: [PATCH 3/5] use `windows-targets` instead of `windows` Only a small subset of the windows APIs is necessary. So we define that subset ourselves and avoid a heavy dependency. --- tracy-client-sys/Cargo.toml | 2 +- tracy-client-sys/src/dbghelp.rs | 48 +++++++++++++++++++++++---------- 2 files changed, 35 insertions(+), 15 deletions(-) diff --git a/tracy-client-sys/Cargo.toml b/tracy-client-sys/Cargo.toml index 3a0e530..8a9aae6 100644 --- a/tracy-client-sys/Cargo.toml +++ b/tracy-client-sys/Cargo.toml @@ -23,7 +23,7 @@ required-features = ["fibers"] [dependencies] [target."cfg(windows)".dependencies] -windows = { version = "0.58.0", features = ["Win32_Security", "Win32_System_Threading"] } +windows-targets = ">=0.48, <0.53" [build-dependencies] cc = { version = "1.0.83", default-features = false } diff --git a/tracy-client-sys/src/dbghelp.rs b/tracy-client-sys/src/dbghelp.rs index f65ac9b..2fd5a07 100644 --- a/tracy-client-sys/src/dbghelp.rs +++ b/tracy-client-sys/src/dbghelp.rs @@ -27,11 +27,31 @@ use std::io::{sink, Write}; use std::sync::atomic::{AtomicPtr, Ordering}; -use windows::core::PCSTR; -use windows::Win32::Foundation::{GetLastError, ERROR_ALREADY_EXISTS, FALSE, HANDLE}; -use windows::Win32::System::Threading::{ - CreateMutexA, GetCurrentProcessId, ReleaseMutex, WaitForSingleObject, INFINITE, -}; + +// Use the `windows_targets` crate and define all the things we need ourselves to avoid a dependency on `windows` +#[allow(clippy::upper_case_acronyms)] +type BOOL = i32; +#[allow(clippy::upper_case_acronyms)] +type HANDLE = *mut core::ffi::c_void; +#[allow(clippy::upper_case_acronyms)] +type PCSTR = *const u8; +type WIN32_ERROR = u32; +#[repr(C)] +struct SECURITY_ATTRIBUTES { + nLength: u32, + lpSecurityDescriptor: *mut core::ffi::c_void, + bInheritHandle: BOOL, +} + +const FALSE: BOOL = 0i32; +const ERROR_ALREADY_EXISTS: WIN32_ERROR = 183u32; +const INFINITE: u32 = u32::MAX; + +windows_targets::link!("kernel32.dll" "system" fn GetCurrentProcessId() -> u32); +windows_targets::link!("kernel32.dll" "system" fn CreateMutexA(lpmutexattributes: *const SECURITY_ATTRIBUTES, binitialowner: BOOL, lpname: PCSTR) -> HANDLE); +windows_targets::link!("kernel32.dll" "system" fn GetLastError() -> WIN32_ERROR); +windows_targets::link!("kernel32.dll" "system" fn WaitForSingleObject(hhandle: HANDLE, dwmilliseconds: u32) -> u32); +windows_targets::link!("kernel32.dll" "system" fn ReleaseMutex(hmutex: HANDLE) -> BOOL); /// Handle to the shared named Windows mutex that synchronizes access to the `dbghelp.dll` symbol helper, /// with the standard library and `backtrace-rs`. @@ -51,12 +71,12 @@ extern "C" fn RustBacktraceMutexInit() { let mut name = [0; 33]; let id = GetCurrentProcessId(); write!(&mut name[..], "Local\\RustBacktraceMutex{id:08X}\0").unwrap(); - let name = PCSTR::from_raw(name.as_ptr()); + let name: PCSTR = name.as_ptr(); // Creates a named mutex that is shared with the standard library and `backtrace-rs` // to synchronize access to `dbghelp.dll` functions, which are single threaded. - let mutex = CreateMutexA(None, FALSE, name).unwrap(); - assert!(!mutex.is_invalid()); + let mutex = CreateMutexA(std::ptr::null(), FALSE, name); + assert!(mutex != -1 as _ && mutex != 0 as _); // Initialization of the `dbghelp.dll` symbol helper should have already happened // through the standard library backtrace above. @@ -65,15 +85,15 @@ extern "C" fn RustBacktraceMutexInit() { // The old value is ignored because this function is only called once, // and normally the handle to the mutex is leaked anyway. - RUST_BACKTRACE_MUTEX.store(mutex.0, Ordering::Release); + RUST_BACKTRACE_MUTEX.store(mutex, Ordering::Release); } } #[no_mangle] extern "C" fn RustBacktraceMutexLock() { unsafe { - let mutex = HANDLE(RUST_BACKTRACE_MUTEX.load(Ordering::Acquire)); - assert!(!mutex.is_invalid()); + let mutex = RUST_BACKTRACE_MUTEX.load(Ordering::Acquire); + assert!(mutex != -1 as _ && mutex != 0 as _); WaitForSingleObject(mutex, INFINITE); } } @@ -81,8 +101,8 @@ extern "C" fn RustBacktraceMutexLock() { #[no_mangle] extern "C" fn RustBacktraceMutexUnlock() { unsafe { - let mutex = HANDLE(RUST_BACKTRACE_MUTEX.load(Ordering::Acquire)); - assert!(!mutex.is_invalid()); - ReleaseMutex(mutex).unwrap(); + let mutex = RUST_BACKTRACE_MUTEX.load(Ordering::Acquire); + assert!(mutex != -1 as _ && mutex != 0 as _); + assert_ne!(ReleaseMutex(mutex), 0); } } From a254a82ccac95094af82b1d36ea1befda37a8a61 Mon Sep 17 00:00:00 2001 From: Tim Balsfulland Date: Thu, 26 Sep 2024 19:33:16 +0200 Subject: [PATCH 4/5] simplify shared mutex name formatting --- tracy-client-sys/src/dbghelp.rs | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/tracy-client-sys/src/dbghelp.rs b/tracy-client-sys/src/dbghelp.rs index 2fd5a07..263ebfd 100644 --- a/tracy-client-sys/src/dbghelp.rs +++ b/tracy-client-sys/src/dbghelp.rs @@ -68,14 +68,11 @@ extern "C" fn RustBacktraceMutexInit() { write!(sink(), "{:?}", std::backtrace::Backtrace::force_capture()).unwrap(); // The name is the same one that the standard library and `backtrace-rs` use - let mut name = [0; 33]; - let id = GetCurrentProcessId(); - write!(&mut name[..], "Local\\RustBacktraceMutex{id:08X}\0").unwrap(); - let name: PCSTR = name.as_ptr(); + let name = format!("Local\\RustBacktraceMutex{:08X}\0", GetCurrentProcessId()); // Creates a named mutex that is shared with the standard library and `backtrace-rs` // to synchronize access to `dbghelp.dll` functions, which are single threaded. - let mutex = CreateMutexA(std::ptr::null(), FALSE, name); + let mutex = CreateMutexA(std::ptr::null(), FALSE, name.as_ptr()); assert!(mutex != -1 as _ && mutex != 0 as _); // Initialization of the `dbghelp.dll` symbol helper should have already happened From 9f4c7d73c6bbf7537dfd43b5ea1f486ab796130c Mon Sep 17 00:00:00 2001 From: Tim Balsfulland Date: Thu, 26 Sep 2024 20:08:23 +0200 Subject: [PATCH 5/5] better error handling for the backtrace mutex --- tracy-client-sys/src/dbghelp.rs | 34 ++++++++++++++++++++------------- 1 file changed, 21 insertions(+), 13 deletions(-) diff --git a/tracy-client-sys/src/dbghelp.rs b/tracy-client-sys/src/dbghelp.rs index 263ebfd..64cffee 100644 --- a/tracy-client-sys/src/dbghelp.rs +++ b/tracy-client-sys/src/dbghelp.rs @@ -46,6 +46,7 @@ struct SECURITY_ATTRIBUTES { const FALSE: BOOL = 0i32; const ERROR_ALREADY_EXISTS: WIN32_ERROR = 183u32; const INFINITE: u32 = u32::MAX; +const WAIT_FAILED: u32 = 0xFFFFFFFF; windows_targets::link!("kernel32.dll" "system" fn GetCurrentProcessId() -> u32); windows_targets::link!("kernel32.dll" "system" fn CreateMutexA(lpmutexattributes: *const SECURITY_ATTRIBUTES, binitialowner: BOOL, lpname: PCSTR) -> HANDLE); @@ -65,24 +66,24 @@ extern "C" fn RustBacktraceMutexInit() { // Initialize the `dbghelp.dll` symbol helper by capturing and resolving a backtrace using the standard library. // Since symbol resolution is lazy, the backtrace is written to `sink`, which forces symbol resolution. // Refer to the module documentation on why the standard library should do the initialization instead of Tracy. - write!(sink(), "{:?}", std::backtrace::Backtrace::force_capture()).unwrap(); + // Errors are ignored because we don't care about the actual output. + let _ = write!(sink(), "{:?}", std::backtrace::Backtrace::force_capture()); // The name is the same one that the standard library and `backtrace-rs` use let name = format!("Local\\RustBacktraceMutex{:08X}\0", GetCurrentProcessId()); - // Creates a named mutex that is shared with the standard library and `backtrace-rs` // to synchronize access to `dbghelp.dll` functions, which are single threaded. let mutex = CreateMutexA(std::ptr::null(), FALSE, name.as_ptr()); - assert!(mutex != -1 as _ && mutex != 0 as _); // Initialization of the `dbghelp.dll` symbol helper should have already happened // through the standard library backtrace above. - // Therefore, the shared named mutex should already have existed. - assert_eq!(GetLastError(), ERROR_ALREADY_EXISTS); - - // The old value is ignored because this function is only called once, - // and normally the handle to the mutex is leaked anyway. - RUST_BACKTRACE_MUTEX.store(mutex, Ordering::Release); + // To be robust against changes to symbol resolving in the standard library, + // the mutex is only used if it is valid and already existed. + if mutex != std::ptr::null_mut() && GetLastError() == ERROR_ALREADY_EXISTS { + // The old value is ignored because this function is only called once, + // and normally the handle to the mutex is leaked anyway. + RUST_BACKTRACE_MUTEX.store(mutex, Ordering::Release); + } } } @@ -90,8 +91,14 @@ extern "C" fn RustBacktraceMutexInit() { extern "C" fn RustBacktraceMutexLock() { unsafe { let mutex = RUST_BACKTRACE_MUTEX.load(Ordering::Acquire); - assert!(mutex != -1 as _ && mutex != 0 as _); - WaitForSingleObject(mutex, INFINITE); + if mutex != std::ptr::null_mut() { + assert_ne!( + WaitForSingleObject(mutex, INFINITE), + WAIT_FAILED, + "{}", + GetLastError() + ); + } } } @@ -99,7 +106,8 @@ extern "C" fn RustBacktraceMutexLock() { extern "C" fn RustBacktraceMutexUnlock() { unsafe { let mutex = RUST_BACKTRACE_MUTEX.load(Ordering::Acquire); - assert!(mutex != -1 as _ && mutex != 0 as _); - assert_ne!(ReleaseMutex(mutex), 0); + if mutex != std::ptr::null_mut() { + assert_ne!(ReleaseMutex(mutex), 0, "{}", GetLastError()); + } } }