fix various issues

Author: beepster4096

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<u16>) -> InterpResult<'tcx, OsString> {
+        pub fn u16vec_to_osstring<'tcx>(u16_vec: Vec<u16>) -> InterpResult<'tcx, OsString> {
             Ok(OsString::from_wide(&u16_vec[..]))
         }
         #[cfg(not(windows))]

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 },

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)
     }

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)?;
             }

src/shims/windows/foreign_items.rs

@@ -323,22 +323,14 @@ 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)?;
                 let which = this.read_scalar(which)?.to_i32()?;
                 // 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),
         }

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<Provenance> {
         // 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<Self>> {
         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")?,
         }
 

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)
     }
 }

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 <https://docs.microsoft.com/en-us/windows/win32/procthread/thread-handles-and-identifiers>:
+    /// > 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<u16>) {
+    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]

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

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
     }
 }