From 8deb65160307c0cf71b07f5d930cba7ae6344bc1 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 3 Mar 2024 09:00:38 +0100 Subject: [PATCH 1/2] move thread-panic tests to their own file; test getting the thread name --- .../miri/tests/pass/concurrency/simple.rs | 19 -------------- .../miri/tests/pass/concurrency/simple.stderr | 5 ---- .../tests/pass/concurrency/thread_name.rs | 21 ++++++++++++++++ .../miri/tests/pass/panic/thread_panic.rs | 25 +++++++++++++++++++ .../miri/tests/pass/panic/thread_panic.stderr | 5 ++++ 5 files changed, 51 insertions(+), 24 deletions(-) delete mode 100644 src/tools/miri/tests/pass/concurrency/simple.stderr create mode 100644 src/tools/miri/tests/pass/concurrency/thread_name.rs create mode 100644 src/tools/miri/tests/pass/panic/thread_panic.rs create mode 100644 src/tools/miri/tests/pass/panic/thread_panic.stderr diff --git a/src/tools/miri/tests/pass/concurrency/simple.rs b/src/tools/miri/tests/pass/concurrency/simple.rs index ec549a998bae7..46033eea35d1d 100644 --- a/src/tools/miri/tests/pass/concurrency/simple.rs +++ b/src/tools/miri/tests/pass/concurrency/simple.rs @@ -45,23 +45,6 @@ fn create_move_out() { assert_eq!(result.len(), 6); } -fn panic() { - let result = thread::spawn(|| panic!("Hello!")).join().unwrap_err(); - let msg = result.downcast_ref::<&'static str>().unwrap(); - assert_eq!(*msg, "Hello!"); -} - -fn panic_named() { - thread::Builder::new() - .name("childthread".to_string()) - .spawn(move || { - panic!("Hello, world!"); - }) - .unwrap() - .join() - .unwrap_err(); -} - // This is not a data race! fn shared_readonly() { use std::sync::Arc; @@ -89,6 +72,4 @@ fn main() { create_move_in(); create_move_out(); shared_readonly(); - panic(); - panic_named(); } diff --git a/src/tools/miri/tests/pass/concurrency/simple.stderr b/src/tools/miri/tests/pass/concurrency/simple.stderr deleted file mode 100644 index 33d6b6841ade6..0000000000000 --- a/src/tools/miri/tests/pass/concurrency/simple.stderr +++ /dev/null @@ -1,5 +0,0 @@ -thread '' panicked at $DIR/simple.rs:LL:CC: -Hello! -note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace -thread 'childthread' panicked at $DIR/simple.rs:LL:CC: -Hello, world! diff --git a/src/tools/miri/tests/pass/concurrency/thread_name.rs b/src/tools/miri/tests/pass/concurrency/thread_name.rs new file mode 100644 index 0000000000000..6dd5f1f5c9149 --- /dev/null +++ b/src/tools/miri/tests/pass/concurrency/thread_name.rs @@ -0,0 +1,21 @@ +use std::thread; + +fn main() { + // When we have not set the name... + thread::spawn(|| { + assert!(thread::current().name().is_none()); + }); + + // ... and when we have set it. + thread::Builder::new() + .name("childthread".to_string()) + .spawn(move || { + assert_eq!(thread::current().name().unwrap(), "childthread"); + }) + .unwrap() + .join() + .unwrap(); + + // Also check main thread name. + assert_eq!(thread::current().name().unwrap(), "main"); +} diff --git a/src/tools/miri/tests/pass/panic/thread_panic.rs b/src/tools/miri/tests/pass/panic/thread_panic.rs new file mode 100644 index 0000000000000..9a3040b2c565a --- /dev/null +++ b/src/tools/miri/tests/pass/panic/thread_panic.rs @@ -0,0 +1,25 @@ +//! Panicking in other threads. + +use std::thread; + +fn panic() { + let result = thread::spawn(|| panic!("Hello!")).join().unwrap_err(); + let msg = result.downcast_ref::<&'static str>().unwrap(); + assert_eq!(*msg, "Hello!"); +} + +fn panic_named() { + thread::Builder::new() + .name("childthread".to_string()) + .spawn(move || { + panic!("Hello, world!"); + }) + .unwrap() + .join() + .unwrap_err(); +} + +fn main() { + panic(); + panic_named(); +} diff --git a/src/tools/miri/tests/pass/panic/thread_panic.stderr b/src/tools/miri/tests/pass/panic/thread_panic.stderr new file mode 100644 index 0000000000000..badd409d13fab --- /dev/null +++ b/src/tools/miri/tests/pass/panic/thread_panic.stderr @@ -0,0 +1,5 @@ +thread '' panicked at $DIR/thread_panic.rs:LL:CC: +Hello! +note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace +thread 'childthread' panicked at $DIR/thread_panic.rs:LL:CC: +Hello, world! From c72b48778df951b16b2161316d41195b788f7518 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 3 Mar 2024 09:58:27 +0100 Subject: [PATCH 2/2] Windows: support getting the thread name --- src/tools/miri/src/concurrency/data_race.rs | 4 +- src/tools/miri/src/concurrency/thread.rs | 11 ----- src/tools/miri/src/machine.rs | 7 ++- .../miri/src/shims/windows/foreign_items.rs | 40 ++++++++++++++-- .../miri/tests/pass/shims/windows-rand.rs | 6 +-- .../tests/pass/shims/windows-threadname.rs | 46 +++++++++++++++++++ 6 files changed, 94 insertions(+), 20 deletions(-) create mode 100644 src/tools/miri/tests/pass/shims/windows-threadname.rs diff --git a/src/tools/miri/src/concurrency/data_race.rs b/src/tools/miri/src/concurrency/data_race.rs index 1fc702fff56ac..e304488408381 100644 --- a/src/tools/miri/src/concurrency/data_race.rs +++ b/src/tools/miri/src/concurrency/data_race.rs @@ -812,6 +812,7 @@ impl VClockAlloc { | MiriMemoryKind::Miri | MiriMemoryKind::C | MiriMemoryKind::WinHeap + | MiriMemoryKind::WinLocal | MiriMemoryKind::Mmap, ) | MemoryKind::Stack => { @@ -820,7 +821,8 @@ impl VClockAlloc { alloc_timestamp.span = current_span; (alloc_timestamp, alloc_index) } - // Other global memory should trace races but be allocated at the 0 timestamp. + // Other global memory should trace races but be allocated at the 0 timestamp + // (conceptually they are allocated before everything). MemoryKind::Machine( MiriMemoryKind::Global | MiriMemoryKind::Machine diff --git a/src/tools/miri/src/concurrency/thread.rs b/src/tools/miri/src/concurrency/thread.rs index 43278387120cd..83ca27a467c8d 100644 --- a/src/tools/miri/src/concurrency/thread.rs +++ b/src/tools/miri/src/concurrency/thread.rs @@ -981,17 +981,6 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { this.machine.threads.set_thread_name(thread, new_thread_name); } - #[inline] - fn set_thread_name_wide(&mut self, thread: ThreadId, new_thread_name: &[u16]) { - let this = self.eval_context_mut(); - - // The Windows `GetThreadDescription` shim to get the thread name isn't implemented, so being lossy is okay. - // This is only read by diagnostics, which already use `from_utf8_lossy`. - this.machine - .threads - .set_thread_name(thread, String::from_utf16_lossy(new_thread_name).into_bytes()); - } - #[inline] fn get_thread_name<'c>(&'c self, thread: ThreadId) -> Option<&[u8]> where diff --git a/src/tools/miri/src/machine.rs b/src/tools/miri/src/machine.rs index c411962fcf2e2..d40f1c4525fc2 100644 --- a/src/tools/miri/src/machine.rs +++ b/src/tools/miri/src/machine.rs @@ -113,6 +113,8 @@ pub enum MiriMemoryKind { C, /// Windows `HeapAlloc` memory. WinHeap, + /// Windows "local" memory (to be freed with `LocalFree`) + WinLocal, /// Memory for args, errno, and other parts of the machine-managed environment. /// This memory may leak. Machine, @@ -144,7 +146,7 @@ impl MayLeak for MiriMemoryKind { fn may_leak(self) -> bool { use self::MiriMemoryKind::*; match self { - Rust | Miri | C | WinHeap | Runtime => false, + Rust | Miri | C | WinHeap | WinLocal | Runtime => false, Machine | Global | ExternStatic | Tls | Mmap => true, } } @@ -156,7 +158,7 @@ impl MiriMemoryKind { use self::MiriMemoryKind::*; match self { // Heap allocations are fine since the `Allocation` is created immediately. - Rust | Miri | C | WinHeap | Mmap => true, + Rust | Miri | C | WinHeap | WinLocal | Mmap => true, // Everything else is unclear, let's not show potentially confusing spans. Machine | Global | ExternStatic | Tls | Runtime => false, } @@ -171,6 +173,7 @@ impl fmt::Display for MiriMemoryKind { Miri => write!(f, "Miri bare-metal heap"), C => write!(f, "C heap"), WinHeap => write!(f, "Windows heap"), + WinLocal => write!(f, "Windows local memory"), Machine => write!(f, "machine-managed memory"), Runtime => write!(f, "language runtime memory"), Global => write!(f, "global (static or const)"), diff --git a/src/tools/miri/src/shims/windows/foreign_items.rs b/src/tools/miri/src/shims/windows/foreign_items.rs index 9674b9da686e0..734737a86dd58 100644 --- a/src/tools/miri/src/shims/windows/foreign_items.rs +++ b/src/tools/miri/src/shims/windows/foreign_items.rs @@ -5,6 +5,7 @@ use rustc_span::Symbol; use rustc_target::abi::Size; use rustc_target::spec::abi::Abi; +use crate::shims::os_str::bytes_to_os_str; use crate::*; use shims::foreign_items::EmulateForeignItemResult; use shims::windows::handle::{EvalContextExt as _, Handle, PseudoHandle}; @@ -12,7 +13,11 @@ use shims::windows::sync::EvalContextExt as _; use shims::windows::thread::EvalContextExt as _; fn is_dyn_sym(name: &str) -> bool { - matches!(name, "SetThreadDescription" | "WaitOnAddress" | "WakeByAddressSingle") + // std does dynamic detection for these symbols + matches!( + name, + "SetThreadDescription" | "GetThreadDescription" | "WaitOnAddress" | "WakeByAddressSingle" + ) } impl<'mir, 'tcx: 'mir> EvalContextExt<'mir, 'tcx> for crate::MiriInterpCx<'mir, 'tcx> {} @@ -172,6 +177,12 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { let res = this.realloc(ptr, size, MiriMemoryKind::WinHeap)?; this.write_pointer(res, dest)?; } + "LocalFree" => { + let [ptr] = this.check_shim(abi, Abi::System { unwind: false }, link_name, args)?; + let ptr = this.read_pointer(ptr)?; + this.free(ptr, MiriMemoryKind::WinLocal)?; + this.write_null(dest)?; + } // errno "SetLastError" => { @@ -403,7 +414,6 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { this.check_shim(abi, Abi::System { unwind: false }, link_name, args)?; let handle = this.read_scalar(handle)?; - let name = this.read_wide_str(this.read_pointer(name)?)?; let thread = match Handle::from_scalar(handle, this)? { @@ -412,7 +422,31 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { _ => this.invalid_handle("SetThreadDescription")?, }; - this.set_thread_name_wide(thread, &name); + // FIXME: use non-lossy conversion + this.set_thread_name(thread, String::from_utf16_lossy(&name).into_bytes()); + + this.write_null(dest)?; + } + "GetThreadDescription" => { + let [handle, name_ptr] = + this.check_shim(abi, Abi::System { unwind: false }, link_name, args)?; + + let handle = this.read_scalar(handle)?; + let name_ptr = this.deref_pointer(name_ptr)?; // the pointer where we should store the ptr to the name + + let thread = match Handle::from_scalar(handle, this)? { + Some(Handle::Thread(thread)) => thread, + Some(Handle::Pseudo(PseudoHandle::CurrentThread)) => this.get_active_thread(), + _ => this.invalid_handle("SetThreadDescription")?, + }; + // Looks like the default thread name is empty. + let name = this.get_thread_name(thread).unwrap_or(b"").to_owned(); + let name = this.alloc_os_str_as_wide_str( + bytes_to_os_str(&name)?, + MiriMemoryKind::WinLocal.into(), + )?; + + this.write_scalar(Scalar::from_maybe_pointer(name, this), &name_ptr)?; this.write_null(dest)?; } diff --git a/src/tools/miri/tests/pass/shims/windows-rand.rs b/src/tools/miri/tests/pass/shims/windows-rand.rs index 5754a82d89050..cfbc1d278a932 100644 --- a/src/tools/miri/tests/pass/shims/windows-rand.rs +++ b/src/tools/miri/tests/pass/shims/windows-rand.rs @@ -1,4 +1,4 @@ -//@only-target-windows: this directly tests windows only random functions +//@only-target-windows: this directly tests windows-only functions use core::ffi::c_void; use core::mem::size_of_val; use core::ptr::null_mut; @@ -26,12 +26,12 @@ extern "system" { #[cfg(target_arch = "x86")] #[link(name = "bcryptprimitives", kind = "raw-dylib", import_name_type = "undecorated")] extern "system" { - pub fn ProcessPrng(pbdata: *mut u8, cbdata: usize) -> BOOL; + fn ProcessPrng(pbdata: *mut u8, cbdata: usize) -> BOOL; } #[cfg(not(target_arch = "x86"))] #[link(name = "bcryptprimitives", kind = "raw-dylib")] extern "system" { - pub fn ProcessPrng(pbdata: *mut u8, cbdata: usize) -> BOOL; + fn ProcessPrng(pbdata: *mut u8, cbdata: usize) -> BOOL; } fn main() { diff --git a/src/tools/miri/tests/pass/shims/windows-threadname.rs b/src/tools/miri/tests/pass/shims/windows-threadname.rs new file mode 100644 index 0000000000000..c863ac670b6d5 --- /dev/null +++ b/src/tools/miri/tests/pass/shims/windows-threadname.rs @@ -0,0 +1,46 @@ +//@only-target-windows: this directly tests windows-only functions + +use std::ffi::OsStr; +use std::os::windows::ffi::OsStrExt; + +use core::ffi::c_void; +type HANDLE = *mut c_void; +type PWSTR = *mut u16; +type PCWSTR = *const u16; +type HRESULT = i32; +type HLOCAL = *mut ::core::ffi::c_void; +extern "system" { + fn GetCurrentThread() -> HANDLE; + fn GetThreadDescription(hthread: HANDLE, lpthreaddescription: *mut PWSTR) -> HRESULT; + fn SetThreadDescription(hthread: HANDLE, lpthreaddescription: PCWSTR) -> HRESULT; + fn LocalFree(hmem: HLOCAL) -> HLOCAL; +} + +fn to_u16s>(s: S) -> Vec { + let mut result: Vec<_> = s.as_ref().encode_wide().collect(); + result.push(0); + result +} + +fn main() { + unsafe { + let name = c"mythreadname"; + + let utf16 = to_u16s(name.to_str().unwrap()); + SetThreadDescription(GetCurrentThread(), utf16.as_ptr()); + + let mut ptr = core::ptr::null_mut::(); + let result = GetThreadDescription(GetCurrentThread(), &mut ptr); + assert!(result >= 0); + let name_gotten = String::from_utf16_lossy({ + let mut len = 0; + while *ptr.add(len) != 0 { + len += 1; + } + core::slice::from_raw_parts(ptr, len) + }); + assert_eq!(name_gotten, name.to_str().unwrap()); + let r = LocalFree(ptr.cast()); + assert!(r.is_null()); + } +}