From 891e91bf28d1f487524cd7aff43f74d1eda7df06 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michael=20W=C3=B6rister?= Date: Fri, 16 Feb 2024 13:25:59 +0100 Subject: [PATCH 1/3] Make dbghelp look for PDBs next to their exe/dll. --- .github/workflows/main.yml | 10 +++ src/dbghelp.rs | 150 +++++++++++++++++++++++++++++++++++++ src/windows.rs | 12 +++ 3 files changed, 172 insertions(+) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 21d7cb492..927582b79 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -110,6 +110,16 @@ jobs: - run: ./ci/debuglink-docker.sh if: contains(matrix.os, 'ubuntu') + # Test that backtraces are still symbolicated if we don't embed an absolute + # path to the PDB file in the binary. + - run: | + cargo clean + cargo test + if: contains(matrix.rust, 'msvc') + name: "Test that backtraces are symbolicated without absolute PDB path" + env: + RUSTFLAGS: "-C link-arg=/PDBALTPATH:%_PDB%" + # Test that including as a submodule will still work, both with and without # the `backtrace` feature enabled. - run: cargo build --manifest-path crates/as-if-std/Cargo.toml diff --git a/src/dbghelp.rs b/src/dbghelp.rs index e456dd45d..690d39881 100644 --- a/src/dbghelp.rs +++ b/src/dbghelp.rs @@ -23,9 +23,12 @@ #![allow(non_snake_case)] +use alloc::vec::Vec; + use super::windows::*; use core::mem; use core::ptr; +use core::slice; // Work around `SymGetOptions` and `SymSetOptions` not being present in winapi // itself. Otherwise this is only used when we're double-checking types against @@ -65,6 +68,17 @@ mod dbghelp { CurContext: LPDWORD, CurFrameIndex: LPDWORD, ) -> BOOL; + pub fn SymGetSearchPathW( + hprocess: HANDLE, + searchpatha: PWSTR, + searchpathlength: u32, + ) -> BOOL; + pub fn SymSetSearchPathW(hprocess: HANDLE, searchpatha: PCWSTR) -> BOOL; + pub fn EnumerateLoadedModulesW64( + hprocess: HANDLE, + enumloadedmodulescallback: PENUMLOADED_MODULES_CALLBACKW64, + usercontext: *const c_void, + ) -> BOOL; } pub fn assert_equal_types(a: T, _b: T) -> T { @@ -174,6 +188,20 @@ dbghelp! { path: PCWSTR, invade: BOOL ) -> BOOL; + fn SymGetSearchPathW( + hprocess: HANDLE, + searchpatha: PWSTR, + searchpathlength: u32 + ) -> BOOL; + fn SymSetSearchPathW( + hprocess: HANDLE, + searchpatha: PCWSTR + ) -> BOOL; + fn EnumerateLoadedModulesW64( + hprocess: HANDLE, + enumloadedmodulescallback: PENUMLOADED_MODULES_CALLBACKW64, + usercontext: *const c_void + ) -> BOOL; fn StackWalk64( MachineType: DWORD, hProcess: HANDLE, @@ -372,11 +400,133 @@ pub fn init() -> Result { // get to initialization first and the other will pick up that // initialization. DBGHELP.SymInitializeW().unwrap()(GetCurrentProcess(), ptr::null_mut(), TRUE); + + // The default search path for dbghelp will only look in the current working + // directory and (possibly) `_NT_SYMBOL_PATH` and `_NT_ALT_SYMBOL_PATH`. + // However, we also want to look in the directory of the executable + // and each DLL that is loaded. To do this, we need to update the search path + // to include these directories. + // + // See https://learn.microsoft.com/cpp/build/reference/pdbpath for an + // example of where symbols are usually searched for. + let mut search_path_buf = Vec::new(); + search_path_buf.resize(1024, 0); + + // Prefill the buffer with the current search path. + if DBGHELP.SymGetSearchPathW().unwrap()( + GetCurrentProcess(), + search_path_buf.as_mut_ptr(), + search_path_buf.len() as _, + ) == TRUE + { + // Trim the buffer to the actual length of the string. + let len = lstrlenW(search_path_buf.as_mut_ptr()); + assert!(len >= 0); + search_path_buf.truncate(len as usize); + } else { + // If getting the search path fails, at least include the current directory. + search_path_buf.clear(); + search_path_buf.push(utf16_char('.')); + search_path_buf.push(utf16_char(';')); + } + + let mut search_path = SearchPath::new(search_path_buf); + + // Update the search path to include the directory of the executable and each DLL. + DBGHELP.EnumerateLoadedModulesW64().unwrap()( + GetCurrentProcess(), + Some(enum_loaded_modules_callback), + &mut search_path as *mut _ as *mut _, + ); + + let new_search_path = search_path.finalize(); + + // Set the new search path. + DBGHELP.SymSetSearchPathW().unwrap()(GetCurrentProcess(), new_search_path.as_ptr()); + INITIALIZED = true; Ok(ret) } } +struct SearchPath { + search_path_utf16: Vec, +} + +fn utf16_char(c: char) -> u16 { + let buf = &mut [0u16; 2]; + let buf = c.encode_utf16(buf); + assert!(buf.len() == 1); + buf[0] +} + +impl SearchPath { + fn new(initial_search_path: Vec) -> Self { + Self { + search_path_utf16: initial_search_path, + } + } + + /// Add a path to the search path if it is not already present. + fn add(&mut self, path: &[u16]) { + let sep = utf16_char(';'); + + // We could deduplicate in a case-insensitive way, but case-sensitivity + // can be configured by directory on Windows, so let's not do that. + // https://learn.microsoft.com/windows/wsl/case-sensitivity + if !self + .search_path_utf16 + .split(|&c| c == sep) + .any(|p| p == path) + { + if self.search_path_utf16.last() != Some(&sep) { + self.search_path_utf16.push(sep); + } + self.search_path_utf16.extend_from_slice(path); + } + } + + fn finalize(mut self) -> Vec { + // Add a null terminator. + self.search_path_utf16.push(0); + self.search_path_utf16 + } +} + +extern "system" fn enum_loaded_modules_callback( + module_name: PCWSTR, + _: u64, + _: u32, + user_context: *const c_void, +) -> BOOL { + // `module_name` is an absolute path like `C:\path\to\module.dll` + // or `C:\path\to\module.exe` + let len: usize = unsafe { lstrlenW(module_name).try_into().unwrap() }; + + if len == 0 { + // This should not happen, but if it does, we can just ignore it. + return TRUE; + } + + let module_name = unsafe { slice::from_raw_parts(module_name, len) }; + let path_sep = utf16_char('\\'); + let alt_path_sep = utf16_char('/'); + + let Some(end_of_directory) = module_name + .iter() + .rposition(|&c| c == path_sep || c == alt_path_sep) + else { + // `module_name` being an absolute path, it should always contain at least one + // path separator. If not, there is nothing we can do. + return TRUE; + }; + + let search_path = unsafe { &mut *(user_context as *mut SearchPath) }; + search_path.add(&module_name[..end_of_directory]); + + TRUE +} + impl Drop for Init { fn drop(&mut self) { unsafe { diff --git a/src/windows.rs b/src/windows.rs index e85fbf167..8472ee66a 100644 --- a/src/windows.rs +++ b/src/windows.rs @@ -54,6 +54,16 @@ cfg_if::cfg_if! { ContextPointers: PKNONVOLATILE_CONTEXT_POINTERS ) -> PEXCEPTION_ROUTINE; } + + // winapi doesn't have this type + pub type PENUMLOADED_MODULES_CALLBACKW64 = Option< + unsafe extern "system" fn( + modulename: PCWSTR, + modulebase: u64, + modulesize: u32, + usercontext: *const c_void, + ) -> BOOL, + >; } } else { pub use core::ffi::c_void; @@ -291,6 +301,7 @@ ffi! { pub type PTRANSLATE_ADDRESS_ROUTINE64 = Option< unsafe extern "system" fn(hProcess: HANDLE, hThread: HANDLE, lpaddr: LPADDRESS64) -> DWORD64, >; + pub type PENUMLOADED_MODULES_CALLBACKW64 = Option BOOL>; pub type PGET_MODULE_BASE_ROUTINE64 = Option DWORD64>; pub type PFUNCTION_TABLE_ACCESS_ROUTINE64 = @@ -444,6 +455,7 @@ ffi! { hSnapshot: HANDLE, lpme: LPMODULEENTRY32W, ) -> BOOL; + pub fn lstrlenW(lpstring: PCWSTR) -> i32; } } From 17148e55f8a0a3e3488652fcc4a9035fd1792fec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michael=20W=C3=B6rister?= Date: Thu, 22 Feb 2024 11:14:55 +0100 Subject: [PATCH 2/3] Use Win32 typedefs for new Win32 API decls. --- src/dbghelp.rs | 16 ++++++++-------- src/windows.rs | 8 ++++---- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/src/dbghelp.rs b/src/dbghelp.rs index 690d39881..e739c8668 100644 --- a/src/dbghelp.rs +++ b/src/dbghelp.rs @@ -71,13 +71,13 @@ mod dbghelp { pub fn SymGetSearchPathW( hprocess: HANDLE, searchpatha: PWSTR, - searchpathlength: u32, + searchpathlength: DWORD, ) -> BOOL; pub fn SymSetSearchPathW(hprocess: HANDLE, searchpatha: PCWSTR) -> BOOL; pub fn EnumerateLoadedModulesW64( hprocess: HANDLE, enumloadedmodulescallback: PENUMLOADED_MODULES_CALLBACKW64, - usercontext: *const c_void, + usercontext: PVOID, ) -> BOOL; } @@ -191,7 +191,7 @@ dbghelp! { fn SymGetSearchPathW( hprocess: HANDLE, searchpatha: PWSTR, - searchpathlength: u32 + searchpathlength: DWORD ) -> BOOL; fn SymSetSearchPathW( hprocess: HANDLE, @@ -200,7 +200,7 @@ dbghelp! { fn EnumerateLoadedModulesW64( hprocess: HANDLE, enumloadedmodulescallback: PENUMLOADED_MODULES_CALLBACKW64, - usercontext: *const c_void + usercontext: PVOID ) -> BOOL; fn StackWalk64( MachineType: DWORD, @@ -436,7 +436,7 @@ pub fn init() -> Result { DBGHELP.EnumerateLoadedModulesW64().unwrap()( GetCurrentProcess(), Some(enum_loaded_modules_callback), - &mut search_path as *mut _ as *mut _, + ((&mut search_path) as *mut SearchPath) as *mut c_void, ); let new_search_path = search_path.finalize(); @@ -495,9 +495,9 @@ impl SearchPath { extern "system" fn enum_loaded_modules_callback( module_name: PCWSTR, - _: u64, - _: u32, - user_context: *const c_void, + _: DWORD64, + _: ULONG, + user_context: PVOID, ) -> BOOL { // `module_name` is an absolute path like `C:\path\to\module.dll` // or `C:\path\to\module.exe` diff --git a/src/windows.rs b/src/windows.rs index 8472ee66a..17bfd34b0 100644 --- a/src/windows.rs +++ b/src/windows.rs @@ -59,9 +59,9 @@ cfg_if::cfg_if! { pub type PENUMLOADED_MODULES_CALLBACKW64 = Option< unsafe extern "system" fn( modulename: PCWSTR, - modulebase: u64, - modulesize: u32, - usercontext: *const c_void, + modulebase: DWORD64, + modulesize: ULONG, + usercontext: PVOID, ) -> BOOL, >; } @@ -301,7 +301,7 @@ ffi! { pub type PTRANSLATE_ADDRESS_ROUTINE64 = Option< unsafe extern "system" fn(hProcess: HANDLE, hThread: HANDLE, lpaddr: LPADDRESS64) -> DWORD64, >; - pub type PENUMLOADED_MODULES_CALLBACKW64 = Option BOOL>; + pub type PENUMLOADED_MODULES_CALLBACKW64 = Option BOOL>; pub type PGET_MODULE_BASE_ROUTINE64 = Option DWORD64>; pub type PFUNCTION_TABLE_ACCESS_ROUTINE64 = From b3a2a03cec2600e72d76b0d3f5b3b710bada4eb0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michael=20W=C3=B6rister?= Date: Tue, 27 Feb 2024 10:46:04 +0100 Subject: [PATCH 3/3] Add -Cforce-frame-pointers to make the PDBALTPATH test case non-flaky on i686-pc-windows-msvc --- .github/workflows/main.yml | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 927582b79..e28c55a97 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -112,13 +112,14 @@ jobs: # Test that backtraces are still symbolicated if we don't embed an absolute # path to the PDB file in the binary. - - run: | - cargo clean - cargo test + # Add -Cforce-frame-pointers for stability. The test otherwise fails + # non-deterministically on i686-pc-windows-msvc because the stack cannot be + # unwound reliably. This failure is not related to the feature being tested. + - run: cargo clean && cargo test if: contains(matrix.rust, 'msvc') name: "Test that backtraces are symbolicated without absolute PDB path" env: - RUSTFLAGS: "-C link-arg=/PDBALTPATH:%_PDB%" + RUSTFLAGS: "-Clink-arg=/PDBALTPATH:%_PDB% -Cforce-frame-pointers" # Test that including as a submodule will still work, both with and without # the `backtrace` feature enabled.