small changes from review

Author: beepster4096

src/shims/tls.rs

@@ -244,11 +244,12 @@ 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 treats both the same.
+        // 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)`.
-        // `h` should be a handle to the current module and what `pv` should be is unknown
+        // 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,

src/shims/unix/thread.rs

@@ -52,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/foreign_items.rs

@@ -355,9 +355,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)?;

src/shims/windows/handle.rs

@@ -157,7 +157,8 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
         let handle = this.read_scalar(handle_op)?.check_init()?;
 
         match Handle::from_scalar(handle, this)? {
-            Some(Handle::Thread(thread)) => this.detach_thread(thread, true)?,
+            Some(Handle::Thread(thread)) =>
+                this.detach_thread(thread, /*allow_terminated_joined*/ true)?,
             _ => this.invalid_handle("CloseHandle")?,
         }
 

src/shims/windows/thread.rs

@@ -22,7 +22,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
         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)?;
 
@@ -40,6 +40,8 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
         let stack_size_param_is_a_reservation =
             this.eval_windows("c", "STACK_SIZE_PARAM_IS_A_RESERVATION")?.to_u32()?;
 
+        // we ignore the `STACK_SIZE_PARAM_IS_A_RESERVATION` flag
+        // reserved stack memory cannot be observed by the program
         if flags != 0 && flags != stack_size_param_is_a_reservation {
             throw_unsup_format!("unsupported `dwCreationFlags` {} in `CreateThread`", flags)
         }
@@ -61,7 +63,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
         &mut self,
         handle_op: &OpTy<'tcx, Provenance>,
         timeout_op: &OpTy<'tcx, Provenance>,
-    ) -> InterpResult<'tcx> {
+    ) -> InterpResult<'tcx, u32> {
         let this = self.eval_context_mut();
 
         let handle = this.read_scalar(handle_op)?.check_init()?;
@@ -70,8 +72,8 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
 
         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")?,
         };
@@ -82,6 +84,6 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
 
         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)

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;