Auto merge of #2231 - DrMeepster:winfred, r=RalfJung

Windows thread support: Part 1

This PR adds support for threads on Windows.

Author: bors

src/lib.rs

@@ -5,6 +5,8 @@
 #![feature(try_blocks)]
 #![feature(let_else)]
 #![feature(io_error_more)]
+#![feature(int_log)]
+#![feature(variant_count)]
 #![feature(yeet_expr)]
 #![feature(is_some_with)]
 #![feature(nonzero_ops)]

src/machine.rs

@@ -421,6 +421,7 @@ impl<'mir, 'tcx> Evaluator<'mir, 'tcx> {
     ) -> InterpResult<'tcx> {
         EnvVars::init(this, config)?;
         Evaluator::init_extern_statics(this)?;
+        ThreadManager::init(this);
         Ok(())
     }
 

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/time.rs

@@ -197,12 +197,11 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
     fn nanosleep(
         &mut self,
         req_op: &OpTy<'tcx, Provenance>,
-        _rem: &OpTy<'tcx, Provenance>,
+        _rem: &OpTy<'tcx, Provenance>, // Signal handlers are not supported, so rem will never be written to.
     ) -> InterpResult<'tcx, i32> {
-        // Signal handlers are not supported, so rem will never be written to.
-
         let this = self.eval_context_mut();
 
+        this.assert_target_os_is_unix("nanosleep");
         this.check_no_isolation("`nanosleep`")?;
 
         let duration = match this.read_timespec(&this.deref_operand(req_op)?)? {
@@ -233,4 +232,31 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
 
         Ok(0)
     }
+
+    #[allow(non_snake_case)]
+    fn Sleep(&mut self, timeout: &OpTy<'tcx, Provenance>) -> InterpResult<'tcx> {
+        let this = self.eval_context_mut();
+
+        this.assert_target_os("windows", "Sleep");
+        this.check_no_isolation("`Sleep`")?;
+
+        let timeout_ms = this.read_scalar(timeout)?.to_u32()?;
+
+        let duration = Duration::from_millis(timeout_ms.into());
+        let timeout_time = Time::Monotonic(Instant::now().checked_add(duration).unwrap());
+
+        let active_thread = this.get_active_thread();
+        this.block_thread(active_thread);
+
+        this.register_timeout_callback(
+            active_thread,
+            timeout_time,
+            Box::new(move |ecx| {
+                ecx.unblock_thread(active_thread);
+                Ok(())
+            }),
+        );
+
+        Ok(())
+    }
 }

src/shims/tls.rs

