From d34242e8f1c538a8b4e01100a273ef1c5d456abd Mon Sep 17 00:00:00 2001 From: DrMeepster <19316085+DrMeepster@users.noreply.github.com> Date: Sun, 31 Jul 2022 17:15:15 -0700 Subject: [PATCH] fix various issues --- src/shims/os_str.rs | 2 +- src/shims/tls.rs | 5 +- src/shims/unix/thread.rs | 15 +- src/shims/windows/dlsym.rs | 2 +- src/shims/windows/foreign_items.rs | 25 +-- src/shims/windows/handle.rs | 29 +-- src/shims/windows/thread.rs | 48 +++-- src/thread.rs | 23 ++- .../libc_pthread_join_detached.stderr | 6 +- .../concurrency/libc_pthread_join_joined.rs | 2 +- .../libc_pthread_join_joined.stderr | 6 +- .../concurrency/libc_pthread_join_main.rs | 2 +- .../concurrency/libc_pthread_join_main.stderr | 6 +- .../concurrency/libc_pthread_join_multiple.rs | 2 +- .../libc_pthread_join_multiple.stderr | 4 +- tests/fail/concurrency/windows_join_main.rs | 2 +- tests/pass/concurrency/tls_lib_drop.rs | 2 + .../pass/concurrency/tls_lib_drop_windows.rs | 191 ++++++++++++++++++ .../concurrency/tls_lib_drop_windows.stdout | 5 + 19 files changed, 306 insertions(+), 71 deletions(-) create mode 100644 tests/pass/concurrency/tls_lib_drop_windows.rs create mode 100644 tests/pass/concurrency/tls_lib_drop_windows.stdout diff --git a/src/shims/os_str.rs b/src/shims/os_str.rs index f99e2d174b53..fcf92dfc9f93 100644 --- a/src/shims/os_str.rs +++ b/src/shims/os_str.rs @@ -74,7 +74,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx 'mir: 'a, { #[cfg(windows)] - pub fn u16vec_to_osstring<'tcx, 'a>(u16_vec: Vec) -> InterpResult<'tcx, OsString> { + pub fn u16vec_to_osstring<'tcx>(u16_vec: Vec) -> InterpResult<'tcx, OsString> { Ok(OsString::from_wide(&u16_vec[..])) } #[cfg(not(windows))] diff --git a/src/shims/tls.rs b/src/shims/tls.rs index 8627d9a04479..a52054879811 100644 --- a/src/shims/tls.rs +++ b/src/shims/tls.rs @@ -244,10 +244,13 @@ trait EvalContextPrivExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx this.eval_windows("thread_local_key", "p_thread_callback")?.to_pointer(this)?; let thread_callback = this.get_ptr_fn(thread_callback)?.as_instance()?; - // Technically, the reason should be `DLL_PROCESS_DETACH` when the main thread exits but std ignores it. + // FIXME: Technically, the reason should be `DLL_PROCESS_DETACH` when the main thread exits + // but std treats both the same. let reason = this.eval_windows("c", "DLL_THREAD_DETACH")?; // The signature of this function is `unsafe extern "system" fn(h: c::LPVOID, dwReason: c::DWORD, pv: c::LPVOID)`. + // FIXME: `h` should be a handle to the current module and what `pv` should be is unknown + // but both are ignored by std this.call_function( thread_callback, Abi::System { unwind: false }, diff --git a/src/shims/unix/thread.rs b/src/shims/unix/thread.rs index d675df0f53f1..9365ec9a21f4 100644 --- a/src/shims/unix/thread.rs +++ b/src/shims/unix/thread.rs @@ -13,11 +13,17 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx ) -> InterpResult<'tcx, i32> { let this = self.eval_context_mut(); + let thread_info_place = this.deref_operand(thread)?; + + let start_routine = this.read_pointer(start_routine)?; + + let func_arg = this.read_immediate(arg)?; + this.start_thread( - Some(thread), + Some(thread_info_place), start_routine, Abi::C { unwind: false }, - arg, + func_arg, this.layout_of(this.tcx.types.usize)?, )?; @@ -46,7 +52,10 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let this = self.eval_context_mut(); let thread_id = this.read_scalar(thread)?.to_machine_usize(this)?; - this.detach_thread(thread_id.try_into().expect("thread ID should fit in u32"), false)?; + this.detach_thread( + thread_id.try_into().expect("thread ID should fit in u32"), + /*allow_terminated_joined*/ false, + )?; Ok(0) } diff --git a/src/shims/windows/dlsym.rs b/src/shims/windows/dlsym.rs index fc36913638e0..4c5e7a9b3138 100644 --- a/src/shims/windows/dlsym.rs +++ b/src/shims/windows/dlsym.rs @@ -122,7 +122,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx _ => this.invalid_handle("SetThreadDescription")?, }; - this.set_thread_name_wide(thread, name); + this.set_thread_name_wide(thread, &name); this.write_null(dest)?; } diff --git a/src/shims/windows/foreign_items.rs b/src/shims/windows/foreign_items.rs index cc030ec3d0cf..d853f3084d49 100644 --- a/src/shims/windows/foreign_items.rs +++ b/src/shims/windows/foreign_items.rs @@ -323,15 +323,6 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx // FIXME: we should set last_error, but to what? this.write_null(dest)?; } - // this is only callable from std because we know that std ignores the return value - "SwitchToThread" if this.frame_in_std() => { - let [] = this.check_shim(abi, Abi::System { unwind: false }, link_name, args)?; - - this.yield_active_thread(); - - // FIXME: this should return a nonzero value if this call does result in switching to another thread. - this.write_null(dest)?; - } "GetStdHandle" => { let [which] = this.check_shim(abi, Abi::System { unwind: false }, link_name, args)?; @@ -339,6 +330,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx // We just make this the identity function, so we know later in `NtWriteFile` which // one it is. This is very fake, but libtest needs it so we cannot make it a // std-only shim. + // FIXME: this should return real HANDLEs when io support is added this.write_scalar(Scalar::from_machine_isize(which.into(), this), dest)?; } "CloseHandle" => { @@ -364,9 +356,8 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let [handle, timeout] = this.check_shim(abi, Abi::System { unwind: false }, link_name, args)?; - this.WaitForSingleObject(handle, timeout)?; - - this.write_scalar(Scalar::from_u32(0), dest)?; + let ret = this.WaitForSingleObject(handle, timeout)?; + this.write_scalar(Scalar::from_u32(ret), dest)?; } "GetCurrentThread" => { let [] = this.check_shim(abi, Abi::System { unwind: false }, link_name, args)?; @@ -382,6 +373,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx "GetProcessHeap" if this.frame_in_std() => { let [] = this.check_shim(abi, Abi::System { unwind: false }, link_name, args)?; // Just fake a HANDLE + // It's fine to not use the Handle type here because its a stub this.write_scalar(Scalar::from_machine_isize(1, this), dest)?; } "GetModuleHandleA" if this.frame_in_std() => { @@ -417,6 +409,15 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let result = this.GetCurrentProcessId()?; this.write_scalar(Scalar::from_u32(result), dest)?; } + // this is only callable from std because we know that std ignores the return value + "SwitchToThread" if this.frame_in_std() => { + let [] = this.check_shim(abi, Abi::System { unwind: false }, link_name, args)?; + + this.yield_active_thread(); + + // FIXME: this should return a nonzero value if this call does result in switching to another thread. + this.write_null(dest)?; + } _ => return Ok(EmulateByNameResult::NotSupported), } diff --git a/src/shims/windows/handle.rs b/src/shims/windows/handle.rs index 041033717e44..443af1dfeaaa 100644 --- a/src/shims/windows/handle.rs +++ b/src/shims/windows/handle.rs @@ -8,6 +8,14 @@ pub enum PseudoHandle { CurrentThread, } +/// Miri representation of a Windows `HANDLE` +#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)] +pub enum Handle { + Null, + Pseudo(PseudoHandle), + Thread(ThreadId), +} + impl PseudoHandle { const CURRENT_THREAD_VALUE: u32 = 0; @@ -25,14 +33,6 @@ impl PseudoHandle { } } -/// Miri representation of a Windows `HANDLE` -#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)] -pub enum Handle { - Null, - Pseudo(PseudoHandle), - Thread(ThreadId), -} - impl Handle { const NULL_DISCRIMINANT: u32 = 0; const PSEUDO_DISCRIMINANT: u32 = 1; @@ -62,9 +62,7 @@ impl Handle { let floor_log2 = variant_count.ilog2(); // we need to add one for non powers of two to compensate for the difference - let ceil_log2 = if variant_count.is_power_of_two() { floor_log2 } else { floor_log2 + 1 }; - - ceil_log2 + if variant_count.is_power_of_two() { floor_log2 } else { floor_log2 + 1 } } /// Converts a handle into its machine representation. @@ -120,6 +118,7 @@ impl Handle { pub fn to_scalar(self, cx: &impl HasDataLayout) -> Scalar { // 64-bit handles are sign extended 32-bit handles // see https://docs.microsoft.com/en-us/windows/win32/winprog64/interprocess-communication + #[allow(clippy::cast_possible_wrap)] // we want it to wrap let signed_handle = self.to_packed() as i32; Scalar::from_machine_isize(signed_handle.into(), cx) } @@ -130,6 +129,7 @@ impl Handle { ) -> InterpResult<'tcx, Option> { let sign_extended_handle = handle.to_machine_isize(cx)?; + #[allow(clippy::cast_sign_loss)] // we want to lose the sign let handle = if let Ok(signed_handle) = i32::try_from(sign_extended_handle) { signed_handle as u32 } else { @@ -154,8 +154,11 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx fn CloseHandle(&mut self, handle_op: &OpTy<'tcx, Provenance>) -> InterpResult<'tcx> { let this = self.eval_context_mut(); - match Handle::from_scalar(this.read_scalar(handle_op)?.check_init()?, this)? { - Some(Handle::Thread(thread)) => this.detach_thread(thread, true)?, + let handle = this.read_scalar(handle_op)?.check_init()?; + + match Handle::from_scalar(handle, this)? { + Some(Handle::Thread(thread)) => + this.detach_thread(thread, /*allow_terminated_joined*/ true)?, _ => this.invalid_handle("CloseHandle")?, } diff --git a/src/shims/windows/thread.rs b/src/shims/windows/thread.rs index 08eb4ddba10c..06a5887d3e50 100644 --- a/src/shims/windows/thread.rs +++ b/src/shims/windows/thread.rs @@ -19,55 +19,71 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx ) -> InterpResult<'tcx, ThreadId> { let this = self.eval_context_mut(); - if !this.ptr_is_null(this.read_pointer(security_op)?)? { - throw_unsup_format!("non-null `lpThreadAttributes` in `CreateThread`") - } + let security = this.read_pointer(security_op)?; // stacksize is ignored, but still needs to be a valid usize - let _ = this.read_scalar(stacksize_op)?.to_machine_usize(this)?; + this.read_scalar(stacksize_op)?.to_machine_usize(this)?; + + let start_routine = this.read_pointer(start_op)?; + + let func_arg = this.read_immediate(arg_op)?; let flags = this.read_scalar(flags_op)?.to_u32()?; + let thread = if this.ptr_is_null(this.read_pointer(thread_op)?)? { + None + } else { + let thread_info_place = this.deref_operand(thread_op)?; + Some(thread_info_place) + }; + let stack_size_param_is_a_reservation = this.eval_windows("c", "STACK_SIZE_PARAM_IS_A_RESERVATION")?.to_u32()?; + // We ignore the stack size, so we also ignore the + // `STACK_SIZE_PARAM_IS_A_RESERVATION` flag. if flags != 0 && flags != stack_size_param_is_a_reservation { throw_unsup_format!("unsupported `dwCreationFlags` {} in `CreateThread`", flags) } - let thread = - if this.ptr_is_null(this.read_pointer(thread_op)?)? { None } else { Some(thread_op) }; + if !this.ptr_is_null(security)? { + throw_unsup_format!("non-null `lpThreadAttributes` in `CreateThread`") + } this.start_thread( thread, - start_op, + start_routine, Abi::System { unwind: false }, - arg_op, + func_arg, this.layout_of(this.tcx.types.u32)?, ) } fn WaitForSingleObject( &mut self, - handle: &OpTy<'tcx, Provenance>, - timeout: &OpTy<'tcx, Provenance>, - ) -> InterpResult<'tcx> { + handle_op: &OpTy<'tcx, Provenance>, + timeout_op: &OpTy<'tcx, Provenance>, + ) -> InterpResult<'tcx, u32> { let this = self.eval_context_mut(); - let thread = match Handle::from_scalar(this.read_scalar(handle)?.check_init()?, this)? { + let handle = this.read_scalar(handle_op)?.check_init()?; + + let timeout = this.read_scalar(timeout_op)?.to_u32()?; + + let thread = match Handle::from_scalar(handle, this)? { Some(Handle::Thread(thread)) => thread, - // Unlike on posix, joining the current thread is not UB on windows. - // It will just deadlock. + // Unlike on posix, the outcome of joining the current thread is not documented. + // On current Windows, it just deadlocks. Some(Handle::Pseudo(PseudoHandle::CurrentThread)) => this.get_active_thread(), _ => this.invalid_handle("WaitForSingleObject")?, }; - if this.read_scalar(timeout)?.to_u32()? != this.eval_windows("c", "INFINITE")?.to_u32()? { + if timeout != this.eval_windows("c", "INFINITE")?.to_u32()? { throw_unsup_format!("`WaitForSingleObject` with non-infinite timeout"); } this.join_thread(thread)?; - Ok(()) + Ok(0) } } diff --git a/src/thread.rs b/src/thread.rs index f7fcdd5822a2..b92728be2087 100644 --- a/src/thread.rs +++ b/src/thread.rs @@ -181,7 +181,7 @@ impl<'mir, 'tcx> Thread<'mir, 'tcx> { } /// A specific moment in time. -#[derive(Debug, Copy, Clone)] +#[derive(Debug)] pub enum Time { Monotonic(Instant), RealTime(SystemTime), @@ -357,16 +357,19 @@ impl<'mir, 'tcx: 'mir> ThreadManager<'mir, 'tcx> { /// /// `allow_terminated_joined` allows detaching joined threads that have already terminated. /// This matches Windows's behavior for `CloseHandle`. + /// + /// See : + /// > The handle is valid until closed, even after the thread it represents has been terminated. fn detach_thread(&mut self, id: ThreadId, allow_terminated_joined: bool) -> InterpResult<'tcx> { trace!("detaching {:?}", id); let is_ub = if allow_terminated_joined && self.threads[id].state == ThreadState::Terminated { + // "Detached" in particular means "not yet joined". Redundant detaching is still UB. self.threads[id].join_status == ThreadJoinStatus::Detached } else { self.threads[id].join_status != ThreadJoinStatus::Joinable }; - if is_ub { throw_ub_format!("trying to detach thread that was already detached or joined"); } @@ -406,7 +409,7 @@ impl<'mir, 'tcx: 'mir> ThreadManager<'mir, 'tcx> { } /// Mark that the active thread tries to exclusively join the thread with `joined_thread_id`. - /// If the thread is already joined by another thread + /// If the thread is already joined by another thread, it will throw UB fn join_thread_exclusive( &mut self, joined_thread_id: ThreadId, @@ -424,7 +427,7 @@ impl<'mir, 'tcx: 'mir> ThreadManager<'mir, 'tcx> { self.threads .iter() .all(|thread| thread.state != ThreadState::BlockedOnJoin(joined_thread_id)), - "a joinable thread already has threads waiting for its termination" + "this thread already has threads waiting for its termination" ); self.join_thread(joined_thread_id, data_race) @@ -803,12 +806,14 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx } #[inline] - fn set_thread_name_wide(&mut self, thread: ThreadId, new_thread_name: Vec) { + fn set_thread_name_wide(&mut self, thread: ThreadId, new_thread_name: &[u16]) { let this = self.eval_context_mut(); - this.machine.threads.set_thread_name( - thread, - new_thread_name.into_iter().flat_map(u16::to_ne_bytes).collect(), - ); + + // 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] diff --git a/tests/fail/concurrency/libc_pthread_join_detached.stderr b/tests/fail/concurrency/libc_pthread_join_detached.stderr index e381a71b2520..92b693c0fd6b 100644 --- a/tests/fail/concurrency/libc_pthread_join_detached.stderr +++ b/tests/fail/concurrency/libc_pthread_join_detached.stderr @@ -1,8 +1,8 @@ -error: Undefined Behavior: trying to join a detached or already joined thread +error: Undefined Behavior: trying to join a detached thread --> $DIR/libc_pthread_join_detached.rs:LL:CC | -LL | ... assert_eq!(libc::pthread_join(native, ptr::null_mut()), 0); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ trying to join a detached thread +LL | assert_eq!(libc::pthread_join(native, ptr::null_mut()), 0); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ trying to join a detached thread | = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information diff --git a/tests/fail/concurrency/libc_pthread_join_joined.rs b/tests/fail/concurrency/libc_pthread_join_joined.rs index ebd1710bbf22..04ca4bbb3f61 100644 --- a/tests/fail/concurrency/libc_pthread_join_joined.rs +++ b/tests/fail/concurrency/libc_pthread_join_joined.rs @@ -15,6 +15,6 @@ fn main() { // assert_eq!(libc::pthread_attr_init(&mut attr), 0); FIXME: this function is not yet implemented. assert_eq!(libc::pthread_create(&mut native, &attr, thread_start, ptr::null_mut()), 0); assert_eq!(libc::pthread_join(native, ptr::null_mut()), 0); - assert_eq!(libc::pthread_join(native, ptr::null_mut()), 0); //~ ERROR: Undefined Behavior: trying to join a detached or already joined thread + assert_eq!(libc::pthread_join(native, ptr::null_mut()), 0); //~ ERROR: Undefined Behavior: trying to join an already joined thread } } diff --git a/tests/fail/concurrency/libc_pthread_join_joined.stderr b/tests/fail/concurrency/libc_pthread_join_joined.stderr index 5ac35ffe512e..f11b94cde8ee 100644 --- a/tests/fail/concurrency/libc_pthread_join_joined.stderr +++ b/tests/fail/concurrency/libc_pthread_join_joined.stderr @@ -1,8 +1,8 @@ -error: Undefined Behavior: trying to join a detached or already joined thread +error: Undefined Behavior: trying to join an already joined thread --> $DIR/libc_pthread_join_joined.rs:LL:CC | -LL | ... assert_eq!(libc::pthread_join(native, ptr::null_mut()), 0); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ trying to join a detached or already joined thread +LL | assert_eq!(libc::pthread_join(native, ptr::null_mut()), 0); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ trying to join an already joined thread | = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information diff --git a/tests/fail/concurrency/libc_pthread_join_main.rs b/tests/fail/concurrency/libc_pthread_join_main.rs index df6b520431b6..757651821637 100644 --- a/tests/fail/concurrency/libc_pthread_join_main.rs +++ b/tests/fail/concurrency/libc_pthread_join_main.rs @@ -8,7 +8,7 @@ fn main() { let thread_id: libc::pthread_t = unsafe { libc::pthread_self() }; let handle = thread::spawn(move || { unsafe { - assert_eq!(libc::pthread_join(thread_id, ptr::null_mut()), 0); //~ ERROR: Undefined Behavior: trying to join a detached or already joined thread + assert_eq!(libc::pthread_join(thread_id, ptr::null_mut()), 0); //~ ERROR: Undefined Behavior: trying to join a detached thread } }); thread::yield_now(); diff --git a/tests/fail/concurrency/libc_pthread_join_main.stderr b/tests/fail/concurrency/libc_pthread_join_main.stderr index fe136549ce51..c162f37b309f 100644 --- a/tests/fail/concurrency/libc_pthread_join_main.stderr +++ b/tests/fail/concurrency/libc_pthread_join_main.stderr @@ -1,8 +1,8 @@ -error: Undefined Behavior: trying to join a detached or already joined thread +error: Undefined Behavior: trying to join a detached thread --> $DIR/libc_pthread_join_main.rs:LL:CC | -LL | ... assert_eq!(libc::pthread_join(thread_id, ptr::null_mut()), 0); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ trying to join a detached or already joined thread +LL | assert_eq!(libc::pthread_join(thread_id, ptr::null_mut()), 0); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ trying to join a detached thread | = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information diff --git a/tests/fail/concurrency/libc_pthread_join_multiple.rs b/tests/fail/concurrency/libc_pthread_join_multiple.rs index e5187093befd..966f416eeac7 100644 --- a/tests/fail/concurrency/libc_pthread_join_multiple.rs +++ b/tests/fail/concurrency/libc_pthread_join_multiple.rs @@ -21,7 +21,7 @@ fn main() { let mut native_copy: libc::pthread_t = mem::zeroed(); ptr::copy_nonoverlapping(&native, &mut native_copy, 1); let handle = thread::spawn(move || { - assert_eq!(libc::pthread_join(native_copy, ptr::null_mut()), 0); //~ ERROR: Undefined Behavior: trying to join a detached or already joined thread + assert_eq!(libc::pthread_join(native_copy, ptr::null_mut()), 0); //~ ERROR: Undefined Behavior: trying to join an already joined thread }); assert_eq!(libc::pthread_join(native, ptr::null_mut()), 0); handle.join().unwrap(); diff --git a/tests/fail/concurrency/libc_pthread_join_multiple.stderr b/tests/fail/concurrency/libc_pthread_join_multiple.stderr index 9b91e5a3d0ed..c0c73086f14d 100644 --- a/tests/fail/concurrency/libc_pthread_join_multiple.stderr +++ b/tests/fail/concurrency/libc_pthread_join_multiple.stderr @@ -1,8 +1,8 @@ -error: Undefined Behavior: trying to join a detached or already joined thread +error: Undefined Behavior: trying to join an already joined thread --> $DIR/libc_pthread_join_multiple.rs:LL:CC | LL | ... assert_eq!(libc::pthread_join(native_copy, ptr::null_mut()), 0); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ trying to join a detached or already joined thread + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ trying to join an already joined thread | = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information diff --git a/tests/fail/concurrency/windows_join_main.rs b/tests/fail/concurrency/windows_join_main.rs index d3b54cdf156f..cde6d19ef25b 100644 --- a/tests/fail/concurrency/windows_join_main.rs +++ b/tests/fail/concurrency/windows_join_main.rs @@ -12,7 +12,7 @@ extern "system" { const INFINITE: u32 = u32::MAX; -// This is how miri represents the handle for thread 0. +// XXX HACK: This is how miri represents the handle for thread 0. // This value can be "legitimately" obtained by using `GetCurrentThread` with `DuplicateHandle` // but miri does not implement `DuplicateHandle` yet. const MAIN_THREAD: isize = (2i32 << 30) as isize; diff --git a/tests/pass/concurrency/tls_lib_drop.rs b/tests/pass/concurrency/tls_lib_drop.rs index 8ce011ede34e..3fd6e2d6f242 100644 --- a/tests/pass/concurrency/tls_lib_drop.rs +++ b/tests/pass/concurrency/tls_lib_drop.rs @@ -1,3 +1,5 @@ +//@ignore-target-windows: TLS destructor order is different on Windows. + use std::cell::RefCell; use std::thread; diff --git a/tests/pass/concurrency/tls_lib_drop_windows.rs b/tests/pass/concurrency/tls_lib_drop_windows.rs new file mode 100644 index 000000000000..e8c6538e701d --- /dev/null +++ b/tests/pass/concurrency/tls_lib_drop_windows.rs @@ -0,0 +1,191 @@ +//@only-target-windows: TLS destructor order is different on Windows. + +use std::cell::RefCell; +use std::thread; + +struct TestCell { + value: RefCell, +} + +impl Drop for TestCell { + fn drop(&mut self) { + for _ in 0..10 { + thread::yield_now(); + } + println!("Dropping: {} (should be before 'Continue main 1').", *self.value.borrow()) + } +} + +thread_local! { + static A: TestCell = TestCell { value: RefCell::new(0) }; + static A_CONST: TestCell = const { TestCell { value: RefCell::new(10) } }; +} + +/// Check that destructors of the library thread locals are executed immediately +/// after a thread terminates. +fn check_destructors() { + thread::spawn(|| { + A.with(|f| { + assert_eq!(*f.value.borrow(), 0); + *f.value.borrow_mut() = 5; + }); + A_CONST.with(|f| { + assert_eq!(*f.value.borrow(), 10); + *f.value.borrow_mut() = 15; + }); + }) + .join() + .unwrap(); + println!("Continue main 1.") +} + +struct JoinCell { + value: RefCell>>, +} + +impl Drop for JoinCell { + fn drop(&mut self) { + for _ in 0..10 { + thread::yield_now(); + } + let join_handle = self.value.borrow_mut().take().unwrap(); + println!("Joining: {} (should be before 'Continue main 2').", join_handle.join().unwrap()); + } +} + +thread_local! { + static B: JoinCell = JoinCell { value: RefCell::new(None) }; +} + +/// Check that the destructor can be blocked joining another thread. +fn check_blocking() { + thread::spawn(|| { + B.with(|f| { + assert!(f.value.borrow().is_none()); + let handle = thread::spawn(|| 7); + *f.value.borrow_mut() = Some(handle); + }); + }) + .join() + .unwrap(); + println!("Continue main 2."); + // Preempt the main thread so that the destructor gets executed and can join + // the thread. + thread::yield_now(); + thread::yield_now(); +} + +// This test tests that TLS destructors have run before the thread joins. The +// test has no false positives (meaning: if the test fails, there's actually +// an ordering problem). It may have false negatives, where the test passes but +// join is not guaranteed to be after the TLS destructors. However, false +// negatives should be exceedingly rare due to judicious use of +// thread::yield_now and running the test several times. +fn join_orders_after_tls_destructors() { + use std::sync::atomic::{AtomicU8, Ordering}; + + // We emulate a synchronous MPSC rendezvous channel using only atomics and + // thread::yield_now. We can't use std::mpsc as the implementation itself + // may rely on thread locals. + // + // The basic state machine for an SPSC rendezvous channel is: + // FRESH -> THREAD1_WAITING -> MAIN_THREAD_RENDEZVOUS + // where the first transition is done by the “receiving” thread and the 2nd + // transition is done by the “sending” thread. + // + // We add an additional state `THREAD2_LAUNCHED` between `FRESH` and + // `THREAD1_WAITING` to block until all threads are actually running. + // + // A thread that joins on the “receiving” thread completion should never + // observe the channel in the `THREAD1_WAITING` state. If this does occur, + // we switch to the “poison” state `THREAD2_JOINED` and panic all around. + // (This is equivalent to “sending” from an alternate producer thread.) + const FRESH: u8 = 0; + const THREAD2_LAUNCHED: u8 = 1; + const THREAD1_WAITING: u8 = 2; + const MAIN_THREAD_RENDEZVOUS: u8 = 3; + const THREAD2_JOINED: u8 = 4; + static SYNC_STATE: AtomicU8 = AtomicU8::new(FRESH); + + for _ in 0..10 { + SYNC_STATE.store(FRESH, Ordering::SeqCst); + + let jh = thread::Builder::new() + .name("thread1".into()) + .spawn(move || { + struct TlDrop; + + impl Drop for TlDrop { + fn drop(&mut self) { + let mut sync_state = SYNC_STATE.swap(THREAD1_WAITING, Ordering::SeqCst); + loop { + match sync_state { + THREAD2_LAUNCHED | THREAD1_WAITING => thread::yield_now(), + MAIN_THREAD_RENDEZVOUS => break, + THREAD2_JOINED => + panic!( + "Thread 1 still running after thread 2 joined on thread 1" + ), + v => unreachable!("sync state: {}", v), + } + sync_state = SYNC_STATE.load(Ordering::SeqCst); + } + } + } + + thread_local! { + static TL_DROP: TlDrop = TlDrop; + } + + TL_DROP.with(|_| {}); + + loop { + match SYNC_STATE.load(Ordering::SeqCst) { + FRESH => thread::yield_now(), + THREAD2_LAUNCHED => break, + v => unreachable!("sync state: {}", v), + } + } + }) + .unwrap(); + + let jh2 = thread::Builder::new() + .name("thread2".into()) + .spawn(move || { + assert_eq!(SYNC_STATE.swap(THREAD2_LAUNCHED, Ordering::SeqCst), FRESH); + jh.join().unwrap(); + match SYNC_STATE.swap(THREAD2_JOINED, Ordering::SeqCst) { + MAIN_THREAD_RENDEZVOUS => return, + THREAD2_LAUNCHED | THREAD1_WAITING => { + panic!("Thread 2 running after thread 1 join before main thread rendezvous") + } + v => unreachable!("sync state: {:?}", v), + } + }) + .unwrap(); + + loop { + match SYNC_STATE.compare_exchange( + THREAD1_WAITING, + MAIN_THREAD_RENDEZVOUS, + Ordering::SeqCst, + Ordering::SeqCst, + ) { + Ok(_) => break, + Err(FRESH) => thread::yield_now(), + Err(THREAD2_LAUNCHED) => thread::yield_now(), + Err(THREAD2_JOINED) => { + panic!("Main thread rendezvous after thread 2 joined thread 1") + } + v => unreachable!("sync state: {:?}", v), + } + } + jh2.join().unwrap(); + } +} + +fn main() { + check_destructors(); + check_blocking(); + join_orders_after_tls_destructors(); +} diff --git a/tests/pass/concurrency/tls_lib_drop_windows.stdout b/tests/pass/concurrency/tls_lib_drop_windows.stdout new file mode 100644 index 000000000000..e5b8efcaf5fa --- /dev/null +++ b/tests/pass/concurrency/tls_lib_drop_windows.stdout @@ -0,0 +1,5 @@ +Dropping: 15 (should be before 'Continue main 1'). +Dropping: 5 (should be before 'Continue main 1'). +Continue main 1. +Joining: 7 (should be before 'Continue main 2'). +Continue main 2.