Auto merge of #3345 - RalfJung:win-get-thread-name, r=RalfJung

Windows: support getting the thread name

Also organize the thread name tests a bit.

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

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

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)"),

src/tools/miri/src/shims/windows/foreign_items.rs

@@ -5,14 +5,19 @@ 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};
 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)?;
             }

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

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");
+}

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();
+}

src/tools/miri/tests/pass/panic/thread_panic.stderr

@@ -0,0 +1,5 @@
+thread '<unnamed>' 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!

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() {

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: AsRef<OsStr>>(s: S) -> Vec<u16> {
+    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::<u16>();
+        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());
+    }
+}