@@ -229,25 +229,28 @@ impl<'tcx> TlsData<'tcx> {
 
 impl<'mir, 'tcx: 'mir> EvalContextPrivExt<'mir, 'tcx> for crate::MiriEvalContext<'mir, 'tcx> {}
 trait EvalContextPrivExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx> {
-    /// Schedule TLS destructors for the main thread on Windows. The
-    /// implementation assumes that we do not support concurrency on Windows
-    /// yet.
+    /// Schedule TLS destructors for Windows.
+    /// On windows, TLS destructors are managed by std.
     fn schedule_windows_tls_dtors(&mut self) -> InterpResult<'tcx> {
         let this = self.eval_context_mut();
         let active_thread = this.get_active_thread();
-        assert_eq!(this.get_total_thread_count(), 1, "concurrency on Windows is not supported");
+
         // Windows has a special magic linker section that is run on certain events.
         // Instead of searching for that section and supporting arbitrary hooks in there
         // (that would be basically https://github.com/rust-lang/miri/issues/450),
         // we specifically look up the static in libstd that we know is placed
         // in that section.
-        let thread_callback = this
-            .eval_path_scalar(&["std", "sys", "windows", "thread_local_key", "p_thread_callback"])?
-            .to_pointer(this)?;
+        let thread_callback =
+            this.eval_windows("thread_local_key", "p_thread_callback")?.to_pointer(this)?;
         let thread_callback = this.get_ptr_fn(thread_callback)?.as_instance()?;
 
+        // 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)`.
-        let reason = this.eval_path_scalar(&["std", "sys", "windows", "c", "DLL_THREAD_DETACH"])?;
+        // 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,47 +13,20 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
     ) -> InterpResult<'tcx, i32> {
         let this = self.eval_context_mut();
 
-        // Create the new thread
-        let new_thread_id = this.create_thread();
-
-        // Write the current thread-id, switch to the next thread later
-        // to treat this write operation as occuring on the current thread.
         let thread_info_place = this.deref_operand(thread)?;
-        this.write_scalar(
-            Scalar::from_uint(new_thread_id.to_u32(), thread_info_place.layout.size),
-            &thread_info_place.into(),
-        )?;
-
-        // Read the function argument that will be sent to the new thread
-        // before the thread starts executing since reading after the
-        // context switch will incorrectly report a data-race.
-        let fn_ptr = this.read_pointer(start_routine)?;
-        let func_arg = this.read_immediate(arg)?;
 
-        // Finally switch to new thread so that we can push the first stackframe.
-        // After this all accesses will be treated as occuring in the new thread.
-        let old_thread_id = this.set_active_thread(new_thread_id);
+        let start_routine = this.read_pointer(start_routine)?;
 
-        // Perform the function pointer load in the new thread frame.
-        let instance = this.get_ptr_fn(fn_ptr)?.as_instance()?;
-
-        // Note: the returned value is currently ignored (see the FIXME in
-        // pthread_join below) because the Rust standard library does not use
-        // it.
-        let ret_place =
-            this.allocate(this.layout_of(this.tcx.types.usize)?, MiriMemoryKind::Machine.into())?;
+        let func_arg = this.read_immediate(arg)?;
 
-        this.call_function(
-            instance,
+        this.start_thread(
+            Some(thread_info_place),
+            start_routine,
             Abi::C { unwind: false },
-            &[*func_arg],
-            Some(&ret_place.into()),
-            StackPopCleanup::Root { cleanup: true },
+            func_arg,
+            this.layout_of(this.tcx.types.usize)?,
         )?;
 
-        // Restore the old active thread frame.
-        this.set_active_thread(old_thread_id);
-
         Ok(0)
     }
 
@@ -70,7 +43,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
         }
 
         let thread_id = this.read_scalar(thread)?.to_machine_usize(this)?;
-        this.join_thread(thread_id.try_into().expect("thread ID should fit in u32"))?;
+        this.join_thread_exclusive(thread_id.try_into().expect("thread ID should fit in u32"))?;
 
         Ok(0)
     }
@@ -79,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"))?;
+        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

@@ -5,11 +5,13 @@ use rustc_target::spec::abi::Abi;
 use log::trace;
 
 use crate::helpers::check_arg_count;
+use crate::shims::windows::handle::{EvalContextExt as _, Handle, PseudoHandle};
 use crate::*;
 
 #[derive(Debug, Copy, Clone)]
 pub enum Dlsym {
     NtWriteFile,
+    SetThreadDescription,
 }
 
 impl Dlsym {
@@ -18,8 +20,8 @@ impl Dlsym {
     pub fn from_str<'tcx>(name: &str) -> InterpResult<'tcx, Option<Dlsym>> {
         Ok(match name {
             "GetSystemTimePreciseAsFileTime" => None,
-            "SetThreadDescription" => None,
             "NtWriteFile" => Some(Dlsym::NtWriteFile),
+            "SetThreadDescription" => Some(Dlsym::SetThreadDescription),
             _ => throw_unsup_format!("unsupported Windows dlsym: {}", name),
         })
     }
@@ -107,6 +109,23 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
                     dest,
                 )?;
             }
+            Dlsym::SetThreadDescription => {
+                let [handle, name] = check_arg_count(args)?;
+
+                let handle = this.read_scalar(handle)?.check_init()?;
+
+                let name = this.read_wide_str(this.read_pointer(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")?,
+                };
+
+                this.set_thread_name_wide(thread, &name);
+
+                this.write_null(dest)?;
+            }
         }
 
         trace!("{:?}", this.dump_place(**dest));

src/shims/windows/foreign_items.rs

@@ -6,7 +6,10 @@ use rustc_target::spec::abi::Abi;
 
 use crate::*;
 use shims::foreign_items::EmulateByNameResult;
+use shims::windows::handle::{EvalContextExt as _, Handle, PseudoHandle};
 use shims::windows::sync::EvalContextExt as _;
