From 3986d06542c59e4f826086d6afdd4ce64ab1cd7a Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Wed, 26 Oct 2022 14:01:11 +0200 Subject: [PATCH 01/12] test on windows-gnu target --- src/tools/miri/ci.sh | 1 + 1 file changed, 1 insertion(+) diff --git a/src/tools/miri/ci.sh b/src/tools/miri/ci.sh index aa322e54a31da..72b7b791a47e0 100755 --- a/src/tools/miri/ci.sh +++ b/src/tools/miri/ci.sh @@ -93,6 +93,7 @@ case $HOST_TARGET in ;; i686-pc-windows-msvc) MIRI_TEST_TARGET=x86_64-unknown-linux-gnu run_tests + MIRI_TEST_TARGET=x86_64-pc-windows-gnu run_tests ;; *) echo "FATAL: unknown OS" From 27e5cc8898b3de022c0912f20a0b1bed7db53a89 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Wed, 26 Oct 2022 14:05:52 +0200 Subject: [PATCH 02/12] simplify GHA --- src/tools/miri/.github/workflows/ci.yml | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/tools/miri/.github/workflows/ci.yml b/src/tools/miri/.github/workflows/ci.yml index 659c51f77e6ab..78e0865f1c020 100644 --- a/src/tools/miri/.github/workflows/ci.yml +++ b/src/tools/miri/.github/workflows/ci.yml @@ -24,16 +24,12 @@ jobs: strategy: fail-fast: false matrix: - build: [linux64, macos, win32] include: - - build: linux64 - os: ubuntu-latest + - os: ubuntu-latest host_target: x86_64-unknown-linux-gnu - - build: macos - os: macos-latest + - os: macos-latest host_target: x86_64-apple-darwin - - build: win32 - os: windows-latest + - os: windows-latest host_target: i686-pc-windows-msvc steps: - uses: actions/checkout@v3 From 50af895ef712a03e1bb1ea000128434937cadd15 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Thu, 27 Oct 2022 09:36:40 +0200 Subject: [PATCH 03/12] change cronjob time --- src/tools/miri/.github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tools/miri/.github/workflows/ci.yml b/src/tools/miri/.github/workflows/ci.yml index 78e0865f1c020..3efb2d733d426 100644 --- a/src/tools/miri/.github/workflows/ci.yml +++ b/src/tools/miri/.github/workflows/ci.yml @@ -10,7 +10,7 @@ on: branches: - 'master' schedule: - - cron: '5 15 * * *' # At 15:05 UTC every day. + - cron: '6 6 * * *' # At 6:06 UTC every day. env: CARGO_UNSTABLE_SPARSE_REGISTRY: 'true' From 7663305dff3b6634de90a9f3bc726e976357dc48 Mon Sep 17 00:00:00 2001 From: Maybe Waffle Date: Thu, 27 Oct 2022 22:23:58 +0400 Subject: [PATCH 04/12] Implement `ptr_mask` intrinsic --- src/tools/miri/src/shims/intrinsics/mod.rs | 13 ++++++++++++- src/tools/miri/tests/pass/shims/ptr_mask.rs | 18 ++++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) create mode 100644 src/tools/miri/tests/pass/shims/ptr_mask.rs diff --git a/src/tools/miri/src/shims/intrinsics/mod.rs b/src/tools/miri/src/shims/intrinsics/mod.rs index e0985ace5be7d..6004e2078ad4f 100644 --- a/src/tools/miri/src/shims/intrinsics/mod.rs +++ b/src/tools/miri/src/shims/intrinsics/mod.rs @@ -11,7 +11,7 @@ use rustc_middle::{ mir, ty::{self, FloatTy, Ty}, }; -use rustc_target::abi::Integer; +use rustc_target::abi::{Integer, Size}; use crate::*; use atomic::EvalContextExt as _; @@ -120,6 +120,17 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { this.write_bytes_ptr(ptr, iter::repeat(val_byte).take(byte_count.bytes_usize()))?; } + "ptr_mask" => { + let [ptr, mask] = check_arg_count(args)?; + + let ptr = this.read_pointer(ptr)?; + let mask = this.read_scalar(mask)?.to_machine_usize(this)?; + + let masked_addr = Size::from_bytes(ptr.addr().bytes() & mask); + + this.write_pointer(Pointer::new(ptr.provenance, masked_addr), dest)?; + } + // Floating-point operations "fabsf32" => { let [f] = check_arg_count(args)?; diff --git a/src/tools/miri/tests/pass/shims/ptr_mask.rs b/src/tools/miri/tests/pass/shims/ptr_mask.rs new file mode 100644 index 0000000000000..fb8bb6b13dbc2 --- /dev/null +++ b/src/tools/miri/tests/pass/shims/ptr_mask.rs @@ -0,0 +1,18 @@ +#![feature(ptr_mask)] +#![feature(strict_provenance)] + +fn main() { + let v: u32 = 0xABCDABCD; + let ptr: *const u32 = &v; + + // u32 is 4 aligned, + // so the lower `log2(4) = 2` bits of the address are always 0 + assert_eq!(ptr.addr() & 0b11, 0); + + let tagged_ptr = ptr.map_addr(|a| a | 0b11); + let tag = tagged_ptr.addr() & 0b11; + let masked_ptr = tagged_ptr.mask(!0b11); + + assert_eq!(tag, 0b11); + assert_eq!(unsafe { *masked_ptr }, 0xABCDABCD); +} From 6e3b0df8b8f21c3b833c386c7b8c6554f0be2b94 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 28 Oct 2022 10:31:43 +0200 Subject: [PATCH 05/12] pthread_setname_np returns an int on macOS --- .../miri/src/shims/unix/macos/foreign_items.rs | 5 ++++- .../miri/tests/pass-dep/shims/pthreads.rs | 18 +++++++++++++++--- 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/src/tools/miri/src/shims/unix/macos/foreign_items.rs b/src/tools/miri/src/shims/unix/macos/foreign_items.rs index 371f56ca35550..221dc39697f90 100644 --- a/src/tools/miri/src/shims/unix/macos/foreign_items.rs +++ b/src/tools/miri/src/shims/unix/macos/foreign_items.rs @@ -177,11 +177,14 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { let [name] = this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?; let thread = this.pthread_self()?; let max_len = this.eval_libc("MAXTHREADNAMESIZE")?.to_machine_usize(this)?; - this.pthread_setname_np( + let res = this.pthread_setname_np( thread, this.read_scalar(name)?, max_len.try_into().unwrap(), )?; + // Contrary to the manpage, `pthread_setname_np` on macOS still + // returns an integer indicating success. + this.write_scalar(res, dest)?; } "pthread_getname_np" => { let [thread, name, len] = diff --git a/src/tools/miri/tests/pass-dep/shims/pthreads.rs b/src/tools/miri/tests/pass-dep/shims/pthreads.rs index bbddca74754c5..ae02a8e3c980a 100644 --- a/src/tools/miri/tests/pass-dep/shims/pthreads.rs +++ b/src/tools/miri/tests/pass-dep/shims/pthreads.rs @@ -1,6 +1,6 @@ //@ignore-target-windows: No libc on Windows #![feature(cstr_from_bytes_until_nul)] -use std::ffi::CStr; +use std::ffi::{CStr, CString}; use std::thread; fn main() { @@ -135,6 +135,13 @@ fn test_named_thread_truncation() { .chain(std::iter::repeat(" yada").take(100)) .collect::(); + fn set_thread_name(name: &CStr) -> i32 { + #[cfg(target_os = "linux")] + return unsafe { libc::pthread_setname_np(libc::pthread_self(), name.as_ptr().cast()) }; + #[cfg(target_os = "macos")] + return unsafe { libc::pthread_setname_np(name.as_ptr().cast()) }; + } + let result = thread::Builder::new().name(long_name.clone()).spawn(move || { // Rust remembers the full thread name itself. assert_eq!(thread::current().name(), Some(long_name.as_str())); @@ -142,11 +149,16 @@ fn test_named_thread_truncation() { // But the system is limited -- make sure we successfully set a truncation. let mut buf = vec![0u8; long_name.len() + 1]; unsafe { - libc::pthread_getname_np(libc::pthread_self(), buf.as_mut_ptr().cast(), buf.len()); - } + libc::pthread_getname_np(libc::pthread_self(), buf.as_mut_ptr().cast(), buf.len()) + }; let cstr = CStr::from_bytes_until_nul(&buf).unwrap(); assert!(cstr.to_bytes().len() >= 15); // POSIX seems to promise at least 15 chars assert!(long_name.as_bytes().starts_with(cstr.to_bytes())); + + // Also test directly calling pthread_setname to check its return value. + assert_eq!(set_thread_name(&cstr), 0); + // But with a too long name it should fail. + assert_ne!(set_thread_name(&CString::new(long_name).unwrap()), 0); }); result.unwrap().join().unwrap(); } From 676e53f5b108c7c460925d3d96874657be249569 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 28 Oct 2022 11:42:42 +0200 Subject: [PATCH 06/12] update ignore-windows comments --- src/tools/miri/tests/fail/data_race/stack_pop_race.rs | 1 - src/tools/miri/tests/fail/panic/no_std.rs | 2 +- src/tools/miri/tests/pass/concurrency/sync.rs | 2 +- src/tools/miri/tests/pass/concurrency/sync_nopreempt.rs | 2 +- src/tools/miri/tests/pass/no_std.rs | 2 +- src/tools/miri/tests/pass/shims/env/current_exe.rs | 2 +- src/tools/miri/tests/pass/shims/sleep_long.rs | 1 - 7 files changed, 5 insertions(+), 7 deletions(-) diff --git a/src/tools/miri/tests/fail/data_race/stack_pop_race.rs b/src/tools/miri/tests/fail/data_race/stack_pop_race.rs index 8f371a680f11d..163f46eacc19f 100644 --- a/src/tools/miri/tests/fail/data_race/stack_pop_race.rs +++ b/src/tools/miri/tests/fail/data_race/stack_pop_race.rs @@ -1,4 +1,3 @@ -//@ignore-target-windows: Concurrency on Windows is not supported yet. //@compile-flags: -Zmiri-preemption-rate=0 use std::thread; diff --git a/src/tools/miri/tests/fail/panic/no_std.rs b/src/tools/miri/tests/fail/panic/no_std.rs index b6a5c0755701e..589f843cf8270 100644 --- a/src/tools/miri/tests/fail/panic/no_std.rs +++ b/src/tools/miri/tests/fail/panic/no_std.rs @@ -3,7 +3,7 @@ // windows tls dtors go through libstd right now, thus this test // cannot pass. When windows tls dtors go through the special magic // windows linker section, we can run this test on windows again. -//@ignore-target-windows +//@ignore-target-windows: no-std not supported on Windows // Plumbing to let us use `writeln!` to host stderr: diff --git a/src/tools/miri/tests/pass/concurrency/sync.rs b/src/tools/miri/tests/pass/concurrency/sync.rs index 8bda32bb95a48..aa1c4892d1867 100644 --- a/src/tools/miri/tests/pass/concurrency/sync.rs +++ b/src/tools/miri/tests/pass/concurrency/sync.rs @@ -1,4 +1,4 @@ -//@ignore-target-windows: Concurrency on Windows is not supported yet. +//@ignore-target-windows: Condvars on Windows are not supported yet. //@compile-flags: -Zmiri-disable-isolation -Zmiri-strict-provenance use std::sync::{Arc, Barrier, Condvar, Mutex, Once, RwLock}; diff --git a/src/tools/miri/tests/pass/concurrency/sync_nopreempt.rs b/src/tools/miri/tests/pass/concurrency/sync_nopreempt.rs index 3da33fee4c0ed..55206f4bfc526 100644 --- a/src/tools/miri/tests/pass/concurrency/sync_nopreempt.rs +++ b/src/tools/miri/tests/pass/concurrency/sync_nopreempt.rs @@ -1,4 +1,4 @@ -//@ignore-target-windows: Concurrency on Windows is not supported yet. +//@ignore-target-windows: Condvars on Windows are not supported yet. // We are making scheduler assumptions here. //@compile-flags: -Zmiri-strict-provenance -Zmiri-preemption-rate=0 diff --git a/src/tools/miri/tests/pass/no_std.rs b/src/tools/miri/tests/pass/no_std.rs index 0203edfe18180..eb0e860e68ebf 100644 --- a/src/tools/miri/tests/pass/no_std.rs +++ b/src/tools/miri/tests/pass/no_std.rs @@ -3,7 +3,7 @@ // windows tls dtors go through libstd right now, thus this test // cannot pass. When windows tls dtors go through the special magic // windows linker section, we can run this test on windows again. -//@ignore-target-windows +//@ignore-target-windows: no-std not supported on Windows // Plumbing to let us use `writeln!` to host stdout: diff --git a/src/tools/miri/tests/pass/shims/env/current_exe.rs b/src/tools/miri/tests/pass/shims/env/current_exe.rs index 15ea6a52b7b6b..3f1153d265d7e 100644 --- a/src/tools/miri/tests/pass/shims/env/current_exe.rs +++ b/src/tools/miri/tests/pass/shims/env/current_exe.rs @@ -1,4 +1,4 @@ -//@ignore-target-windows +//@ignore-target-windows: current_exe not supported on Windows //@only-on-host: the Linux std implementation opens /proc/self/exe, which doesn't work cross-target //@compile-flags: -Zmiri-disable-isolation use std::env; diff --git a/src/tools/miri/tests/pass/shims/sleep_long.rs b/src/tools/miri/tests/pass/shims/sleep_long.rs index dd4a1843942c4..c94f63a54274a 100644 --- a/src/tools/miri/tests/pass/shims/sleep_long.rs +++ b/src/tools/miri/tests/pass/shims/sleep_long.rs @@ -1,4 +1,3 @@ -//@ignore-target-windows: no threads nor sleep on Windows //@compile-flags: -Zmiri-ignore-leaks -Zmiri-disable-isolation use std::sync::{Arc, Mutex}; use std::thread; From a23d1fb1acf1079614ca581bc8d83174d0dd07c0 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 28 Oct 2022 15:48:07 +0200 Subject: [PATCH 07/12] implement thread parking on Windows --- src/tools/miri/src/lib.rs | 4 +- src/tools/miri/src/machine.rs | 30 +++++ src/tools/miri/src/shims/unix/linux/sync.rs | 3 +- src/tools/miri/src/shims/windows/dlsym.rs | 15 +++ src/tools/miri/src/shims/windows/sync.rs | 108 +++++++++++++++++- .../miri/tests/pass/concurrency/channels.rs | 1 - .../pass/concurrency/spin_loops_nopreempt.rs | 1 - .../miri/tests/pass/threadleak_ignored.rs | 1 - 8 files changed, 155 insertions(+), 8 deletions(-) diff --git a/src/tools/miri/src/lib.rs b/src/tools/miri/src/lib.rs index 21e65bb1b706e..f55c0b43e39bc 100644 --- a/src/tools/miri/src/lib.rs +++ b/src/tools/miri/src/lib.rs @@ -98,8 +98,8 @@ pub use crate::eval::{ pub use crate::helpers::{CurrentSpan, EvalContextExt as _}; pub use crate::intptrcast::ProvenanceMode; pub use crate::machine::{ - AllocExtra, FrameData, MiriInterpCx, MiriInterpCxExt, MiriMachine, MiriMemoryKind, Provenance, - ProvenanceExtra, PAGE_SIZE, STACK_ADDR, STACK_SIZE, + AllocExtra, FrameData, MiriInterpCx, MiriInterpCxExt, MiriMachine, MiriMemoryKind, + PrimitiveLayouts, Provenance, ProvenanceExtra, PAGE_SIZE, STACK_ADDR, STACK_SIZE, }; pub use crate::mono_hash_map::MonoHashMap; pub use crate::operator::EvalContextExt as _; diff --git a/src/tools/miri/src/machine.rs b/src/tools/miri/src/machine.rs index e014e2db1e1f2..231a99c1d034e 100644 --- a/src/tools/miri/src/machine.rs +++ b/src/tools/miri/src/machine.rs @@ -276,10 +276,14 @@ pub struct PrimitiveLayouts<'tcx> { pub i8: TyAndLayout<'tcx>, pub i16: TyAndLayout<'tcx>, pub i32: TyAndLayout<'tcx>, + pub i64: TyAndLayout<'tcx>, + pub i128: TyAndLayout<'tcx>, pub isize: TyAndLayout<'tcx>, pub u8: TyAndLayout<'tcx>, pub u16: TyAndLayout<'tcx>, pub u32: TyAndLayout<'tcx>, + pub u64: TyAndLayout<'tcx>, + pub u128: TyAndLayout<'tcx>, pub usize: TyAndLayout<'tcx>, pub bool: TyAndLayout<'tcx>, pub mut_raw_ptr: TyAndLayout<'tcx>, // *mut () @@ -296,16 +300,42 @@ impl<'mir, 'tcx: 'mir> PrimitiveLayouts<'tcx> { i8: layout_cx.layout_of(tcx.types.i8)?, i16: layout_cx.layout_of(tcx.types.i16)?, i32: layout_cx.layout_of(tcx.types.i32)?, + i64: layout_cx.layout_of(tcx.types.i64)?, + i128: layout_cx.layout_of(tcx.types.i128)?, isize: layout_cx.layout_of(tcx.types.isize)?, u8: layout_cx.layout_of(tcx.types.u8)?, u16: layout_cx.layout_of(tcx.types.u16)?, u32: layout_cx.layout_of(tcx.types.u32)?, + u64: layout_cx.layout_of(tcx.types.u64)?, + u128: layout_cx.layout_of(tcx.types.u128)?, usize: layout_cx.layout_of(tcx.types.usize)?, bool: layout_cx.layout_of(tcx.types.bool)?, mut_raw_ptr: layout_cx.layout_of(mut_raw_ptr)?, const_raw_ptr: layout_cx.layout_of(const_raw_ptr)?, }) } + + pub fn uint(&self, size: Size) -> Option> { + match size.bits() { + 8 => Some(self.u8), + 16 => Some(self.u16), + 32 => Some(self.u32), + 64 => Some(self.u64), + 128 => Some(self.u128), + _ => None, + } + } + + pub fn int(&self, size: Size) -> Option> { + match size.bits() { + 8 => Some(self.i8), + 16 => Some(self.i16), + 32 => Some(self.i32), + 64 => Some(self.i64), + 128 => Some(self.i128), + _ => None, + } + } } /// The machine itself. diff --git a/src/tools/miri/src/shims/unix/linux/sync.rs b/src/tools/miri/src/shims/unix/linux/sync.rs index 5762ee27b84af..8ae94971f6a32 100644 --- a/src/tools/miri/src/shims/unix/linux/sync.rs +++ b/src/tools/miri/src/shims/unix/linux/sync.rs @@ -214,11 +214,10 @@ pub fn futex<'tcx>( } } - let dest = dest.clone(); this.register_timeout_callback( thread, timeout_time, - Box::new(Callback { thread, addr_usize, dest }), + Box::new(Callback { thread, addr_usize, dest: dest.clone() }), ); } } else { diff --git a/src/tools/miri/src/shims/windows/dlsym.rs b/src/tools/miri/src/shims/windows/dlsym.rs index 41b9473f81fef..4b2a90723c79c 100644 --- a/src/tools/miri/src/shims/windows/dlsym.rs +++ b/src/tools/miri/src/shims/windows/dlsym.rs @@ -6,12 +6,15 @@ use log::trace; use crate::helpers::check_arg_count; use crate::shims::windows::handle::{EvalContextExt as _, Handle, PseudoHandle}; +use crate::shims::windows::sync::EvalContextExt as _; use crate::*; #[derive(Debug, Copy, Clone)] pub enum Dlsym { NtWriteFile, SetThreadDescription, + WaitOnAddress, + WakeByAddressSingle, } impl Dlsym { @@ -22,6 +25,8 @@ impl Dlsym { "GetSystemTimePreciseAsFileTime" => None, "NtWriteFile" => Some(Dlsym::NtWriteFile), "SetThreadDescription" => Some(Dlsym::SetThreadDescription), + "WaitOnAddress" => Some(Dlsym::WaitOnAddress), + "WakeByAddressSingle" => Some(Dlsym::WakeByAddressSingle), _ => throw_unsup_format!("unsupported Windows dlsym: {}", name), }) } @@ -127,6 +132,16 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { this.write_null(dest)?; } + Dlsym::WaitOnAddress => { + let [ptr_op, compare_op, size_op, timeout_op] = check_arg_count(args)?; + + this.WaitOnAddress(ptr_op, compare_op, size_op, timeout_op, dest)?; + } + Dlsym::WakeByAddressSingle => { + let [ptr_op] = check_arg_count(args)?; + + this.WakeByAddressSingle(ptr_op)?; + } } trace!("{:?}", this.dump_place(**dest)); diff --git a/src/tools/miri/src/shims/windows/sync.rs b/src/tools/miri/src/shims/windows/sync.rs index 8064ca566755c..336ba7db95f1f 100644 --- a/src/tools/miri/src/shims/windows/sync.rs +++ b/src/tools/miri/src/shims/windows/sync.rs @@ -1,3 +1,7 @@ +use std::time::Duration; + +use rustc_target::abi::Size; + use crate::concurrency::init_once::InitOnceStatus; use crate::concurrency::thread::MachineCallback; use crate::*; @@ -6,7 +10,6 @@ const SRWLOCK_ID_OFFSET: u64 = 0; const INIT_ONCE_ID_OFFSET: u64 = 0; impl<'mir, 'tcx> EvalContextExt<'mir, 'tcx> for crate::MiriInterpCx<'mir, 'tcx> {} - #[allow(non_snake_case)] pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { fn AcquireSRWLockExclusive(&mut self, lock_op: &OpTy<'tcx, Provenance>) -> InterpResult<'tcx> { @@ -221,4 +224,107 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { this.eval_windows("c", "TRUE") } + + fn WaitOnAddress( + &mut self, + ptr_op: &OpTy<'tcx, Provenance>, + compare_op: &OpTy<'tcx, Provenance>, + size_op: &OpTy<'tcx, Provenance>, + timeout_op: &OpTy<'tcx, Provenance>, + dest: &PlaceTy<'tcx, Provenance>, + ) -> InterpResult<'tcx> { + let this = self.eval_context_mut(); + + let ptr = this.read_pointer(ptr_op)?; + let compare = this.read_pointer(compare_op)?; + let size = this.read_scalar(size_op)?.to_machine_usize(this)?; + let timeout_ms = this.read_scalar(timeout_op)?.to_u32()?; + + let thread = this.get_active_thread(); + let addr = ptr.addr().bytes(); + + if size > 8 || !size.is_power_of_two() { + let invalid_param = this.eval_windows("c", "ERROR_INVALID_PARAMETER")?; + this.set_last_error(invalid_param)?; + this.write_scalar(Scalar::from_i32(0), dest)?; + return Ok(()); + }; + let size = Size::from_bytes(size); + + let timeout_time = if timeout_ms == this.eval_windows("c", "INFINITE")?.to_u32()? { + None + } else { + this.check_no_isolation("`WaitOnAddress` with non-infinite timeout")?; + + let duration = Duration::from_millis(timeout_ms.into()); + Some(Time::Monotonic(this.machine.clock.now().checked_add(duration).unwrap())) + }; + + // See the Linux futex implementation for why this fence exists. + this.atomic_fence(AtomicFenceOrd::SeqCst)?; + + let layout = this.machine.layouts.uint(size).unwrap(); + let futex_val = this + .read_scalar_atomic(&MPlaceTy::from_aligned_ptr(ptr, layout), AtomicReadOrd::Relaxed)?; + let compare_val = this.read_scalar(&MPlaceTy::from_aligned_ptr(compare, layout).into())?; + + if futex_val == compare_val { + // If the values are the same, we have to block. + this.block_thread(thread); + this.futex_wait(addr, thread, u32::MAX); + + if let Some(timeout_time) = timeout_time { + struct Callback<'tcx> { + thread: ThreadId, + addr: u64, + dest: PlaceTy<'tcx, Provenance>, + } + + impl<'tcx> VisitTags for Callback<'tcx> { + fn visit_tags(&self, visit: &mut dyn FnMut(SbTag)) { + let Callback { thread: _, addr: _, dest } = self; + dest.visit_tags(visit); + } + } + + impl<'mir, 'tcx: 'mir> MachineCallback<'mir, 'tcx> for Callback<'tcx> { + fn call(&self, this: &mut MiriInterpCx<'mir, 'tcx>) -> InterpResult<'tcx> { + this.unblock_thread(self.thread); + this.futex_remove_waiter(self.addr, self.thread); + let error_timeout = this.eval_windows("c", "ERROR_TIMEOUT")?; + this.set_last_error(error_timeout)?; + this.write_scalar(Scalar::from_i32(0), &self.dest)?; + + Ok(()) + } + } + + this.register_timeout_callback( + thread, + timeout_time, + Box::new(Callback { thread, addr, dest: dest.clone() }), + ); + } + } + + this.write_scalar(Scalar::from_i32(1), dest)?; + + Ok(()) + } + + fn WakeByAddressSingle(&mut self, ptr_op: &OpTy<'tcx, Provenance>) -> InterpResult<'tcx> { + let this = self.eval_context_mut(); + + let ptr = this.read_pointer(ptr_op)?; + + // See the Linux futex implementation for why this fence exists. + this.atomic_fence(AtomicFenceOrd::SeqCst)?; + + if let Some(thread) = this.futex_wake(ptr.addr().bytes(), u32::MAX) { + this.unblock_thread(thread); + this.unregister_timeout_callback_if_exists(thread); + } + + Ok(()) + } } diff --git a/src/tools/miri/tests/pass/concurrency/channels.rs b/src/tools/miri/tests/pass/concurrency/channels.rs index c75c5199bf11d..53b57942d76a4 100644 --- a/src/tools/miri/tests/pass/concurrency/channels.rs +++ b/src/tools/miri/tests/pass/concurrency/channels.rs @@ -1,4 +1,3 @@ -//@ignore-target-windows: Channels on Windows are not supported yet. //@compile-flags: -Zmiri-strict-provenance use std::sync::mpsc::{channel, sync_channel}; diff --git a/src/tools/miri/tests/pass/concurrency/spin_loops_nopreempt.rs b/src/tools/miri/tests/pass/concurrency/spin_loops_nopreempt.rs index 5d8e2ef5f0282..44b16e1ac74d4 100644 --- a/src/tools/miri/tests/pass/concurrency/spin_loops_nopreempt.rs +++ b/src/tools/miri/tests/pass/concurrency/spin_loops_nopreempt.rs @@ -1,4 +1,3 @@ -//@ignore-target-windows: Channels on Windows are not supported yet. // This specifically tests behavior *without* preemption. //@compile-flags: -Zmiri-preemption-rate=0 diff --git a/src/tools/miri/tests/pass/threadleak_ignored.rs b/src/tools/miri/tests/pass/threadleak_ignored.rs index 99bac7aa42a80..6a10748ee9510 100644 --- a/src/tools/miri/tests/pass/threadleak_ignored.rs +++ b/src/tools/miri/tests/pass/threadleak_ignored.rs @@ -1,4 +1,3 @@ -//@ignore-target-windows: Channels on Windows are not supported yet. // FIXME: disallow preemption to work around https://github.com/rust-lang/rust/issues/55005 //@compile-flags: -Zmiri-ignore-leaks -Zmiri-preemption-rate=0 From 3f6fc1fb5a9550347d13beb8004b2391126a4633 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 28 Oct 2022 15:48:58 +0200 Subject: [PATCH 08/12] threadleak_ignored should now pass with preemption (the issue has been fixed a while ago) --- src/tools/miri/tests/pass/threadleak_ignored.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/tools/miri/tests/pass/threadleak_ignored.rs b/src/tools/miri/tests/pass/threadleak_ignored.rs index 6a10748ee9510..a5f81573e963d 100644 --- a/src/tools/miri/tests/pass/threadleak_ignored.rs +++ b/src/tools/miri/tests/pass/threadleak_ignored.rs @@ -1,5 +1,4 @@ -// FIXME: disallow preemption to work around https://github.com/rust-lang/rust/issues/55005 -//@compile-flags: -Zmiri-ignore-leaks -Zmiri-preemption-rate=0 +//@compile-flags: -Zmiri-ignore-leaks //! Test that leaking threads works, and that their destructors are not executed. From 8f99d011f2fc7712c0eb6109f3484d51a3ea1478 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 28 Oct 2022 15:50:41 +0200 Subject: [PATCH 09/12] simplify Linux futex impl a bit --- src/tools/miri/src/shims/unix/linux/sync.rs | 29 +++++---------------- 1 file changed, 7 insertions(+), 22 deletions(-) diff --git a/src/tools/miri/src/shims/unix/linux/sync.rs b/src/tools/miri/src/shims/unix/linux/sync.rs index 8ae94971f6a32..91db30e93d8ea 100644 --- a/src/tools/miri/src/shims/unix/linux/sync.rs +++ b/src/tools/miri/src/shims/unix/linux/sync.rs @@ -1,7 +1,7 @@ +use std::time::SystemTime; + use crate::concurrency::thread::{MachineCallback, Time}; use crate::*; -use rustc_target::abi::{Align, Size}; -use std::time::SystemTime; /// Implementation of the SYS_futex syscall. /// `args` is the arguments *after* the syscall number. @@ -28,13 +28,14 @@ pub fn futex<'tcx>( // The first three arguments (after the syscall number itself) are the same to all futex operations: // (int *addr, int op, int val). // We checked above that these definitely exist. - let addr = this.read_immediate(&args[0])?; + let addr = this.read_pointer(&args[0])?; let op = this.read_scalar(&args[1])?.to_i32()?; let val = this.read_scalar(&args[2])?.to_i32()?; let thread = this.get_active_thread(); - let addr_scalar = addr.to_scalar(); - let addr_usize = addr_scalar.to_machine_usize(this)?; + // This is a vararg function so we have to bring our own type for this pointer. + let addr = MPlaceTy::from_aligned_ptr(addr, this.machine.layouts.i32); + let addr_usize = addr.ptr.addr().bytes(); let futex_private = this.eval_libc_i32("FUTEX_PRIVATE_FLAG")?; let futex_wait = this.eval_libc_i32("FUTEX_WAIT")?; @@ -117,15 +118,6 @@ pub fn futex<'tcx>( } }) }; - // Check the pointer for alignment and validity. - // The API requires `addr` to be a 4-byte aligned pointer, and will - // use the 4 bytes at the given address as an (atomic) i32. - this.check_ptr_access_align( - addr_scalar.to_pointer(this)?, - Size::from_bytes(4), - Align::from_bytes(4).unwrap(), - CheckInAllocMsg::MemoryAccessTest, - )?; // There may be a concurrent thread changing the value of addr // and then invoking the FUTEX_WAKE syscall. It is critical that the // effects of this and the other thread are correctly observed, @@ -172,14 +164,7 @@ pub fn futex<'tcx>( this.atomic_fence(AtomicFenceOrd::SeqCst)?; // Read an `i32` through the pointer, regardless of any wrapper types. // It's not uncommon for `addr` to be passed as another type than `*mut i32`, such as `*const AtomicI32`. - let futex_val = this - .read_scalar_at_offset_atomic( - &addr.into(), - 0, - this.machine.layouts.i32, - AtomicReadOrd::Relaxed, - )? - .to_i32()?; + let futex_val = this.read_scalar_atomic(&addr, AtomicReadOrd::Relaxed)?.to_i32()?; if val == futex_val { // The value still matches, so we block the thread make it wait for FUTEX_WAKE. this.block_thread(thread); From 40e340e9fb58c1260a922f9eaafe88dd49868545 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 28 Oct 2022 15:57:40 +0200 Subject: [PATCH 10/12] test most sync primitives on Windows --- src/tools/miri/tests/pass/concurrency/sync.rs | 21 ++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/src/tools/miri/tests/pass/concurrency/sync.rs b/src/tools/miri/tests/pass/concurrency/sync.rs index aa1c4892d1867..b1518a49fbb1b 100644 --- a/src/tools/miri/tests/pass/concurrency/sync.rs +++ b/src/tools/miri/tests/pass/concurrency/sync.rs @@ -1,4 +1,3 @@ -//@ignore-target-windows: Condvars on Windows are not supported yet. //@compile-flags: -Zmiri-disable-isolation -Zmiri-strict-provenance use std::sync::{Arc, Barrier, Condvar, Mutex, Once, RwLock}; @@ -225,14 +224,26 @@ fn park_unpark() { } fn main() { - check_barriers(); - check_conditional_variables_notify_one(); - check_conditional_variables_timed_wait_timeout(); - check_conditional_variables_timed_wait_notimeout(); check_mutex(); check_rwlock_write(); check_rwlock_read_no_deadlock(); check_once(); park_timeout(); park_unpark(); + + if !cfg!(windows) { + // ignore-target-windows: Condvars on Windows are not supported yet + check_barriers(); + check_conditional_variables_notify_one(); + check_conditional_variables_timed_wait_timeout(); + check_conditional_variables_timed_wait_notimeout(); + } else { + // We need to fake the same output... + for _ in 0..10 { + println!("before wait"); + } + for _ in 0..10 { + println!("after wait"); + } + } } From 9b0cdf9a6ef71708fbb405d53a64e4e60b2691a2 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 28 Oct 2022 16:00:42 +0200 Subject: [PATCH 11/12] cleanup some test cfg --- .../miri/tests/pass-dep/shims/libc-misc.rs | 22 +++++++++---------- .../miri/tests/pass-dep/shims/pthreads.rs | 2 +- 2 files changed, 11 insertions(+), 13 deletions(-) diff --git a/src/tools/miri/tests/pass-dep/shims/libc-misc.rs b/src/tools/miri/tests/pass-dep/shims/libc-misc.rs index a883a3d967a3c..904ae2fb17f9b 100644 --- a/src/tools/miri/tests/pass-dep/shims/libc-misc.rs +++ b/src/tools/miri/tests/pass-dep/shims/libc-misc.rs @@ -87,7 +87,7 @@ fn test_posix_realpath_errors() { assert_eq!(e.kind(), ErrorKind::NotFound); } -#[cfg(any(target_os = "linux"))] +#[cfg(target_os = "linux")] fn test_posix_fadvise() { use std::convert::TryInto; use std::io::Write; @@ -115,7 +115,7 @@ fn test_posix_fadvise() { assert_eq!(result, 0); } -#[cfg(any(target_os = "linux"))] +#[cfg(target_os = "linux")] fn test_sync_file_range() { use std::io::Write; @@ -181,7 +181,7 @@ fn test_thread_local_errno() { } /// Tests whether clock support exists at all -#[cfg(any(target_os = "linux"))] +#[cfg(target_os = "linux")] fn test_clocks() { let mut tp = std::mem::MaybeUninit::::uninit(); let is_error = unsafe { libc::clock_gettime(libc::CLOCK_REALTIME, tp.as_mut_ptr()) }; @@ -283,9 +283,6 @@ fn test_posix_mkstemp() { } fn main() { - #[cfg(any(target_os = "linux"))] - test_posix_fadvise(); - test_posix_gettimeofday(); test_posix_mkstemp(); @@ -293,13 +290,14 @@ fn main() { test_posix_realpath_noalloc(); test_posix_realpath_errors(); - #[cfg(any(target_os = "linux"))] - test_sync_file_range(); - test_thread_local_errno(); - #[cfg(any(target_os = "linux"))] - test_clocks(); - test_isatty(); + + #[cfg(target_os = "linux")] + { + test_posix_fadvise(); + test_sync_file_range(); + test_clocks(); + } } diff --git a/src/tools/miri/tests/pass-dep/shims/pthreads.rs b/src/tools/miri/tests/pass-dep/shims/pthreads.rs index ae02a8e3c980a..09dd92564d39a 100644 --- a/src/tools/miri/tests/pass-dep/shims/pthreads.rs +++ b/src/tools/miri/tests/pass-dep/shims/pthreads.rs @@ -10,7 +10,7 @@ fn main() { test_rwlock_libc_static_initializer(); test_named_thread_truncation(); - #[cfg(any(target_os = "linux"))] + #[cfg(target_os = "linux")] test_mutex_libc_static_initializer_recursive(); } From c8a5d4b6000146db63574196970e9f7d803a386d Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 28 Oct 2022 16:13:10 +0200 Subject: [PATCH 12/12] relative futex and condvar timeouts can work with isolation --- src/tools/miri/src/shims/unix/linux/sync.rs | 8 +- src/tools/miri/src/shims/unix/sync.rs | 3 +- src/tools/miri/src/shims/windows/sync.rs | 2 - .../concurrency/libc_pthread_cond_isolated.rs | 82 +++++++++++++++++++ .../pass/concurrency/thread_park_isolated.rs | 12 +++ 5 files changed, 100 insertions(+), 7 deletions(-) create mode 100644 src/tools/miri/tests/pass-dep/concurrency/libc_pthread_cond_isolated.rs create mode 100644 src/tools/miri/tests/pass/concurrency/thread_park_isolated.rs diff --git a/src/tools/miri/src/shims/unix/linux/sync.rs b/src/tools/miri/src/shims/unix/linux/sync.rs index 91db30e93d8ea..292b9d2e7a176 100644 --- a/src/tools/miri/src/shims/unix/linux/sync.rs +++ b/src/tools/miri/src/shims/unix/linux/sync.rs @@ -90,9 +90,11 @@ pub fn futex<'tcx>( let timeout_time = if this.ptr_is_null(timeout.ptr)? { None } else { - this.check_no_isolation( - "`futex` syscall with `op=FUTEX_WAIT` and non-null timeout", - )?; + if op & futex_realtime != 0 { + this.check_no_isolation( + "`futex` syscall with `op=FUTEX_WAIT` and non-null timeout with `FUTEX_CLOCK_REALTIME`", + )?; + } let duration = match this.read_timespec(&timeout)? { Some(duration) => duration, None => { diff --git a/src/tools/miri/src/shims/unix/sync.rs b/src/tools/miri/src/shims/unix/sync.rs index 3e1e34c5dbe7b..fcb006920794c 100644 --- a/src/tools/miri/src/shims/unix/sync.rs +++ b/src/tools/miri/src/shims/unix/sync.rs @@ -743,8 +743,6 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { ) -> InterpResult<'tcx> { let this = self.eval_context_mut(); - this.check_no_isolation("`pthread_cond_timedwait`")?; - let id = this.condvar_get_or_create_id(cond_op, CONDVAR_ID_OFFSET)?; let mutex_id = this.mutex_get_or_create_id(mutex_op, MUTEX_ID_OFFSET)?; let active_thread = this.get_active_thread(); @@ -761,6 +759,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { }; let timeout_time = if clock_id == this.eval_libc_i32("CLOCK_REALTIME")? { + this.check_no_isolation("`pthread_cond_timedwait` with `CLOCK_REALTIME`")?; Time::RealTime(SystemTime::UNIX_EPOCH.checked_add(duration).unwrap()) } else if clock_id == this.eval_libc_i32("CLOCK_MONOTONIC")? { Time::Monotonic(this.machine.clock.anchor().checked_add(duration).unwrap()) diff --git a/src/tools/miri/src/shims/windows/sync.rs b/src/tools/miri/src/shims/windows/sync.rs index 336ba7db95f1f..8156ae8af1ef1 100644 --- a/src/tools/miri/src/shims/windows/sync.rs +++ b/src/tools/miri/src/shims/windows/sync.rs @@ -254,8 +254,6 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { let timeout_time = if timeout_ms == this.eval_windows("c", "INFINITE")?.to_u32()? { None } else { - this.check_no_isolation("`WaitOnAddress` with non-infinite timeout")?; - let duration = Duration::from_millis(timeout_ms.into()); Some(Time::Monotonic(this.machine.clock.now().checked_add(duration).unwrap())) }; diff --git a/src/tools/miri/tests/pass-dep/concurrency/libc_pthread_cond_isolated.rs b/src/tools/miri/tests/pass-dep/concurrency/libc_pthread_cond_isolated.rs new file mode 100644 index 0000000000000..103ce44006d3a --- /dev/null +++ b/src/tools/miri/tests/pass-dep/concurrency/libc_pthread_cond_isolated.rs @@ -0,0 +1,82 @@ +//@ignore-target-windows: No libc on Windows +//@ignore-target-apple: pthread_condattr_setclock is not supported on MacOS. + +/// Test that conditional variable timeouts are working properly +/// with monotonic clocks even under isolation. +use std::mem::MaybeUninit; +use std::time::Instant; + +fn test_timed_wait_timeout(clock_id: i32) { + unsafe { + let mut attr: MaybeUninit = MaybeUninit::uninit(); + assert_eq!(libc::pthread_condattr_init(attr.as_mut_ptr()), 0); + assert_eq!(libc::pthread_condattr_setclock(attr.as_mut_ptr(), clock_id), 0); + + let mut cond: MaybeUninit = MaybeUninit::uninit(); + assert_eq!(libc::pthread_cond_init(cond.as_mut_ptr(), attr.as_ptr()), 0); + assert_eq!(libc::pthread_condattr_destroy(attr.as_mut_ptr()), 0); + + let mut mutex: libc::pthread_mutex_t = libc::PTHREAD_MUTEX_INITIALIZER; + + let mut now_mu: MaybeUninit = MaybeUninit::uninit(); + assert_eq!(libc::clock_gettime(clock_id, now_mu.as_mut_ptr()), 0); + let now = now_mu.assume_init(); + // Waiting for a second... mostly because waiting less requires mich more tricky arithmetic. + // FIXME: wait less. + let timeout = libc::timespec { tv_sec: now.tv_sec + 1, tv_nsec: now.tv_nsec }; + + assert_eq!(libc::pthread_mutex_lock(&mut mutex as *mut _), 0); + let current_time = Instant::now(); + assert_eq!( + libc::pthread_cond_timedwait(cond.as_mut_ptr(), &mut mutex as *mut _, &timeout), + libc::ETIMEDOUT + ); + let elapsed_time = current_time.elapsed().as_millis(); + assert!(900 <= elapsed_time && elapsed_time <= 1300); + + // Test calling `pthread_cond_timedwait` again with an already elapsed timeout. + assert_eq!( + libc::pthread_cond_timedwait(cond.as_mut_ptr(), &mut mutex as *mut _, &timeout), + libc::ETIMEDOUT + ); + + // Test that invalid nanosecond values (above 10^9 or negative) are rejected with the + // correct error code. + let invalid_timeout_1 = libc::timespec { tv_sec: now.tv_sec + 1, tv_nsec: 1_000_000_000 }; + assert_eq!( + libc::pthread_cond_timedwait( + cond.as_mut_ptr(), + &mut mutex as *mut _, + &invalid_timeout_1 + ), + libc::EINVAL + ); + let invalid_timeout_2 = libc::timespec { tv_sec: now.tv_sec + 1, tv_nsec: -1 }; + assert_eq!( + libc::pthread_cond_timedwait( + cond.as_mut_ptr(), + &mut mutex as *mut _, + &invalid_timeout_2 + ), + libc::EINVAL + ); + // Test that invalid second values (negative) are rejected with the correct error code. + let invalid_timeout_3 = libc::timespec { tv_sec: -1, tv_nsec: 0 }; + assert_eq!( + libc::pthread_cond_timedwait( + cond.as_mut_ptr(), + &mut mutex as *mut _, + &invalid_timeout_3 + ), + libc::EINVAL + ); + + assert_eq!(libc::pthread_mutex_unlock(&mut mutex as *mut _), 0); + assert_eq!(libc::pthread_mutex_destroy(&mut mutex as *mut _), 0); + assert_eq!(libc::pthread_cond_destroy(cond.as_mut_ptr()), 0); + } +} + +fn main() { + test_timed_wait_timeout(libc::CLOCK_MONOTONIC); +} diff --git a/src/tools/miri/tests/pass/concurrency/thread_park_isolated.rs b/src/tools/miri/tests/pass/concurrency/thread_park_isolated.rs new file mode 100644 index 0000000000000..bf004012e8489 --- /dev/null +++ b/src/tools/miri/tests/pass/concurrency/thread_park_isolated.rs @@ -0,0 +1,12 @@ +//@ignore-target-apple: park_timeout on macOS uses the system clock +use std::thread; +use std::time::{Duration, Instant}; + +fn main() { + let start = Instant::now(); + + thread::park_timeout(Duration::from_millis(200)); + + // Thanks to deterministic execution, this will wiat *exactly* 200ms (rounded to 1ms). + assert!((200..201).contains(&start.elapsed().as_millis())); +}