+use shims::windows::thread::EvalContextExt as _;
+
 use smallvec::SmallVec;
 
 impl<'mir, 'tcx: 'mir> EvalContextExt<'mir, 'tcx> for crate::MiriEvalContext<'mir, 'tcx> {}
@@ -219,6 +222,12 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
                 let result = this.QueryPerformanceFrequency(lpFrequency)?;
                 this.write_scalar(Scalar::from_i32(result), dest)?;
             }
+            "Sleep" => {
+                let [timeout] =
+                    this.check_shim(abi, Abi::System { unwind: false }, link_name, args)?;
+
+                this.Sleep(timeout)?;
+            }
 
             // Synchronization primitives
             "AcquireSRWLockExclusive" => {
@@ -314,36 +323,57 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
                 // FIXME: we should set last_error, but to what?
                 this.write_null(dest)?;
             }
-            "SwitchToThread" => {
-                let [] = this.check_shim(abi, Abi::System { unwind: false }, link_name, args)?;
-                // Note that once Miri supports concurrency, this will need to 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" => {
+                let [handle] =
+                    this.check_shim(abi, Abi::System { unwind: false }, link_name, args)?;
+
+                this.CloseHandle(handle)?;
+
+                this.write_scalar(Scalar::from_u32(1), dest)?;
+            }
 
-            // Better error for attempts to create a thread
+            // Threading
             "CreateThread" => {
-                let [_, _, _, _, _, _] =
+                let [security, stacksize, start, arg, flags, thread] =
                     this.check_shim(abi, Abi::System { unwind: false }, link_name, args)?;
 
-                this.handle_unsupported("can't create threads on Windows")?;
-                return Ok(EmulateByNameResult::AlreadyJumped);
+                let thread_id =
+                    this.CreateThread(security, stacksize, start, arg, flags, thread)?;
+
+                this.write_scalar(Handle::Thread(thread_id).to_scalar(this), dest)?;
+            }
+            "WaitForSingleObject" => {
+                let [handle, timeout] =
+                    this.check_shim(abi, Abi::System { unwind: false }, link_name, args)?;
+
+                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)?;
+
+                this.write_scalar(
+                    Handle::Pseudo(PseudoHandle::CurrentThread).to_scalar(this),
+                    dest,
+                )?;
             }
 
             // Incomplete shims that we "stub out" just to get pre-main initialization code to work.
             // These shims are enabled only when the caller is in the standard library.
             "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() => {
@@ -374,45 +404,20 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
                 // Any non zero value works for the stdlib. This is just used for stack overflows anyway.
                 this.write_scalar(Scalar::from_u32(1), dest)?;
             }
-            | "InitializeCriticalSection"
-            | "EnterCriticalSection"
-            | "LeaveCriticalSection"
-            | "DeleteCriticalSection"
-                if this.frame_in_std() =>
-            {
-                #[allow(non_snake_case)]
-                let [_lpCriticalSection] =
-                    this.check_shim(abi, Abi::System { unwind: false }, link_name, args)?;
-                assert_eq!(
-                    this.get_total_thread_count(),
-                    1,
-                    "concurrency on Windows is not supported"
-                );
-                // Nothing to do, not even a return value.
-                // (Windows locks are reentrant, and we have only 1 thread,
-                // so not doing any futher checks here is at least not incorrect.)
-            }
-            "TryEnterCriticalSection" if this.frame_in_std() => {
-                #[allow(non_snake_case)]
-                let [_lpCriticalSection] =
-                    this.check_shim(abi, Abi::System { unwind: false }, link_name, args)?;
-                assert_eq!(
-                    this.get_total_thread_count(),
-                    1,
-                    "concurrency on Windows is not supported"
-                );
-                // There is only one thread, so this always succeeds and returns TRUE.
-                this.write_scalar(Scalar::from_i32(1), dest)?;
-            }
-            "GetCurrentThread" if this.frame_in_std() => {
-                let [] = this.check_shim(abi, Abi::System { unwind: false }, link_name, args)?;
-                this.write_scalar(Scalar::from_machine_isize(1, this), dest)?;
-            }
             "GetCurrentProcessId" if this.frame_in_std() => {
                 let [] = this.check_shim(abi, Abi::System { unwind: false }, link_name, args)?;
                 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),
         }