From 20f418252f2a207abca10a83ed2b04cb0b7aa57c Mon Sep 17 00:00:00 2001 From: The Miri Cronjob Bot Date: Tue, 16 Apr 2024 05:15:16 +0000 Subject: [PATCH 01/33] Preparing for merge from rustc --- src/tools/miri/rust-version | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tools/miri/rust-version b/src/tools/miri/rust-version index 6ad8fba723c..dfa7f8ca507 100644 --- a/src/tools/miri/rust-version +++ b/src/tools/miri/rust-version @@ -1 +1 @@ -23d47dba319331d4418827cfbb8c1af283497d3c +63f70b3d104e20289a1a0df82747066c3d85b9a1 From 3d3a584e4d88a572d68d7996122cda69a38dcf2e Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 16 Apr 2024 08:03:48 +0200 Subject: [PATCH 02/33] bless test-cargo-miri --- src/tools/miri/test-cargo-miri/test.bin-target.stdout.ref | 2 +- src/tools/miri/test-cargo-miri/test.cross-target.stdout.ref | 4 ++-- src/tools/miri/test-cargo-miri/test.default.stdout.ref | 4 ++-- .../miri/test-cargo-miri/test.filter.cross-target.stdout.ref | 4 ++-- src/tools/miri/test-cargo-miri/test.filter.stdout.ref | 4 ++-- src/tools/miri/test-cargo-miri/test.subcrate.stdout.ref | 2 +- src/tools/miri/test-cargo-miri/test.test-target.stdout.ref | 2 +- 7 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/tools/miri/test-cargo-miri/test.bin-target.stdout.ref b/src/tools/miri/test-cargo-miri/test.bin-target.stdout.ref index 5264530160b..6f480259965 100644 --- a/src/tools/miri/test-cargo-miri/test.bin-target.stdout.ref +++ b/src/tools/miri/test-cargo-miri/test.bin-target.stdout.ref @@ -3,5 +3,5 @@ running 2 tests test test::dev_dependency ... ok test test::exported_symbol ... ok -test result: ok. 2 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out +test result: ok. 2 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in $TIME diff --git a/src/tools/miri/test-cargo-miri/test.cross-target.stdout.ref b/src/tools/miri/test-cargo-miri/test.cross-target.stdout.ref index 8c543e479f4..2ef124e4de8 100644 --- a/src/tools/miri/test-cargo-miri/test.cross-target.stdout.ref +++ b/src/tools/miri/test-cargo-miri/test.cross-target.stdout.ref @@ -1,11 +1,11 @@ running 2 tests .. -test result: ok. 2 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out +test result: ok. 2 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in $TIME imported main running 6 tests ...i.. -test result: ok. 5 passed; 0 failed; 1 ignored; 0 measured; 0 filtered out +test result: ok. 5 passed; 0 failed; 1 ignored; 0 measured; 0 filtered out; finished in $TIME diff --git a/src/tools/miri/test-cargo-miri/test.default.stdout.ref b/src/tools/miri/test-cargo-miri/test.default.stdout.ref index 922d2120bed..2d74d82f769 100644 --- a/src/tools/miri/test-cargo-miri/test.default.stdout.ref +++ b/src/tools/miri/test-cargo-miri/test.default.stdout.ref @@ -1,13 +1,13 @@ running 2 tests .. -test result: ok. 2 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out +test result: ok. 2 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in $TIME imported main running 6 tests ...i.. -test result: ok. 5 passed; 0 failed; 1 ignored; 0 measured; 0 filtered out +test result: ok. 5 passed; 0 failed; 1 ignored; 0 measured; 0 filtered out; finished in $TIME running 5 tests diff --git a/src/tools/miri/test-cargo-miri/test.filter.cross-target.stdout.ref b/src/tools/miri/test-cargo-miri/test.filter.cross-target.stdout.ref index bb0282d6c91..59b4deb1ff3 100644 --- a/src/tools/miri/test-cargo-miri/test.filter.cross-target.stdout.ref +++ b/src/tools/miri/test-cargo-miri/test.filter.cross-target.stdout.ref @@ -1,12 +1,12 @@ running 0 tests -test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 2 filtered out +test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 2 filtered out; finished in $TIME imported main running 1 test test simple ... ok -test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 5 filtered out +test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 5 filtered out; finished in $TIME diff --git a/src/tools/miri/test-cargo-miri/test.filter.stdout.ref b/src/tools/miri/test-cargo-miri/test.filter.stdout.ref index 5c819dd5323..b68bc983276 100644 --- a/src/tools/miri/test-cargo-miri/test.filter.stdout.ref +++ b/src/tools/miri/test-cargo-miri/test.filter.stdout.ref @@ -1,14 +1,14 @@ running 0 tests -test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 2 filtered out +test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 2 filtered out; finished in $TIME imported main running 1 test test simple ... ok -test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 5 filtered out +test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 5 filtered out; finished in $TIME running 0 tests diff --git a/src/tools/miri/test-cargo-miri/test.subcrate.stdout.ref b/src/tools/miri/test-cargo-miri/test.subcrate.stdout.ref index 67e5c7f8e92..e50838ebc83 100644 --- a/src/tools/miri/test-cargo-miri/test.subcrate.stdout.ref +++ b/src/tools/miri/test-cargo-miri/test.subcrate.stdout.ref @@ -1,6 +1,6 @@ running 0 tests -test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out +test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in $TIME subcrate testing diff --git a/src/tools/miri/test-cargo-miri/test.test-target.stdout.ref b/src/tools/miri/test-cargo-miri/test.test-target.stdout.ref index dd59b32b780..38b3f5c0989 100644 --- a/src/tools/miri/test-cargo-miri/test.test-target.stdout.ref +++ b/src/tools/miri/test-cargo-miri/test.test-target.stdout.ref @@ -7,5 +7,5 @@ test does_not_work_on_miri ... ignored test fail_index_check - should panic ... ok test simple ... ok -test result: ok. 5 passed; 0 failed; 1 ignored; 0 measured; 0 filtered out +test result: ok. 5 passed; 0 failed; 1 ignored; 0 measured; 0 filtered out; finished in $TIME From 3d5acebbf17b97b6c9d22b47271c5deb3525b575 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 16 Apr 2024 11:29:04 +0200 Subject: [PATCH 03/33] threads: keep track of why we are blocked, and sanity-check that when waking up --- src/tools/miri/src/concurrency/init_once.rs | 4 +- src/tools/miri/src/concurrency/sync.rs | 32 +++------ src/tools/miri/src/concurrency/thread.rs | 74 +++++++++++++-------- src/tools/miri/src/lib.rs | 4 +- src/tools/miri/src/shims/time.rs | 10 +-- src/tools/miri/src/shims/unix/linux/sync.rs | 23 +++++-- src/tools/miri/src/shims/unix/sync.rs | 40 +++++------ src/tools/miri/src/shims/windows/sync.rs | 16 +++-- 8 files changed, 108 insertions(+), 95 deletions(-) diff --git a/src/tools/miri/src/concurrency/init_once.rs b/src/tools/miri/src/concurrency/init_once.rs index 35dcfecbbe3..9dbea08f3ef 100644 --- a/src/tools/miri/src/concurrency/init_once.rs +++ b/src/tools/miri/src/concurrency/init_once.rs @@ -77,7 +77,7 @@ trait EvalContextExtPriv<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { let this = self.eval_context_mut(); let current_thread = this.get_active_thread(); - this.unblock_thread(waiter.thread); + this.unblock_thread(waiter.thread, BlockReason::InitOnce(id)); // Call callback, with the woken-up thread as `current`. this.set_active_thread(waiter.thread); @@ -142,7 +142,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { let init_once = &mut this.machine.threads.sync.init_onces[id]; assert_ne!(init_once.status, InitOnceStatus::Complete, "queueing on complete init once"); init_once.waiters.push_back(InitOnceWaiter { thread, callback }); - this.block_thread(thread); + this.block_thread(thread, BlockReason::InitOnce(id)); } /// Begin initializing this InitOnce. Must only be called after checking that it is currently diff --git a/src/tools/miri/src/concurrency/sync.rs b/src/tools/miri/src/concurrency/sync.rs index 956a02ded0f..0a428715690 100644 --- a/src/tools/miri/src/concurrency/sync.rs +++ b/src/tools/miri/src/concurrency/sync.rs @@ -115,25 +115,13 @@ struct RwLock { declare_id!(CondvarId); -#[derive(Debug, Copy, Clone)] -pub enum RwLockMode { - Read, - Write, -} - -#[derive(Debug)] -pub enum CondvarLock { - Mutex(MutexId), - RwLock { id: RwLockId, mode: RwLockMode }, -} - /// A thread waiting on a conditional variable. #[derive(Debug)] struct CondvarWaiter { /// The thread that is waiting on this variable. thread: ThreadId, - /// The mutex or rwlock on which the thread is waiting. - lock: CondvarLock, + /// The mutex on which the thread is waiting. + lock: MutexId, } /// The conditional variable state. @@ -232,7 +220,7 @@ pub(super) trait EvalContextExtPriv<'mir, 'tcx: 'mir>: fn rwlock_dequeue_and_lock_reader(&mut self, id: RwLockId) -> bool { let this = self.eval_context_mut(); if let Some(reader) = this.machine.threads.sync.rwlocks[id].reader_queue.pop_front() { - this.unblock_thread(reader); + this.unblock_thread(reader, BlockReason::RwLock(id)); this.rwlock_reader_lock(id, reader); true } else { @@ -246,7 +234,7 @@ pub(super) trait EvalContextExtPriv<'mir, 'tcx: 'mir>: fn rwlock_dequeue_and_lock_writer(&mut self, id: RwLockId) -> bool { let this = self.eval_context_mut(); if let Some(writer) = this.machine.threads.sync.rwlocks[id].writer_queue.pop_front() { - this.unblock_thread(writer); + this.unblock_thread(writer, BlockReason::RwLock(id)); this.rwlock_writer_lock(id, writer); true } else { @@ -260,7 +248,7 @@ pub(super) trait EvalContextExtPriv<'mir, 'tcx: 'mir>: fn mutex_dequeue_and_lock(&mut self, id: MutexId) -> bool { let this = self.eval_context_mut(); if let Some(thread) = this.machine.threads.sync.mutexes[id].queue.pop_front() { - this.unblock_thread(thread); + this.unblock_thread(thread, BlockReason::Mutex(id)); this.mutex_lock(id, thread); true } else { @@ -406,7 +394,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { let this = self.eval_context_mut(); assert!(this.mutex_is_locked(id), "queing on unlocked mutex"); this.machine.threads.sync.mutexes[id].queue.push_back(thread); - this.block_thread(thread); + this.block_thread(thread, BlockReason::Mutex(id)); } /// Provides the closure with the next RwLockId. Creates that RwLock if the closure returns None, @@ -511,7 +499,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { let this = self.eval_context_mut(); assert!(this.rwlock_is_write_locked(id), "read-queueing on not write locked rwlock"); this.machine.threads.sync.rwlocks[id].reader_queue.push_back(reader); - this.block_thread(reader); + this.block_thread(reader, BlockReason::RwLock(id)); } /// Lock by setting the writer that owns the lock. @@ -573,7 +561,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { let this = self.eval_context_mut(); assert!(this.rwlock_is_locked(id), "write-queueing on unlocked rwlock"); this.machine.threads.sync.rwlocks[id].writer_queue.push_back(writer); - this.block_thread(writer); + this.block_thread(writer, BlockReason::RwLock(id)); } /// Provides the closure with the next CondvarId. Creates that Condvar if the closure returns None, @@ -605,7 +593,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { } /// Mark that the thread is waiting on the conditional variable. - fn condvar_wait(&mut self, id: CondvarId, thread: ThreadId, lock: CondvarLock) { + fn condvar_wait(&mut self, id: CondvarId, thread: ThreadId, lock: MutexId) { let this = self.eval_context_mut(); let waiters = &mut this.machine.threads.sync.condvars[id].waiters; assert!(waiters.iter().all(|waiter| waiter.thread != thread), "thread is already waiting"); @@ -614,7 +602,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { /// Wake up some thread (if there is any) sleeping on the conditional /// variable. - fn condvar_signal(&mut self, id: CondvarId) -> Option<(ThreadId, CondvarLock)> { + fn condvar_signal(&mut self, id: CondvarId) -> Option<(ThreadId, MutexId)> { let this = self.eval_context_mut(); let current_thread = this.get_active_thread(); let current_span = this.machine.current_span(); diff --git a/src/tools/miri/src/concurrency/thread.rs b/src/tools/miri/src/concurrency/thread.rs index d1136272f01..06e23188332 100644 --- a/src/tools/miri/src/concurrency/thread.rs +++ b/src/tools/miri/src/concurrency/thread.rs @@ -88,18 +88,33 @@ impl From for u64 { } } +/// Keeps track of what the thread is blocked on. +#[derive(Debug, Copy, Clone, PartialEq, Eq)] +pub enum BlockReason { + /// The thread tried to join the specified thread and is blocked until that + /// thread terminates. + Join(ThreadId), + /// Waiting for time to pass. + Sleep, + /// Blocked on a mutex. + Mutex(MutexId), + /// Blocked on a condition variable. + Condvar(CondvarId), + /// Blocked on a reader-writer lock. + RwLock(RwLockId), + /// Blocled on a Futex variable. + Futex { addr: u64 }, + /// Blocked on an InitOnce. + InitOnce(InitOnceId), +} + /// The state of a thread. #[derive(Debug, Copy, Clone, PartialEq, Eq)] pub enum ThreadState { /// The thread is enabled and can be executed. Enabled, - /// The thread tried to join the specified thread and is blocked until that - /// thread terminates. - BlockedOnJoin(ThreadId), - /// The thread is blocked on some synchronization primitive. It is the - /// responsibility of the synchronization primitives to track threads that - /// are blocked by them. - BlockedOnSync, + /// The thread is blocked on something. + Blocked(BlockReason), /// The thread has terminated its execution. We do not delete terminated /// threads (FIXME: why?). Terminated, @@ -296,17 +311,17 @@ impl VisitProvenance for Frame<'_, '_, Provenance, FrameExtra<'_>> { /// A specific moment in time. #[derive(Debug)] -pub enum Time { +pub enum CallbackTime { Monotonic(Instant), RealTime(SystemTime), } -impl Time { +impl CallbackTime { /// How long do we have to wait from now until the specified time? fn get_wait_time(&self, clock: &Clock) -> Duration { match self { - Time::Monotonic(instant) => instant.duration_since(clock.now()), - Time::RealTime(time) => + CallbackTime::Monotonic(instant) => instant.duration_since(clock.now()), + CallbackTime::RealTime(time) => time.duration_since(SystemTime::now()).unwrap_or(Duration::new(0, 0)), } } @@ -318,7 +333,7 @@ impl Time { /// conditional variable, the signal handler deletes the callback. struct TimeoutCallbackInfo<'mir, 'tcx> { /// The callback should be called no earlier than this time. - call_time: Time, + call_time: CallbackTime, /// The called function. callback: TimeoutCallback<'mir, 'tcx>, } @@ -539,7 +554,8 @@ impl<'mir, 'tcx: 'mir> ThreadManager<'mir, 'tcx> { self.threads[joined_thread_id].join_status = ThreadJoinStatus::Joined; if self.threads[joined_thread_id].state != ThreadState::Terminated { // The joined thread is still running, we need to wait for it. - self.active_thread_mut().state = ThreadState::BlockedOnJoin(joined_thread_id); + self.active_thread_mut().state = + ThreadState::Blocked(BlockReason::Join(joined_thread_id)); trace!( "{:?} blocked on {:?} when trying to join", self.active_thread, @@ -569,10 +585,11 @@ impl<'mir, 'tcx: 'mir> ThreadManager<'mir, 'tcx> { throw_ub_format!("trying to join itself"); } + // Sanity check `join_status`. assert!( - self.threads - .iter() - .all(|thread| thread.state != ThreadState::BlockedOnJoin(joined_thread_id)), + self.threads.iter().all(|thread| { + thread.state != ThreadState::Blocked(BlockReason::Join(joined_thread_id)) + }), "this thread already has threads waiting for its termination" ); @@ -594,16 +611,17 @@ impl<'mir, 'tcx: 'mir> ThreadManager<'mir, 'tcx> { } /// Put the thread into the blocked state. - fn block_thread(&mut self, thread: ThreadId) { + fn block_thread(&mut self, thread: ThreadId, reason: BlockReason) { let state = &mut self.threads[thread].state; assert_eq!(*state, ThreadState::Enabled); - *state = ThreadState::BlockedOnSync; + *state = ThreadState::Blocked(reason); } /// Put the blocked thread into the enabled state. - fn unblock_thread(&mut self, thread: ThreadId) { + /// Sanity-checks that the thread previously was blocked for the right reason. + fn unblock_thread(&mut self, thread: ThreadId, reason: BlockReason) { let state = &mut self.threads[thread].state; - assert_eq!(*state, ThreadState::BlockedOnSync); + assert_eq!(*state, ThreadState::Blocked(reason)); *state = ThreadState::Enabled; } @@ -622,7 +640,7 @@ impl<'mir, 'tcx: 'mir> ThreadManager<'mir, 'tcx> { fn register_timeout_callback( &mut self, thread: ThreadId, - call_time: Time, + call_time: CallbackTime, callback: TimeoutCallback<'mir, 'tcx>, ) { self.timeout_callbacks @@ -683,7 +701,7 @@ impl<'mir, 'tcx: 'mir> ThreadManager<'mir, 'tcx> { // Check if we need to unblock any threads. let mut joined_threads = vec![]; // store which threads joined, we'll need it for (i, thread) in self.threads.iter_enumerated_mut() { - if thread.state == ThreadState::BlockedOnJoin(self.active_thread) { + if thread.state == ThreadState::Blocked(BlockReason::Join(self.active_thread)) { // The thread has terminated, mark happens-before edge to joining thread if data_race.is_some() { joined_threads.push(i); @@ -999,13 +1017,13 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { } #[inline] - fn block_thread(&mut self, thread: ThreadId) { - self.eval_context_mut().machine.threads.block_thread(thread); + fn block_thread(&mut self, thread: ThreadId, reason: BlockReason) { + self.eval_context_mut().machine.threads.block_thread(thread, reason); } #[inline] - fn unblock_thread(&mut self, thread: ThreadId) { - self.eval_context_mut().machine.threads.unblock_thread(thread); + fn unblock_thread(&mut self, thread: ThreadId, reason: BlockReason) { + self.eval_context_mut().machine.threads.unblock_thread(thread, reason); } #[inline] @@ -1027,11 +1045,11 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { fn register_timeout_callback( &mut self, thread: ThreadId, - call_time: Time, + call_time: CallbackTime, callback: TimeoutCallback<'mir, 'tcx>, ) { let this = self.eval_context_mut(); - if !this.machine.communicate() && matches!(call_time, Time::RealTime(..)) { + if !this.machine.communicate() && matches!(call_time, CallbackTime::RealTime(..)) { panic!("cannot have `RealTime` callback with isolation enabled!") } this.machine.threads.register_timeout_callback(thread, call_time, callback); diff --git a/src/tools/miri/src/lib.rs b/src/tools/miri/src/lib.rs index 484908086ac..fbe4c9c6760 100644 --- a/src/tools/miri/src/lib.rs +++ b/src/tools/miri/src/lib.rs @@ -116,7 +116,9 @@ pub use crate::concurrency::{ data_race::{AtomicFenceOrd, AtomicReadOrd, AtomicRwOrd, AtomicWriteOrd, EvalContextExt as _}, init_once::{EvalContextExt as _, InitOnceId}, sync::{CondvarId, EvalContextExt as _, MutexId, RwLockId, SyncId}, - thread::{EvalContextExt as _, StackEmptyCallback, ThreadId, ThreadManager, Time}, + thread::{ + BlockReason, CallbackTime, EvalContextExt as _, StackEmptyCallback, ThreadId, ThreadManager, + }, }; pub use crate::diagnostics::{ report_error, EvalContextExt as _, NonHaltingDiagnostic, TerminationInfo, diff --git a/src/tools/miri/src/shims/time.rs b/src/tools/miri/src/shims/time.rs index 4535bcf6dfe..1126c900226 100644 --- a/src/tools/miri/src/shims/time.rs +++ b/src/tools/miri/src/shims/time.rs @@ -236,11 +236,11 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { .unwrap_or_else(|| now.checked_add(Duration::from_secs(3600)).unwrap()); let active_thread = this.get_active_thread(); - this.block_thread(active_thread); + this.block_thread(active_thread, BlockReason::Sleep); this.register_timeout_callback( active_thread, - Time::Monotonic(timeout_time), + CallbackTime::Monotonic(timeout_time), Box::new(UnblockCallback { thread_to_unblock: active_thread }), ); @@ -259,11 +259,11 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { let timeout_time = this.machine.clock.now().checked_add(duration).unwrap(); let active_thread = this.get_active_thread(); - this.block_thread(active_thread); + this.block_thread(active_thread, BlockReason::Sleep); this.register_timeout_callback( active_thread, - Time::Monotonic(timeout_time), + CallbackTime::Monotonic(timeout_time), Box::new(UnblockCallback { thread_to_unblock: active_thread }), ); @@ -281,7 +281,7 @@ impl VisitProvenance for UnblockCallback { impl<'mir, 'tcx: 'mir> MachineCallback<'mir, 'tcx> for UnblockCallback { fn call(&self, ecx: &mut MiriInterpCx<'mir, 'tcx>) -> InterpResult<'tcx> { - ecx.unblock_thread(self.thread_to_unblock); + ecx.unblock_thread(self.thread_to_unblock, BlockReason::Sleep); Ok(()) } } diff --git a/src/tools/miri/src/shims/unix/linux/sync.rs b/src/tools/miri/src/shims/unix/linux/sync.rs index ed27066aa6a..d4a6cd96f48 100644 --- a/src/tools/miri/src/shims/unix/linux/sync.rs +++ b/src/tools/miri/src/shims/unix/linux/sync.rs @@ -107,16 +107,22 @@ pub fn futex<'tcx>( Some(if wait_bitset { // FUTEX_WAIT_BITSET uses an absolute timestamp. if realtime { - Time::RealTime(SystemTime::UNIX_EPOCH.checked_add(duration).unwrap()) + CallbackTime::RealTime( + SystemTime::UNIX_EPOCH.checked_add(duration).unwrap(), + ) } else { - Time::Monotonic(this.machine.clock.anchor().checked_add(duration).unwrap()) + CallbackTime::Monotonic( + this.machine.clock.anchor().checked_add(duration).unwrap(), + ) } } else { // FUTEX_WAIT uses a relative timestamp. if realtime { - Time::RealTime(SystemTime::now().checked_add(duration).unwrap()) + CallbackTime::RealTime(SystemTime::now().checked_add(duration).unwrap()) } else { - Time::Monotonic(this.machine.clock.now().checked_add(duration).unwrap()) + CallbackTime::Monotonic( + this.machine.clock.now().checked_add(duration).unwrap(), + ) } }) }; @@ -169,7 +175,7 @@ pub fn futex<'tcx>( 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); + this.block_thread(thread, BlockReason::Futex { addr: addr_usize }); this.futex_wait(addr_usize, thread, bitset); // Succesfully waking up from FUTEX_WAIT always returns zero. this.write_scalar(Scalar::from_target_isize(0, this), dest)?; @@ -191,7 +197,10 @@ pub fn futex<'tcx>( 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.unblock_thread( + self.thread, + BlockReason::Futex { addr: self.addr_usize }, + ); this.futex_remove_waiter(self.addr_usize, self.thread); let etimedout = this.eval_libc("ETIMEDOUT"); this.set_last_error(etimedout)?; @@ -249,7 +258,7 @@ pub fn futex<'tcx>( #[allow(clippy::arithmetic_side_effects)] for _ in 0..val { if let Some(thread) = this.futex_wake(addr_usize, bitset) { - this.unblock_thread(thread); + this.unblock_thread(thread, BlockReason::Futex { addr: addr_usize }); this.unregister_timeout_callback_if_exists(thread); n += 1; } else { diff --git a/src/tools/miri/src/shims/unix/sync.rs b/src/tools/miri/src/shims/unix/sync.rs index dd301f9ee6d..e50a8934e09 100644 --- a/src/tools/miri/src/shims/unix/sync.rs +++ b/src/tools/miri/src/shims/unix/sync.rs @@ -1,6 +1,5 @@ use std::time::SystemTime; -use crate::concurrency::sync::CondvarLock; use crate::concurrency::thread::MachineCallback; use crate::*; @@ -225,9 +224,10 @@ fn cond_set_clock_id<'mir, 'tcx: 'mir>( fn reacquire_cond_mutex<'mir, 'tcx: 'mir>( ecx: &mut MiriInterpCx<'mir, 'tcx>, thread: ThreadId, + condvar: CondvarId, mutex: MutexId, ) -> InterpResult<'tcx> { - ecx.unblock_thread(thread); + ecx.unblock_thread(thread, BlockReason::Condvar(condvar)); if ecx.mutex_is_locked(mutex) { ecx.mutex_enqueue_and_block(mutex, thread); } else { @@ -242,9 +242,10 @@ fn reacquire_cond_mutex<'mir, 'tcx: 'mir>( fn post_cond_signal<'mir, 'tcx: 'mir>( ecx: &mut MiriInterpCx<'mir, 'tcx>, thread: ThreadId, + condvar: CondvarId, mutex: MutexId, ) -> InterpResult<'tcx> { - reacquire_cond_mutex(ecx, thread, mutex)?; + reacquire_cond_mutex(ecx, thread, condvar, mutex)?; // Waiting for the mutex is not included in the waiting time because we need // to acquire the mutex always even if we get a timeout. ecx.unregister_timeout_callback_if_exists(thread); @@ -256,6 +257,7 @@ fn post_cond_signal<'mir, 'tcx: 'mir>( fn release_cond_mutex_and_block<'mir, 'tcx: 'mir>( ecx: &mut MiriInterpCx<'mir, 'tcx>, active_thread: ThreadId, + condvar: CondvarId, mutex: MutexId, ) -> InterpResult<'tcx> { if let Some(old_locked_count) = ecx.mutex_unlock(mutex, active_thread) { @@ -265,7 +267,7 @@ fn release_cond_mutex_and_block<'mir, 'tcx: 'mir>( } else { throw_ub_format!("awaiting on unlocked or owned by a different thread mutex"); } - ecx.block_thread(active_thread); + ecx.block_thread(active_thread, BlockReason::Condvar(condvar)); Ok(()) } @@ -792,12 +794,8 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { fn pthread_cond_signal(&mut self, cond_op: &OpTy<'tcx, Provenance>) -> InterpResult<'tcx, i32> { let this = self.eval_context_mut(); let id = cond_get_id(this, cond_op)?; - if let Some((thread, lock)) = this.condvar_signal(id) { - if let CondvarLock::Mutex(mutex) = lock { - post_cond_signal(this, thread, mutex)?; - } else { - panic!("condvar should not have an rwlock on unix"); - } + if let Some((thread, mutex)) = this.condvar_signal(id) { + post_cond_signal(this, thread, id, mutex)?; } Ok(0) @@ -810,12 +808,8 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { let this = self.eval_context_mut(); let id = cond_get_id(this, cond_op)?; - while let Some((thread, lock)) = this.condvar_signal(id) { - if let CondvarLock::Mutex(mutex) = lock { - post_cond_signal(this, thread, mutex)?; - } else { - panic!("condvar should not have an rwlock on unix"); - } + while let Some((thread, mutex)) = this.condvar_signal(id) { + post_cond_signal(this, thread, id, mutex)?; } Ok(0) @@ -832,8 +826,8 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { let mutex_id = mutex_get_id(this, mutex_op)?; let active_thread = this.get_active_thread(); - release_cond_mutex_and_block(this, active_thread, mutex_id)?; - this.condvar_wait(id, active_thread, CondvarLock::Mutex(mutex_id)); + release_cond_mutex_and_block(this, active_thread, id, mutex_id)?; + this.condvar_wait(id, active_thread, mutex_id); Ok(0) } @@ -866,15 +860,15 @@ 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()) + CallbackTime::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()) + CallbackTime::Monotonic(this.machine.clock.anchor().checked_add(duration).unwrap()) } else { throw_unsup_format!("unsupported clock id: {}", clock_id); }; - release_cond_mutex_and_block(this, active_thread, mutex_id)?; - this.condvar_wait(id, active_thread, CondvarLock::Mutex(mutex_id)); + release_cond_mutex_and_block(this, active_thread, id, mutex_id)?; + this.condvar_wait(id, active_thread, mutex_id); // We return success for now and override it in the timeout callback. this.write_scalar(Scalar::from_i32(0), dest)?; @@ -897,7 +891,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { fn call(&self, ecx: &mut MiriInterpCx<'mir, 'tcx>) -> InterpResult<'tcx> { // We are not waiting for the condvar any more, wait for the // mutex instead. - reacquire_cond_mutex(ecx, self.active_thread, self.mutex_id)?; + reacquire_cond_mutex(ecx, self.active_thread, self.id, self.mutex_id)?; // Remove the thread from the conditional variable. ecx.condvar_remove_waiter(self.id, self.active_thread); diff --git a/src/tools/miri/src/shims/windows/sync.rs b/src/tools/miri/src/shims/windows/sync.rs index f02939f888e..836b9e92595 100644 --- a/src/tools/miri/src/shims/windows/sync.rs +++ b/src/tools/miri/src/shims/windows/sync.rs @@ -170,7 +170,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { None } else { let duration = Duration::from_millis(timeout_ms.into()); - Some(Time::Monotonic(this.machine.clock.now().checked_add(duration).unwrap())) + Some(CallbackTime::Monotonic(this.machine.clock.now().checked_add(duration).unwrap())) }; // See the Linux futex implementation for why this fence exists. @@ -183,7 +183,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { if futex_val == compare_val { // If the values are the same, we have to block. - this.block_thread(thread); + this.block_thread(thread, BlockReason::Futex { addr }); this.futex_wait(addr, thread, u32::MAX); if let Some(timeout_time) = timeout_time { @@ -202,7 +202,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { 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.unblock_thread(self.thread, BlockReason::Futex { addr: self.addr }); this.futex_remove_waiter(self.addr, self.thread); let error_timeout = this.eval_windows("c", "ERROR_TIMEOUT"); this.set_last_error(error_timeout)?; @@ -233,8 +233,9 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { // 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); + let addr = ptr.addr().bytes(); + if let Some(thread) = this.futex_wake(addr, u32::MAX) { + this.unblock_thread(thread, BlockReason::Futex { addr }); this.unregister_timeout_callback_if_exists(thread); } @@ -248,8 +249,9 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { // See the Linux futex implementation for why this fence exists. this.atomic_fence(AtomicFenceOrd::SeqCst)?; - while let Some(thread) = this.futex_wake(ptr.addr().bytes(), u32::MAX) { - this.unblock_thread(thread); + let addr = ptr.addr().bytes(); + while let Some(thread) = this.futex_wake(addr, u32::MAX) { + this.unblock_thread(thread, BlockReason::Futex { addr }); this.unregister_timeout_callback_if_exists(thread); } From 811d4de5a01348a47fc747796094a593b7e6bb73 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 16 Apr 2024 13:06:19 +0200 Subject: [PATCH 04/33] deadlock: show backtrace for all threads --- .../stacked_borrows/diagnostics.rs | 2 +- src/tools/miri/src/concurrency/thread.rs | 5 +- src/tools/miri/src/diagnostics.rs | 70 +++++++++++++++---- .../fail-dep/concurrency/windows_join_main.rs | 1 + .../concurrency/windows_join_main.stderr | 23 +++++- .../fail-dep/concurrency/windows_join_self.rs | 1 + .../concurrency/windows_join_self.stderr | 24 ++++++- .../shims/sync/libc_pthread_mutex_deadlock.rs | 1 + .../sync/libc_pthread_mutex_deadlock.stderr | 21 +++++- ...libc_pthread_rwlock_write_read_deadlock.rs | 1 + ..._pthread_rwlock_write_read_deadlock.stderr | 21 +++++- ...ibc_pthread_rwlock_write_write_deadlock.rs | 1 + ...pthread_rwlock_write_write_deadlock.stderr | 21 +++++- .../uninit/uninit_alloc_diagnostic.stderr | 4 +- ...it_alloc_diagnostic_with_provenance.stderr | 4 +- 15 files changed, 175 insertions(+), 25 deletions(-) diff --git a/src/tools/miri/src/borrow_tracker/stacked_borrows/diagnostics.rs b/src/tools/miri/src/borrow_tracker/stacked_borrows/diagnostics.rs index aa99a14b18e..5a941ae9d03 100644 --- a/src/tools/miri/src/borrow_tracker/stacked_borrows/diagnostics.rs +++ b/src/tools/miri/src/borrow_tracker/stacked_borrows/diagnostics.rs @@ -438,7 +438,7 @@ impl<'history, 'ecx, 'mir, 'tcx> DiagnosticCx<'history, 'ecx, 'mir, 'tcx> { .machine .threads .all_stacks() - .flatten() + .flat_map(|(_id, stack)| stack) .map(|frame| { frame.extra.borrow_tracker.as_ref().expect("we should have borrow tracking data") }) diff --git a/src/tools/miri/src/concurrency/thread.rs b/src/tools/miri/src/concurrency/thread.rs index d1136272f01..6b3dc0fa341 100644 --- a/src/tools/miri/src/concurrency/thread.rs +++ b/src/tools/miri/src/concurrency/thread.rs @@ -430,11 +430,10 @@ impl<'mir, 'tcx: 'mir> ThreadManager<'mir, 'tcx> { ) -> &mut Vec>> { &mut self.threads[self.active_thread].stack } - pub fn all_stacks( &self, - ) -> impl Iterator>]> { - self.threads.iter().map(|t| &t.stack[..]) + ) -> impl Iterator>])> { + self.threads.iter_enumerated().map(|(id, t)| (id, &t.stack[..])) } /// Create a new thread and returns its id. diff --git a/src/tools/miri/src/diagnostics.rs b/src/tools/miri/src/diagnostics.rs index 30349c003a9..dfbcaaac5c6 100644 --- a/src/tools/miri/src/diagnostics.rs +++ b/src/tools/miri/src/diagnostics.rs @@ -361,9 +361,12 @@ pub fn report_error<'tcx, 'mir>( }; let stacktrace = ecx.generate_stacktrace(); - let (stacktrace, was_pruned) = prune_stacktrace(stacktrace, &ecx.machine); + let (stacktrace, mut any_pruned) = prune_stacktrace(stacktrace, &ecx.machine); - // We want to dump the allocation if this is `InvalidUninitBytes`. Since `format_error` consumes `e`, we compute the outut early. + let mut show_all_threads = false; + + // We want to dump the allocation if this is `InvalidUninitBytes`. + // Since `format_interp_error` consumes `e`, we compute the outut early. let mut extra = String::new(); match e.kind() { UndefinedBehavior(InvalidUninitBytes(Some((alloc_id, access)))) => { @@ -375,6 +378,15 @@ pub fn report_error<'tcx, 'mir>( .unwrap(); writeln!(extra, "{:?}", ecx.dump_alloc(*alloc_id)).unwrap(); } + MachineStop(info) => { + let info = info.downcast_ref::().expect("invalid MachineStop payload"); + match info { + TerminationInfo::Deadlock => { + show_all_threads = true; + } + _ => {} + } + } _ => {} } @@ -387,18 +399,39 @@ pub fn report_error<'tcx, 'mir>( vec![], helps, &stacktrace, + Some(ecx.get_active_thread()), &ecx.machine, ); + eprint!("{extra}"); // newlines are already in the string + + if show_all_threads { + for (thread, stack) in ecx.machine.threads.all_stacks() { + if thread != ecx.get_active_thread() { + let stacktrace = Frame::generate_stacktrace_from_stack(stack); + let (stacktrace, was_pruned) = prune_stacktrace(stacktrace, &ecx.machine); + any_pruned |= was_pruned; + report_msg( + DiagLevel::Error, + format!("deadlock: the evaluated program deadlocked"), + vec![format!("the evaluated program deadlocked")], + vec![], + vec![], + &stacktrace, + Some(thread), + &ecx.machine, + ) + } + } + } + // Include a note like `std` does when we omit frames from a backtrace - if was_pruned { + if any_pruned { ecx.tcx.dcx().note( "some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace", ); } - eprint!("{extra}"); // newlines are already in the string - // Debug-dump all locals. for (i, frame) in ecx.active_thread_stack().iter().enumerate() { trace!("-------------------"); @@ -435,6 +468,7 @@ pub fn report_leaks<'mir, 'tcx>( vec![], vec![], &backtrace, + None, // we don't know the thread this is from &ecx.machine, ); } @@ -457,6 +491,7 @@ pub fn report_msg<'tcx>( notes: Vec<(Option, String)>, helps: Vec<(Option, String)>, stacktrace: &[FrameInfo<'tcx>], + thread: Option, machine: &MiriMachine<'_, 'tcx>, ) { let span = stacktrace.first().map_or(DUMMY_SP, |fi| fi.span); @@ -506,12 +541,13 @@ pub fn report_msg<'tcx>( if extra_span { write!(backtrace_title, " (of the first span)").unwrap(); } - let thread_name = - machine.threads.get_thread_display_name(machine.threads.get_active_thread_id()); - if thread_name != "main" { - // Only print thread name if it is not `main`. - write!(backtrace_title, " on thread `{thread_name}`").unwrap(); - }; + if let Some(thread) = thread { + let thread_name = machine.threads.get_thread_display_name(thread); + if thread_name != "main" { + // Only print thread name if it is not `main`. + write!(backtrace_title, " on thread `{thread_name}`").unwrap(); + }; + } write!(backtrace_title, ":").unwrap(); err.note(backtrace_title); for (idx, frame_info) in stacktrace.iter().enumerate() { @@ -628,7 +664,16 @@ impl<'mir, 'tcx> MiriMachine<'mir, 'tcx> { _ => vec![], }; - report_msg(diag_level, title, vec![msg], notes, helps, &stacktrace, self); + report_msg( + diag_level, + title, + vec![msg], + notes, + helps, + &stacktrace, + Some(self.threads.get_active_thread_id()), + self, + ); } } @@ -654,6 +699,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { vec![], vec![], &stacktrace, + Some(this.get_active_thread()), &this.machine, ); } diff --git a/src/tools/miri/tests/fail-dep/concurrency/windows_join_main.rs b/src/tools/miri/tests/fail-dep/concurrency/windows_join_main.rs index 910e06222ee..532bda20136 100644 --- a/src/tools/miri/tests/fail-dep/concurrency/windows_join_main.rs +++ b/src/tools/miri/tests/fail-dep/concurrency/windows_join_main.rs @@ -1,6 +1,7 @@ //@only-target-windows: Uses win32 api functions // We are making scheduler assumptions here. //@compile-flags: -Zmiri-preemption-rate=0 +//@error-in-other-file: deadlock // On windows, joining main is not UB, but it will block a thread forever. diff --git a/src/tools/miri/tests/fail-dep/concurrency/windows_join_main.stderr b/src/tools/miri/tests/fail-dep/concurrency/windows_join_main.stderr index d9137ee7437..12f35fdeb02 100644 --- a/src/tools/miri/tests/fail-dep/concurrency/windows_join_main.stderr +++ b/src/tools/miri/tests/fail-dep/concurrency/windows_join_main.stderr @@ -8,7 +8,28 @@ LL | assert_eq!(WaitForSingleObject(MAIN_THREAD, INFINITE), WAIT_OBJ = note: inside closure at RUSTLIB/core/src/macros/mod.rs:LL:CC = note: this error originates in the macro `assert_eq` (in Nightly builds, run with -Z macro-backtrace for more info) +error: deadlock: the evaluated program deadlocked + --> RUSTLIB/std/src/sys/pal/PLATFORM/thread.rs:LL:CC + | +LL | let rc = unsafe { c::WaitForSingleObject(self.handle.as_raw_handle(), c::INFINITE) }; + | ^ the evaluated program deadlocked + | + = note: BACKTRACE: + = note: inside `std::sys::pal::PLATFORM::thread::Thread::join` at RUSTLIB/std/src/sys/pal/PLATFORM/thread.rs:LL:CC + = note: inside `std::thread::JoinInner::<'_, ()>::join` at RUSTLIB/std/src/thread/mod.rs:LL:CC + = note: inside `std::thread::JoinHandle::<()>::join` at RUSTLIB/std/src/thread/mod.rs:LL:CC +note: inside `main` + --> $DIR/windows_join_main.rs:LL:CC + | +LL | / thread::spawn(|| { +LL | | unsafe { +LL | | assert_eq!(WaitForSingleObject(MAIN_THREAD, INFINITE), WAIT_OBJECT_0); +LL | | } +LL | | }) +LL | | .join() + | |___________^ + note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace -error: aborting due to 1 previous error +error: aborting due to 2 previous errors diff --git a/src/tools/miri/tests/fail-dep/concurrency/windows_join_self.rs b/src/tools/miri/tests/fail-dep/concurrency/windows_join_self.rs index a7c8faf5a98..a64265ca0ca 100644 --- a/src/tools/miri/tests/fail-dep/concurrency/windows_join_self.rs +++ b/src/tools/miri/tests/fail-dep/concurrency/windows_join_self.rs @@ -1,6 +1,7 @@ //@only-target-windows: Uses win32 api functions // We are making scheduler assumptions here. //@compile-flags: -Zmiri-preemption-rate=0 +//@error-in-other-file: deadlock // On windows, a thread joining itself is not UB, but it will deadlock. diff --git a/src/tools/miri/tests/fail-dep/concurrency/windows_join_self.stderr b/src/tools/miri/tests/fail-dep/concurrency/windows_join_self.stderr index 74699a0317f..8d26c35de8a 100644 --- a/src/tools/miri/tests/fail-dep/concurrency/windows_join_self.stderr +++ b/src/tools/miri/tests/fail-dep/concurrency/windows_join_self.stderr @@ -7,7 +7,29 @@ LL | assert_eq!(WaitForSingleObject(native, INFINITE), WAIT_OBJECT_0 = note: BACKTRACE on thread `unnamed-ID`: = note: inside closure at $DIR/windows_join_self.rs:LL:CC +error: deadlock: the evaluated program deadlocked + --> RUSTLIB/std/src/sys/pal/PLATFORM/thread.rs:LL:CC + | +LL | let rc = unsafe { c::WaitForSingleObject(self.handle.as_raw_handle(), c::INFINITE) }; + | ^ the evaluated program deadlocked + | + = note: BACKTRACE: + = note: inside `std::sys::pal::PLATFORM::thread::Thread::join` at RUSTLIB/std/src/sys/pal/PLATFORM/thread.rs:LL:CC + = note: inside `std::thread::JoinInner::<'_, ()>::join` at RUSTLIB/std/src/thread/mod.rs:LL:CC + = note: inside `std::thread::JoinHandle::<()>::join` at RUSTLIB/std/src/thread/mod.rs:LL:CC +note: inside `main` + --> $DIR/windows_join_self.rs:LL:CC + | +LL | / thread::spawn(|| { +LL | | unsafe { +LL | | let native = GetCurrentThread(); +LL | | assert_eq!(WaitForSingleObject(native, INFINITE), WAIT_OBJECT_0); +LL | | } +LL | | }) +LL | | .join() + | |___________^ + note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace -error: aborting due to 1 previous error +error: aborting due to 2 previous errors diff --git a/src/tools/miri/tests/fail-dep/shims/sync/libc_pthread_mutex_deadlock.rs b/src/tools/miri/tests/fail-dep/shims/sync/libc_pthread_mutex_deadlock.rs index 6c3cb738e29..60d56d41fd9 100644 --- a/src/tools/miri/tests/fail-dep/shims/sync/libc_pthread_mutex_deadlock.rs +++ b/src/tools/miri/tests/fail-dep/shims/sync/libc_pthread_mutex_deadlock.rs @@ -1,4 +1,5 @@ //@ignore-target-windows: No libc on Windows +//@error-in-other-file: deadlock use std::cell::UnsafeCell; use std::sync::Arc; diff --git a/src/tools/miri/tests/fail-dep/shims/sync/libc_pthread_mutex_deadlock.stderr b/src/tools/miri/tests/fail-dep/shims/sync/libc_pthread_mutex_deadlock.stderr index 76b1d26bd33..987d0fc4c2d 100644 --- a/src/tools/miri/tests/fail-dep/shims/sync/libc_pthread_mutex_deadlock.stderr +++ b/src/tools/miri/tests/fail-dep/shims/sync/libc_pthread_mutex_deadlock.stderr @@ -7,7 +7,26 @@ LL | assert_eq!(libc::pthread_mutex_lock(lock_copy.0.get() as *mut _ = note: BACKTRACE on thread `unnamed-ID`: = note: inside closure at $DIR/libc_pthread_mutex_deadlock.rs:LL:CC +error: deadlock: the evaluated program deadlocked + --> RUSTLIB/std/src/sys/pal/PLATFORM/thread.rs:LL:CC + | +LL | let ret = libc::pthread_join(self.id, ptr::null_mut()); + | ^ the evaluated program deadlocked + | + = note: BACKTRACE: + = note: inside `std::sys::pal::PLATFORM::thread::Thread::join` at RUSTLIB/std/src/sys/pal/PLATFORM/thread.rs:LL:CC + = note: inside `std::thread::JoinInner::<'_, ()>::join` at RUSTLIB/std/src/thread/mod.rs:LL:CC + = note: inside `std::thread::JoinHandle::<()>::join` at RUSTLIB/std/src/thread/mod.rs:LL:CC +note: inside `main` + --> $DIR/libc_pthread_mutex_deadlock.rs:LL:CC + | +LL | / thread::spawn(move || { +LL | | assert_eq!(libc::pthread_mutex_lock(lock_copy.0.get() as *mut _), 0); +LL | | }) +LL | | .join() + | |_______________^ + note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace -error: aborting due to 1 previous error +error: aborting due to 2 previous errors diff --git a/src/tools/miri/tests/fail-dep/shims/sync/libc_pthread_rwlock_write_read_deadlock.rs b/src/tools/miri/tests/fail-dep/shims/sync/libc_pthread_rwlock_write_read_deadlock.rs index 201844615e1..0f02c3231a6 100644 --- a/src/tools/miri/tests/fail-dep/shims/sync/libc_pthread_rwlock_write_read_deadlock.rs +++ b/src/tools/miri/tests/fail-dep/shims/sync/libc_pthread_rwlock_write_read_deadlock.rs @@ -1,4 +1,5 @@ //@ignore-target-windows: No libc on Windows +//@error-in-other-file: deadlock use std::cell::UnsafeCell; use std::sync::Arc; diff --git a/src/tools/miri/tests/fail-dep/shims/sync/libc_pthread_rwlock_write_read_deadlock.stderr b/src/tools/miri/tests/fail-dep/shims/sync/libc_pthread_rwlock_write_read_deadlock.stderr index 5501dab81ac..bc9b15f293e 100644 --- a/src/tools/miri/tests/fail-dep/shims/sync/libc_pthread_rwlock_write_read_deadlock.stderr +++ b/src/tools/miri/tests/fail-dep/shims/sync/libc_pthread_rwlock_write_read_deadlock.stderr @@ -7,7 +7,26 @@ LL | assert_eq!(libc::pthread_rwlock_wrlock(lock_copy.0.get() as *mu = note: BACKTRACE on thread `unnamed-ID`: = note: inside closure at $DIR/libc_pthread_rwlock_write_read_deadlock.rs:LL:CC +error: deadlock: the evaluated program deadlocked + --> RUSTLIB/std/src/sys/pal/PLATFORM/thread.rs:LL:CC + | +LL | let ret = libc::pthread_join(self.id, ptr::null_mut()); + | ^ the evaluated program deadlocked + | + = note: BACKTRACE: + = note: inside `std::sys::pal::PLATFORM::thread::Thread::join` at RUSTLIB/std/src/sys/pal/PLATFORM/thread.rs:LL:CC + = note: inside `std::thread::JoinInner::<'_, ()>::join` at RUSTLIB/std/src/thread/mod.rs:LL:CC + = note: inside `std::thread::JoinHandle::<()>::join` at RUSTLIB/std/src/thread/mod.rs:LL:CC +note: inside `main` + --> $DIR/libc_pthread_rwlock_write_read_deadlock.rs:LL:CC + | +LL | / thread::spawn(move || { +LL | | assert_eq!(libc::pthread_rwlock_wrlock(lock_copy.0.get() as *mut _), 0); +LL | | }) +LL | | .join() + | |_______________^ + note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace -error: aborting due to 1 previous error +error: aborting due to 2 previous errors diff --git a/src/tools/miri/tests/fail-dep/shims/sync/libc_pthread_rwlock_write_write_deadlock.rs b/src/tools/miri/tests/fail-dep/shims/sync/libc_pthread_rwlock_write_write_deadlock.rs index b1d7e0492e5..10be5b33752 100644 --- a/src/tools/miri/tests/fail-dep/shims/sync/libc_pthread_rwlock_write_write_deadlock.rs +++ b/src/tools/miri/tests/fail-dep/shims/sync/libc_pthread_rwlock_write_write_deadlock.rs @@ -1,4 +1,5 @@ //@ignore-target-windows: No libc on Windows +//@error-in-other-file: deadlock use std::cell::UnsafeCell; use std::sync::Arc; diff --git a/src/tools/miri/tests/fail-dep/shims/sync/libc_pthread_rwlock_write_write_deadlock.stderr b/src/tools/miri/tests/fail-dep/shims/sync/libc_pthread_rwlock_write_write_deadlock.stderr index 815d85af502..66c142bbc5c 100644 --- a/src/tools/miri/tests/fail-dep/shims/sync/libc_pthread_rwlock_write_write_deadlock.stderr +++ b/src/tools/miri/tests/fail-dep/shims/sync/libc_pthread_rwlock_write_write_deadlock.stderr @@ -7,7 +7,26 @@ LL | assert_eq!(libc::pthread_rwlock_wrlock(lock_copy.0.get() as *mu = note: BACKTRACE on thread `unnamed-ID`: = note: inside closure at $DIR/libc_pthread_rwlock_write_write_deadlock.rs:LL:CC +error: deadlock: the evaluated program deadlocked + --> RUSTLIB/std/src/sys/pal/PLATFORM/thread.rs:LL:CC + | +LL | let ret = libc::pthread_join(self.id, ptr::null_mut()); + | ^ the evaluated program deadlocked + | + = note: BACKTRACE: + = note: inside `std::sys::pal::PLATFORM::thread::Thread::join` at RUSTLIB/std/src/sys/pal/PLATFORM/thread.rs:LL:CC + = note: inside `std::thread::JoinInner::<'_, ()>::join` at RUSTLIB/std/src/thread/mod.rs:LL:CC + = note: inside `std::thread::JoinHandle::<()>::join` at RUSTLIB/std/src/thread/mod.rs:LL:CC +note: inside `main` + --> $DIR/libc_pthread_rwlock_write_write_deadlock.rs:LL:CC + | +LL | / thread::spawn(move || { +LL | | assert_eq!(libc::pthread_rwlock_wrlock(lock_copy.0.get() as *mut _), 0); +LL | | }) +LL | | .join() + | |_______________^ + note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace -error: aborting due to 1 previous error +error: aborting due to 2 previous errors diff --git a/src/tools/miri/tests/fail/uninit/uninit_alloc_diagnostic.stderr b/src/tools/miri/tests/fail/uninit/uninit_alloc_diagnostic.stderr index cca17a07ec2..960cae90124 100644 --- a/src/tools/miri/tests/fail/uninit/uninit_alloc_diagnostic.stderr +++ b/src/tools/miri/tests/fail/uninit/uninit_alloc_diagnostic.stderr @@ -15,13 +15,13 @@ note: inside `main` LL | drop(slice1.cmp(slice2)); | ^^^^^^^^^^^^^^^^^^ -note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace - Uninitialized memory occurred at ALLOC[0x4..0x10], in this allocation: ALLOC (Rust heap, size: 32, align: 8) { 0x00 │ 41 42 43 44 __ __ __ __ __ __ __ __ __ __ __ __ │ ABCD░░░░░░░░░░░░ 0x10 │ 00 __ __ __ __ __ __ __ __ __ __ __ __ __ __ __ │ .░░░░░░░░░░░░░░░ } +note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace + error: aborting due to 1 previous error diff --git a/src/tools/miri/tests/fail/uninit/uninit_alloc_diagnostic_with_provenance.stderr b/src/tools/miri/tests/fail/uninit/uninit_alloc_diagnostic_with_provenance.stderr index 4dc2d27ead4..5439418f267 100644 --- a/src/tools/miri/tests/fail/uninit/uninit_alloc_diagnostic_with_provenance.stderr +++ b/src/tools/miri/tests/fail/uninit/uninit_alloc_diagnostic_with_provenance.stderr @@ -15,8 +15,6 @@ note: inside `main` LL | drop(slice1.cmp(slice2)); | ^^^^^^^^^^^^^^^^^^ -note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace - Uninitialized memory occurred at ALLOC[0x4..0x8], in this allocation: ALLOC (Rust heap, size: 16, align: 8) { ╾42[ALLOC] (1 ptr byte)╼ 12 13 ╾43[ALLOC] (1 ptr byte)╼ __ __ __ __ __ __ __ __ __ __ __ __ │ ━..━░░░░░░░░░░░░ @@ -28,5 +26,7 @@ ALLOC (global (static or const), size: 1, align: 1) { 00 │ . } +note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace + error: aborting due to 1 previous error From a7db62810ee19463b3fc22bd88bad3433b389757 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 16 Apr 2024 21:46:18 +0200 Subject: [PATCH 05/33] no_std works on Windows now --- src/tools/miri/tests/fail/panic/no_std.rs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/tools/miri/tests/fail/panic/no_std.rs b/src/tools/miri/tests/fail/panic/no_std.rs index bad425804dc..26cc0b27821 100644 --- a/src/tools/miri/tests/fail/panic/no_std.rs +++ b/src/tools/miri/tests/fail/panic/no_std.rs @@ -1,10 +1,6 @@ #![feature(start, core_intrinsics)] #![no_std] //@compile-flags: -Cpanic=abort -// 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: no-std not supported on Windows // Plumbing to let us use `writeln!` to host stderr: From f325c8d4faca7e4885f7ce1d8ea92063eeecbd0c Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 16 Apr 2024 21:59:19 +0200 Subject: [PATCH 06/33] implement support for __rust_alloc_error_handler --- src/tools/miri/src/shims/extern_static.rs | 7 +- src/tools/miri/src/shims/foreign_items.rs | 16 ++++- .../tests/fail/alloc/alloc_error_handler.rs | 25 +++++++ .../fail/alloc/alloc_error_handler.stderr | 26 ++++++++ .../fail/alloc/alloc_error_handler_no_std.rs | 65 +++++++++++++++++++ .../alloc/alloc_error_handler_no_std.stderr | 29 +++++++++ .../tests/panic/alloc_error_handler_panic.rs | 32 +++++++++ .../panic/alloc_error_handler_panic.stderr | 4 ++ .../miri/tests/pass/alloc-access-tracking.rs | 2 +- .../tests/pass/alloc-access-tracking.stderr | 8 +-- 10 files changed, 207 insertions(+), 7 deletions(-) create mode 100644 src/tools/miri/tests/fail/alloc/alloc_error_handler.rs create mode 100644 src/tools/miri/tests/fail/alloc/alloc_error_handler.stderr create mode 100644 src/tools/miri/tests/fail/alloc/alloc_error_handler_no_std.rs create mode 100644 src/tools/miri/tests/fail/alloc/alloc_error_handler_no_std.stderr create mode 100644 src/tools/miri/tests/panic/alloc_error_handler_panic.rs create mode 100644 src/tools/miri/tests/panic/alloc_error_handler_panic.stderr diff --git a/src/tools/miri/src/shims/extern_static.rs b/src/tools/miri/src/shims/extern_static.rs index 0284e5b606c..7c4a54fb461 100644 --- a/src/tools/miri/src/shims/extern_static.rs +++ b/src/tools/miri/src/shims/extern_static.rs @@ -32,9 +32,14 @@ impl<'mir, 'tcx> MiriMachine<'mir, 'tcx> { /// Sets up the "extern statics" for this machine. pub fn init_extern_statics(this: &mut MiriInterpCx<'mir, 'tcx>) -> InterpResult<'tcx> { // "__rust_no_alloc_shim_is_unstable" - let val = ImmTy::from_int(0, this.machine.layouts.u8); + let val = ImmTy::from_int(0, this.machine.layouts.u8); // always 0, value does not matter Self::alloc_extern_static(this, "__rust_no_alloc_shim_is_unstable", val)?; + // "__rust_alloc_error_handler_should_panic" + let val = this.tcx.sess.opts.unstable_opts.oom.should_panic(); + let val = ImmTy::from_int(val, this.machine.layouts.u8); + Self::alloc_extern_static(this, "__rust_alloc_error_handler_should_panic", val)?; + match this.tcx.sess.target.os.as_ref() { "linux" => { Self::null_ptr_extern_statics( diff --git a/src/tools/miri/src/shims/foreign_items.rs b/src/tools/miri/src/shims/foreign_items.rs index 6b0797f6da1..e6fc29a5ae0 100644 --- a/src/tools/miri/src/shims/foreign_items.rs +++ b/src/tools/miri/src/shims/foreign_items.rs @@ -1,7 +1,7 @@ use std::{collections::hash_map::Entry, io::Write, iter, path::Path}; use rustc_apfloat::Float; -use rustc_ast::expand::allocator::AllocatorKind; +use rustc_ast::expand::allocator::{alloc_error_handler_name, AllocatorKind}; use rustc_hir::{def::DefKind, def_id::CrateNum}; use rustc_middle::middle::codegen_fn_attrs::CodegenFnAttrFlags; use rustc_middle::mir; @@ -80,6 +80,20 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { panic_impl_instance, ))); } + "__rust_alloc_error_handler" => { + // Forward to the right symbol that implements this function. + let Some(handler_kind) = this.tcx.alloc_error_handler_kind(()) else { + // in real code, this symbol does not exist without an allocator + throw_unsup_format!( + "`__rust_alloc_error_handler` cannot be called when no alloc error handler is set" + ); + }; + let name = alloc_error_handler_name(handler_kind); + let handler = this + .lookup_exported_symbol(Symbol::intern(name))? + .expect("missing alloc error handler symbol"); + return Ok(Some(handler)); + } #[rustfmt::skip] | "exit" | "ExitProcess" diff --git a/src/tools/miri/tests/fail/alloc/alloc_error_handler.rs b/src/tools/miri/tests/fail/alloc/alloc_error_handler.rs new file mode 100644 index 00000000000..dc8e8c73800 --- /dev/null +++ b/src/tools/miri/tests/fail/alloc/alloc_error_handler.rs @@ -0,0 +1,25 @@ +//@error-in-other-file: aborted +//@normalize-stderr-test: "unsafe \{ libc::abort\(\) \}|crate::intrinsics::abort\(\);" -> "ABORT();" +//@normalize-stderr-test: "\| +\^+" -> "| ^" +#![feature(allocator_api)] + +use std::alloc::*; +use std::ptr::NonNull; + +struct BadAlloc; + +// Create a failing allocator; Miri's native allocator never fails so this is the only way to +// actually call the alloc error handler. +unsafe impl Allocator for BadAlloc { + fn allocate(&self, _l: Layout) -> Result, AllocError> { + Err(AllocError) + } + + unsafe fn deallocate(&self, _ptr: NonNull, _layout: Layout) { + unreachable!(); + } +} + +fn main() { + let _b = Box::new_in(0, BadAlloc); +} diff --git a/src/tools/miri/tests/fail/alloc/alloc_error_handler.stderr b/src/tools/miri/tests/fail/alloc/alloc_error_handler.stderr new file mode 100644 index 00000000000..f9d8e80c0fa --- /dev/null +++ b/src/tools/miri/tests/fail/alloc/alloc_error_handler.stderr @@ -0,0 +1,26 @@ +memory allocation of 4 bytes failed +error: abnormal termination: the program aborted execution + --> RUSTLIB/std/src/sys/pal/PLATFORM/mod.rs:LL:CC + | +LL | ABORT(); + | ^ the program aborted execution + | + = note: BACKTRACE: + = note: inside `std::sys::pal::PLATFORM::abort_internal` at RUSTLIB/std/src/sys/pal/PLATFORM/mod.rs:LL:CC + = note: inside `std::process::abort` at RUSTLIB/std/src/process.rs:LL:CC + = note: inside `std::alloc::rust_oom` at RUSTLIB/std/src/alloc.rs:LL:CC + = note: inside `std::alloc::_::__rg_oom` at RUSTLIB/std/src/alloc.rs:LL:CC + = note: inside `std::alloc::handle_alloc_error::rt_error` at RUSTLIB/alloc/src/alloc.rs:LL:CC + = note: inside `std::alloc::handle_alloc_error` at RUSTLIB/alloc/src/alloc.rs:LL:CC + = note: inside `std::boxed::Box::::new_uninit_in` at RUSTLIB/alloc/src/boxed.rs:LL:CC + = note: inside `std::boxed::Box::::new_in` at RUSTLIB/alloc/src/boxed.rs:LL:CC +note: inside `main` + --> $DIR/alloc_error_handler.rs:LL:CC + | +LL | let _b = Box::new_in(0, BadAlloc); + | ^ + +note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace + +error: aborting due to 1 previous error + diff --git a/src/tools/miri/tests/fail/alloc/alloc_error_handler_no_std.rs b/src/tools/miri/tests/fail/alloc/alloc_error_handler_no_std.rs new file mode 100644 index 00000000000..8103296f47b --- /dev/null +++ b/src/tools/miri/tests/fail/alloc/alloc_error_handler_no_std.rs @@ -0,0 +1,65 @@ +//@compile-flags: -Cpanic=abort +#![feature(start, core_intrinsics)] +#![feature(alloc_error_handler)] +#![feature(allocator_api)] +#![no_std] + +extern crate alloc; + +use alloc::alloc::*; +use alloc::boxed::Box; +use core::ptr::NonNull; + +struct BadAlloc; + +// Create a failing allocator; that is the only way to actually call the alloc error handler. +unsafe impl Allocator for BadAlloc { + fn allocate(&self, _l: Layout) -> Result, AllocError> { + Err(AllocError) + } + + unsafe fn deallocate(&self, _ptr: NonNull, _layout: Layout) { + unreachable!(); + } +} + +#[alloc_error_handler] +fn alloc_error_handler(_: Layout) -> ! { + extern "Rust" { + fn miri_write_to_stderr(bytes: &[u8]); + } + let msg = "custom alloc error handler called!\n"; + unsafe { miri_write_to_stderr(msg.as_bytes()) }; + core::intrinsics::abort(); //~ERROR: aborted +} + +// rustc requires us to provide some more things that aren't actually used by this test +mod plumbing { + use super::*; + + #[panic_handler] + fn panic_handler(_: &core::panic::PanicInfo) -> ! { + core::intrinsics::abort(); + } + + struct NoAlloc; + + unsafe impl GlobalAlloc for NoAlloc { + unsafe fn alloc(&self, _: Layout) -> *mut u8 { + unreachable!(); + } + + unsafe fn dealloc(&self, _: *mut u8, _: Layout) { + unreachable!(); + } + } + + #[global_allocator] + static GLOBAL: NoAlloc = NoAlloc; +} + +#[start] +fn start(_: isize, _: *const *const u8) -> isize { + let _b = Box::new_in(0, BadAlloc); + 0 +} diff --git a/src/tools/miri/tests/fail/alloc/alloc_error_handler_no_std.stderr b/src/tools/miri/tests/fail/alloc/alloc_error_handler_no_std.stderr new file mode 100644 index 00000000000..b40ffb70121 --- /dev/null +++ b/src/tools/miri/tests/fail/alloc/alloc_error_handler_no_std.stderr @@ -0,0 +1,29 @@ +custom alloc error handler called! +error: abnormal termination: the program aborted execution + --> $DIR/alloc_error_handler_no_std.rs:LL:CC + | +LL | core::intrinsics::abort(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^ the program aborted execution + | + = note: BACKTRACE: + = note: inside `alloc_error_handler` at $DIR/alloc_error_handler_no_std.rs:LL:CC +note: inside `_::__rg_oom` + --> $DIR/alloc_error_handler_no_std.rs:LL:CC + | +LL | #[alloc_error_handler] + | ---------------------- in this procedural macro expansion +LL | fn alloc_error_handler(_: Layout) -> ! { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + = note: inside `alloc::alloc::handle_alloc_error::rt_error` at RUSTLIB/alloc/src/alloc.rs:LL:CC + = note: inside `alloc::alloc::handle_alloc_error` at RUSTLIB/alloc/src/alloc.rs:LL:CC + = note: inside `alloc::boxed::Box::::new_uninit_in` at RUSTLIB/alloc/src/boxed.rs:LL:CC + = note: inside `alloc::boxed::Box::::new_in` at RUSTLIB/alloc/src/boxed.rs:LL:CC +note: inside `start` + --> $DIR/alloc_error_handler_no_std.rs:LL:CC + | +LL | let _b = Box::new_in(0, BadAlloc); + | ^^^^^^^^^^^^^^^^^^^^^^^^ + = note: this error originates in the attribute macro `alloc_error_handler` (in Nightly builds, run with -Z macro-backtrace for more info) + +error: aborting due to 1 previous error + diff --git a/src/tools/miri/tests/panic/alloc_error_handler_panic.rs b/src/tools/miri/tests/panic/alloc_error_handler_panic.rs new file mode 100644 index 00000000000..186c9667a97 --- /dev/null +++ b/src/tools/miri/tests/panic/alloc_error_handler_panic.rs @@ -0,0 +1,32 @@ +//@compile-flags: -Zoom=panic +#![feature(allocator_api)] + +use std::alloc::*; +use std::ptr::NonNull; + +struct BadAlloc; + +// Create a failing allocator; Miri's native allocator never fails so this is the only way to +// actually call the alloc error handler. +unsafe impl Allocator for BadAlloc { + fn allocate(&self, _l: Layout) -> Result, AllocError> { + Err(AllocError) + } + + unsafe fn deallocate(&self, _ptr: NonNull, _layout: Layout) { + unreachable!(); + } +} + +struct Bomb; +impl Drop for Bomb { + fn drop(&mut self) { + eprintln!("yes we are unwinding!"); + } +} + +fn main() { + let bomb = Bomb; + let _b = Box::new_in(0, BadAlloc); + std::mem::forget(bomb); // defuse unwinding bomb +} diff --git a/src/tools/miri/tests/panic/alloc_error_handler_panic.stderr b/src/tools/miri/tests/panic/alloc_error_handler_panic.stderr new file mode 100644 index 00000000000..202325468bf --- /dev/null +++ b/src/tools/miri/tests/panic/alloc_error_handler_panic.stderr @@ -0,0 +1,4 @@ +thread 'main' panicked at RUSTLIB/std/src/alloc.rs:LL:CC: +memory allocation of 4 bytes failed +note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace +yes we are unwinding! diff --git a/src/tools/miri/tests/pass/alloc-access-tracking.rs b/src/tools/miri/tests/pass/alloc-access-tracking.rs index 5c782fca2df..29c1ee2f7b7 100644 --- a/src/tools/miri/tests/pass/alloc-access-tracking.rs +++ b/src/tools/miri/tests/pass/alloc-access-tracking.rs @@ -1,6 +1,6 @@ #![feature(start)] #![no_std] -//@compile-flags: -Zmiri-track-alloc-id=17 -Zmiri-track-alloc-accesses -Cpanic=abort +//@compile-flags: -Zmiri-track-alloc-id=18 -Zmiri-track-alloc-accesses -Cpanic=abort //@only-target-linux: alloc IDs differ between OSes for some reason extern "Rust" { diff --git a/src/tools/miri/tests/pass/alloc-access-tracking.stderr b/src/tools/miri/tests/pass/alloc-access-tracking.stderr index 5e219fa1bed..bef13701ea2 100644 --- a/src/tools/miri/tests/pass/alloc-access-tracking.stderr +++ b/src/tools/miri/tests/pass/alloc-access-tracking.stderr @@ -2,7 +2,7 @@ note: tracking was triggered --> $DIR/alloc-access-tracking.rs:LL:CC | LL | let ptr = miri_alloc(123, 1); - | ^^^^^^^^^^^^^^^^^^ created Miri bare-metal heap allocation of 123 bytes (alignment ALIGN bytes) with id 17 + | ^^^^^^^^^^^^^^^^^^ created Miri bare-metal heap allocation of 123 bytes (alignment ALIGN bytes) with id 18 | = note: BACKTRACE: = note: inside `start` at $DIR/alloc-access-tracking.rs:LL:CC @@ -11,7 +11,7 @@ note: tracking was triggered --> $DIR/alloc-access-tracking.rs:LL:CC | LL | *ptr = 42; // Crucially, only a write is printed here, no read! - | ^^^^^^^^^ write access to allocation with id 17 + | ^^^^^^^^^ write access to allocation with id 18 | = note: BACKTRACE: = note: inside `start` at $DIR/alloc-access-tracking.rs:LL:CC @@ -20,7 +20,7 @@ note: tracking was triggered --> $DIR/alloc-access-tracking.rs:LL:CC | LL | assert_eq!(*ptr, 42); - | ^^^^^^^^^^^^^^^^^^^^ read access to allocation with id 17 + | ^^^^^^^^^^^^^^^^^^^^ read access to allocation with id 18 | = note: BACKTRACE: = note: inside `start` at RUSTLIB/core/src/macros/mod.rs:LL:CC @@ -30,7 +30,7 @@ note: tracking was triggered --> $DIR/alloc-access-tracking.rs:LL:CC | LL | miri_dealloc(ptr, 123, 1); - | ^^^^^^^^^^^^^^^^^^^^^^^^^ freed allocation with id 17 + | ^^^^^^^^^^^^^^^^^^^^^^^^^ freed allocation with id 18 | = note: BACKTRACE: = note: inside `start` at $DIR/alloc-access-tracking.rs:LL:CC From af28716f190813e6adb82018401aa99c075f6f22 Mon Sep 17 00:00:00 2001 From: The Miri Cronjob Bot Date: Wed, 17 Apr 2024 04:57:09 +0000 Subject: [PATCH 07/33] Preparing for merge from rustc --- src/tools/miri/rust-version | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tools/miri/rust-version b/src/tools/miri/rust-version index dfa7f8ca507..bd87405da3d 100644 --- a/src/tools/miri/rust-version +++ b/src/tools/miri/rust-version @@ -1 +1 @@ -63f70b3d104e20289a1a0df82747066c3d85b9a1 +803e33a4460c82581bd01d4008d0f44aef1ddfe8 From 2cb03ef7394bbcbdefea6dfb13a4d7c0458992fe Mon Sep 17 00:00:00 2001 From: The Miri Cronjob Bot Date: Wed, 17 Apr 2024 05:09:14 +0000 Subject: [PATCH 08/33] fmt --- .../pass/stacked-borrows/stacked-borrows.rs | 22 ++++++++++--------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/src/tools/miri/tests/pass/stacked-borrows/stacked-borrows.rs b/src/tools/miri/tests/pass/stacked-borrows/stacked-borrows.rs index 43ba490d5bb..c75824d7f9b 100644 --- a/src/tools/miri/tests/pass/stacked-borrows/stacked-borrows.rs +++ b/src/tools/miri/tests/pass/stacked-borrows/stacked-borrows.rs @@ -265,13 +265,15 @@ fn write_does_not_invalidate_all_aliases() { assert_eq!(*x, 1337); // oops, the value changed! I guess not all pointers were invalidated } -fn box_into_raw_allows_interior_mutable_alias() { unsafe { - let b = Box::new(std::cell::Cell::new(42)); - let raw = Box::into_raw(b); - let c = &*raw; - let d = raw.cast::(); // bypassing `Cell` -- only okay in Miri tests - // `c` and `d` should permit arbitrary aliasing with each other now. - *d = 1; - c.set(2); - drop(Box::from_raw(raw)); -} } +fn box_into_raw_allows_interior_mutable_alias() { + unsafe { + let b = Box::new(std::cell::Cell::new(42)); + let raw = Box::into_raw(b); + let c = &*raw; + let d = raw.cast::(); // bypassing `Cell` -- only okay in Miri tests + // `c` and `d` should permit arbitrary aliasing with each other now. + *d = 1; + c.set(2); + drop(Box::from_raw(raw)); + } +} From d10f61313f08db014cb76e96872eb056058a0792 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Wed, 17 Apr 2024 09:25:26 +0200 Subject: [PATCH 09/33] alloc_error_handler tests: directly call handle_alloc_error; test more codepaths --- .../tests/fail/alloc/alloc_error_handler.rs | 17 +----- .../fail/alloc/alloc_error_handler.stderr | 4 +- .../fail/alloc/alloc_error_handler_custom.rs | 52 +++++++++++++++++++ .../alloc/alloc_error_handler_custom.stderr | 27 ++++++++++ .../fail/alloc/alloc_error_handler_no_std.rs | 34 +++--------- .../alloc/alloc_error_handler_no_std.stderr | 21 +++----- .../tests/panic/alloc_error_handler_hook.rs | 20 +++++++ .../panic/alloc_error_handler_hook.stderr | 4 ++ .../tests/panic/alloc_error_handler_panic.rs | 18 +------ 9 files changed, 122 insertions(+), 75 deletions(-) create mode 100644 src/tools/miri/tests/fail/alloc/alloc_error_handler_custom.rs create mode 100644 src/tools/miri/tests/fail/alloc/alloc_error_handler_custom.stderr create mode 100644 src/tools/miri/tests/panic/alloc_error_handler_hook.rs create mode 100644 src/tools/miri/tests/panic/alloc_error_handler_hook.stderr diff --git a/src/tools/miri/tests/fail/alloc/alloc_error_handler.rs b/src/tools/miri/tests/fail/alloc/alloc_error_handler.rs index dc8e8c73800..2097126e16b 100644 --- a/src/tools/miri/tests/fail/alloc/alloc_error_handler.rs +++ b/src/tools/miri/tests/fail/alloc/alloc_error_handler.rs @@ -4,22 +4,7 @@ #![feature(allocator_api)] use std::alloc::*; -use std::ptr::NonNull; - -struct BadAlloc; - -// Create a failing allocator; Miri's native allocator never fails so this is the only way to -// actually call the alloc error handler. -unsafe impl Allocator for BadAlloc { - fn allocate(&self, _l: Layout) -> Result, AllocError> { - Err(AllocError) - } - - unsafe fn deallocate(&self, _ptr: NonNull, _layout: Layout) { - unreachable!(); - } -} fn main() { - let _b = Box::new_in(0, BadAlloc); + handle_alloc_error(Layout::for_value(&0)); } diff --git a/src/tools/miri/tests/fail/alloc/alloc_error_handler.stderr b/src/tools/miri/tests/fail/alloc/alloc_error_handler.stderr index f9d8e80c0fa..d1731a0f420 100644 --- a/src/tools/miri/tests/fail/alloc/alloc_error_handler.stderr +++ b/src/tools/miri/tests/fail/alloc/alloc_error_handler.stderr @@ -12,12 +12,10 @@ LL | ABORT(); = note: inside `std::alloc::_::__rg_oom` at RUSTLIB/std/src/alloc.rs:LL:CC = note: inside `std::alloc::handle_alloc_error::rt_error` at RUSTLIB/alloc/src/alloc.rs:LL:CC = note: inside `std::alloc::handle_alloc_error` at RUSTLIB/alloc/src/alloc.rs:LL:CC - = note: inside `std::boxed::Box::::new_uninit_in` at RUSTLIB/alloc/src/boxed.rs:LL:CC - = note: inside `std::boxed::Box::::new_in` at RUSTLIB/alloc/src/boxed.rs:LL:CC note: inside `main` --> $DIR/alloc_error_handler.rs:LL:CC | -LL | let _b = Box::new_in(0, BadAlloc); +LL | handle_alloc_error(Layout::for_value(&0)); | ^ note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace diff --git a/src/tools/miri/tests/fail/alloc/alloc_error_handler_custom.rs b/src/tools/miri/tests/fail/alloc/alloc_error_handler_custom.rs new file mode 100644 index 00000000000..9f8837a626f --- /dev/null +++ b/src/tools/miri/tests/fail/alloc/alloc_error_handler_custom.rs @@ -0,0 +1,52 @@ +//@compile-flags: -Cpanic=abort +#![feature(start, core_intrinsics)] +#![feature(alloc_error_handler)] +#![feature(allocator_api)] +#![no_std] + +extern crate alloc; + +use alloc::alloc::*; + +extern "Rust" { + fn miri_write_to_stderr(bytes: &[u8]); +} + +#[alloc_error_handler] +fn alloc_error_handler(_: Layout) -> ! { + let msg = "custom alloc error handler called!\n"; + unsafe { miri_write_to_stderr(msg.as_bytes()) }; + core::intrinsics::abort(); //~ERROR: aborted +} + +// rustc requires us to provide some more things that aren't actually used by this test +mod plumbing { + use super::*; + + #[panic_handler] + fn panic_handler(_: &core::panic::PanicInfo) -> ! { + let msg = "custom panic handler called!\n"; + unsafe { miri_write_to_stderr(msg.as_bytes()) }; + core::intrinsics::abort(); + } + + struct NoAlloc; + + unsafe impl GlobalAlloc for NoAlloc { + unsafe fn alloc(&self, _: Layout) -> *mut u8 { + unreachable!(); + } + + unsafe fn dealloc(&self, _: *mut u8, _: Layout) { + unreachable!(); + } + } + + #[global_allocator] + static GLOBAL: NoAlloc = NoAlloc; +} + +#[start] +fn start(_: isize, _: *const *const u8) -> isize { + handle_alloc_error(Layout::for_value(&0)); +} diff --git a/src/tools/miri/tests/fail/alloc/alloc_error_handler_custom.stderr b/src/tools/miri/tests/fail/alloc/alloc_error_handler_custom.stderr new file mode 100644 index 00000000000..cdd666e5e0a --- /dev/null +++ b/src/tools/miri/tests/fail/alloc/alloc_error_handler_custom.stderr @@ -0,0 +1,27 @@ +custom alloc error handler called! +error: abnormal termination: the program aborted execution + --> $DIR/alloc_error_handler_custom.rs:LL:CC + | +LL | core::intrinsics::abort(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^ the program aborted execution + | + = note: BACKTRACE: + = note: inside `alloc_error_handler` at $DIR/alloc_error_handler_custom.rs:LL:CC +note: inside `_::__rg_oom` + --> $DIR/alloc_error_handler_custom.rs:LL:CC + | +LL | #[alloc_error_handler] + | ---------------------- in this procedural macro expansion +LL | fn alloc_error_handler(_: Layout) -> ! { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + = note: inside `alloc::alloc::handle_alloc_error::rt_error` at RUSTLIB/alloc/src/alloc.rs:LL:CC + = note: inside `alloc::alloc::handle_alloc_error` at RUSTLIB/alloc/src/alloc.rs:LL:CC +note: inside `start` + --> $DIR/alloc_error_handler_custom.rs:LL:CC + | +LL | handle_alloc_error(Layout::for_value(&0)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + = note: this error originates in the attribute macro `alloc_error_handler` (in Nightly builds, run with -Z macro-backtrace for more info) + +error: aborting due to 1 previous error + diff --git a/src/tools/miri/tests/fail/alloc/alloc_error_handler_no_std.rs b/src/tools/miri/tests/fail/alloc/alloc_error_handler_no_std.rs index 8103296f47b..e3949b889f9 100644 --- a/src/tools/miri/tests/fail/alloc/alloc_error_handler_no_std.rs +++ b/src/tools/miri/tests/fail/alloc/alloc_error_handler_no_std.rs @@ -7,28 +7,16 @@ extern crate alloc; use alloc::alloc::*; -use alloc::boxed::Box; -use core::ptr::NonNull; -struct BadAlloc; - -// Create a failing allocator; that is the only way to actually call the alloc error handler. -unsafe impl Allocator for BadAlloc { - fn allocate(&self, _l: Layout) -> Result, AllocError> { - Err(AllocError) - } - - unsafe fn deallocate(&self, _ptr: NonNull, _layout: Layout) { - unreachable!(); - } +extern "Rust" { + fn miri_write_to_stderr(bytes: &[u8]); } -#[alloc_error_handler] -fn alloc_error_handler(_: Layout) -> ! { - extern "Rust" { - fn miri_write_to_stderr(bytes: &[u8]); - } - let msg = "custom alloc error handler called!\n"; +// The default no_std alloc_error_handler is a panic. + +#[panic_handler] +fn panic_handler(_panic_info: &core::panic::PanicInfo) -> ! { + let msg = "custom panic handler called!\n"; unsafe { miri_write_to_stderr(msg.as_bytes()) }; core::intrinsics::abort(); //~ERROR: aborted } @@ -37,11 +25,6 @@ fn alloc_error_handler(_: Layout) -> ! { mod plumbing { use super::*; - #[panic_handler] - fn panic_handler(_: &core::panic::PanicInfo) -> ! { - core::intrinsics::abort(); - } - struct NoAlloc; unsafe impl GlobalAlloc for NoAlloc { @@ -60,6 +43,5 @@ mod plumbing { #[start] fn start(_: isize, _: *const *const u8) -> isize { - let _b = Box::new_in(0, BadAlloc); - 0 + handle_alloc_error(Layout::for_value(&0)); } diff --git a/src/tools/miri/tests/fail/alloc/alloc_error_handler_no_std.stderr b/src/tools/miri/tests/fail/alloc/alloc_error_handler_no_std.stderr index b40ffb70121..906c51be8bd 100644 --- a/src/tools/miri/tests/fail/alloc/alloc_error_handler_no_std.stderr +++ b/src/tools/miri/tests/fail/alloc/alloc_error_handler_no_std.stderr @@ -1,4 +1,4 @@ -custom alloc error handler called! +custom panic handler called! error: abnormal termination: the program aborted execution --> $DIR/alloc_error_handler_no_std.rs:LL:CC | @@ -6,24 +6,17 @@ LL | core::intrinsics::abort(); | ^^^^^^^^^^^^^^^^^^^^^^^^^ the program aborted execution | = note: BACKTRACE: - = note: inside `alloc_error_handler` at $DIR/alloc_error_handler_no_std.rs:LL:CC -note: inside `_::__rg_oom` - --> $DIR/alloc_error_handler_no_std.rs:LL:CC - | -LL | #[alloc_error_handler] - | ---------------------- in this procedural macro expansion -LL | fn alloc_error_handler(_: Layout) -> ! { - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + = note: inside `panic_handler` at $DIR/alloc_error_handler_no_std.rs:LL:CC + = note: inside `alloc::alloc::__alloc_error_handler::__rdl_oom` at RUSTLIB/alloc/src/alloc.rs:LL:CC = note: inside `alloc::alloc::handle_alloc_error::rt_error` at RUSTLIB/alloc/src/alloc.rs:LL:CC = note: inside `alloc::alloc::handle_alloc_error` at RUSTLIB/alloc/src/alloc.rs:LL:CC - = note: inside `alloc::boxed::Box::::new_uninit_in` at RUSTLIB/alloc/src/boxed.rs:LL:CC - = note: inside `alloc::boxed::Box::::new_in` at RUSTLIB/alloc/src/boxed.rs:LL:CC note: inside `start` --> $DIR/alloc_error_handler_no_std.rs:LL:CC | -LL | let _b = Box::new_in(0, BadAlloc); - | ^^^^^^^^^^^^^^^^^^^^^^^^ - = note: this error originates in the attribute macro `alloc_error_handler` (in Nightly builds, run with -Z macro-backtrace for more info) +LL | handle_alloc_error(Layout::for_value(&0)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace error: aborting due to 1 previous error diff --git a/src/tools/miri/tests/panic/alloc_error_handler_hook.rs b/src/tools/miri/tests/panic/alloc_error_handler_hook.rs new file mode 100644 index 00000000000..a1eadb45fd1 --- /dev/null +++ b/src/tools/miri/tests/panic/alloc_error_handler_hook.rs @@ -0,0 +1,20 @@ +#![feature(allocator_api, alloc_error_hook)] + +use std::alloc::*; + +struct Bomb; +impl Drop for Bomb { + fn drop(&mut self) { + eprintln!("yes we are unwinding!"); + } +} + +#[allow(unreachable_code, unused_variables)] +fn main() { + // This is a particularly tricky hook, since it unwinds, which the default one does not. + set_alloc_error_hook(|_layout| panic!("alloc error hook called")); + + let bomb = Bomb; + handle_alloc_error(Layout::for_value(&0)); + std::mem::forget(bomb); // defuse unwinding bomb +} diff --git a/src/tools/miri/tests/panic/alloc_error_handler_hook.stderr b/src/tools/miri/tests/panic/alloc_error_handler_hook.stderr new file mode 100644 index 00000000000..01c286fe3d2 --- /dev/null +++ b/src/tools/miri/tests/panic/alloc_error_handler_hook.stderr @@ -0,0 +1,4 @@ +thread 'main' panicked at $DIR/alloc_error_handler_hook.rs:LL:CC: +alloc error hook called +note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace +yes we are unwinding! diff --git a/src/tools/miri/tests/panic/alloc_error_handler_panic.rs b/src/tools/miri/tests/panic/alloc_error_handler_panic.rs index 186c9667a97..c434e8d3227 100644 --- a/src/tools/miri/tests/panic/alloc_error_handler_panic.rs +++ b/src/tools/miri/tests/panic/alloc_error_handler_panic.rs @@ -2,21 +2,6 @@ #![feature(allocator_api)] use std::alloc::*; -use std::ptr::NonNull; - -struct BadAlloc; - -// Create a failing allocator; Miri's native allocator never fails so this is the only way to -// actually call the alloc error handler. -unsafe impl Allocator for BadAlloc { - fn allocate(&self, _l: Layout) -> Result, AllocError> { - Err(AllocError) - } - - unsafe fn deallocate(&self, _ptr: NonNull, _layout: Layout) { - unreachable!(); - } -} struct Bomb; impl Drop for Bomb { @@ -25,8 +10,9 @@ impl Drop for Bomb { } } +#[allow(unreachable_code, unused_variables)] fn main() { let bomb = Bomb; - let _b = Box::new_in(0, BadAlloc); + handle_alloc_error(Layout::for_value(&0)); std::mem::forget(bomb); // defuse unwinding bomb } From d7f79cc2b2c12e77aa05e027d270fe7f5648eed3 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Wed, 17 Apr 2024 09:40:53 +0200 Subject: [PATCH 10/33] tests/utils: add fmt::Write implementations for miri's native stdout/stderr --- .../fail/alloc/alloc_error_handler_custom.rs | 15 +++++------ .../alloc/alloc_error_handler_custom.stderr | 6 ++--- .../fail/alloc/alloc_error_handler_no_std.rs | 12 ++++----- .../alloc/alloc_error_handler_no_std.stderr | 2 ++ src/tools/miri/tests/fail/panic/no_std.rs | 24 +++--------------- src/tools/miri/tests/pass/no_std.rs | 22 +++------------- src/tools/miri/tests/utils/io.rs | 25 +++++++++++++++++++ src/tools/miri/tests/utils/miri_extern.rs | 4 +-- src/tools/miri/tests/utils/mod.no_std.rs | 11 ++++++++ src/tools/miri/tests/utils/mod.rs | 2 ++ 10 files changed, 64 insertions(+), 59 deletions(-) create mode 100644 src/tools/miri/tests/utils/io.rs create mode 100644 src/tools/miri/tests/utils/mod.no_std.rs diff --git a/src/tools/miri/tests/fail/alloc/alloc_error_handler_custom.rs b/src/tools/miri/tests/fail/alloc/alloc_error_handler_custom.rs index 9f8837a626f..babdb73f093 100644 --- a/src/tools/miri/tests/fail/alloc/alloc_error_handler_custom.rs +++ b/src/tools/miri/tests/fail/alloc/alloc_error_handler_custom.rs @@ -7,15 +7,14 @@ extern crate alloc; use alloc::alloc::*; +use core::fmt::Write; -extern "Rust" { - fn miri_write_to_stderr(bytes: &[u8]); -} +#[path = "../../utils/mod.no_std.rs"] +mod utils; #[alloc_error_handler] -fn alloc_error_handler(_: Layout) -> ! { - let msg = "custom alloc error handler called!\n"; - unsafe { miri_write_to_stderr(msg.as_bytes()) }; +fn alloc_error_handler(layout: Layout) -> ! { + let _ = writeln!(utils::MiriStderr, "custom alloc error handler: {layout:?}"); core::intrinsics::abort(); //~ERROR: aborted } @@ -25,9 +24,7 @@ mod plumbing { #[panic_handler] fn panic_handler(_: &core::panic::PanicInfo) -> ! { - let msg = "custom panic handler called!\n"; - unsafe { miri_write_to_stderr(msg.as_bytes()) }; - core::intrinsics::abort(); + loop {} } struct NoAlloc; diff --git a/src/tools/miri/tests/fail/alloc/alloc_error_handler_custom.stderr b/src/tools/miri/tests/fail/alloc/alloc_error_handler_custom.stderr index cdd666e5e0a..5d9c2e2fb4c 100644 --- a/src/tools/miri/tests/fail/alloc/alloc_error_handler_custom.stderr +++ b/src/tools/miri/tests/fail/alloc/alloc_error_handler_custom.stderr @@ -1,4 +1,4 @@ -custom alloc error handler called! +custom alloc error handler: Layout { size: 4, align: 4 (1 << 2) } error: abnormal termination: the program aborted execution --> $DIR/alloc_error_handler_custom.rs:LL:CC | @@ -12,8 +12,8 @@ note: inside `_::__rg_oom` | LL | #[alloc_error_handler] | ---------------------- in this procedural macro expansion -LL | fn alloc_error_handler(_: Layout) -> ! { - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +LL | fn alloc_error_handler(layout: Layout) -> ! { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ = note: inside `alloc::alloc::handle_alloc_error::rt_error` at RUSTLIB/alloc/src/alloc.rs:LL:CC = note: inside `alloc::alloc::handle_alloc_error` at RUSTLIB/alloc/src/alloc.rs:LL:CC note: inside `start` diff --git a/src/tools/miri/tests/fail/alloc/alloc_error_handler_no_std.rs b/src/tools/miri/tests/fail/alloc/alloc_error_handler_no_std.rs index e3949b889f9..18a8a61f22f 100644 --- a/src/tools/miri/tests/fail/alloc/alloc_error_handler_no_std.rs +++ b/src/tools/miri/tests/fail/alloc/alloc_error_handler_no_std.rs @@ -7,17 +7,17 @@ extern crate alloc; use alloc::alloc::*; +use core::fmt::Write; -extern "Rust" { - fn miri_write_to_stderr(bytes: &[u8]); -} +#[path = "../../utils/mod.no_std.rs"] +mod utils; // The default no_std alloc_error_handler is a panic. #[panic_handler] -fn panic_handler(_panic_info: &core::panic::PanicInfo) -> ! { - let msg = "custom panic handler called!\n"; - unsafe { miri_write_to_stderr(msg.as_bytes()) }; +fn panic_handler(panic_info: &core::panic::PanicInfo) -> ! { + let _ = writeln!(utils::MiriStderr, "custom panic handler called!"); + let _ = writeln!(utils::MiriStderr, "{panic_info}"); core::intrinsics::abort(); //~ERROR: aborted } diff --git a/src/tools/miri/tests/fail/alloc/alloc_error_handler_no_std.stderr b/src/tools/miri/tests/fail/alloc/alloc_error_handler_no_std.stderr index 906c51be8bd..6b98f6f75d8 100644 --- a/src/tools/miri/tests/fail/alloc/alloc_error_handler_no_std.stderr +++ b/src/tools/miri/tests/fail/alloc/alloc_error_handler_no_std.stderr @@ -1,4 +1,6 @@ custom panic handler called! +panicked at RUSTLIB/alloc/src/alloc.rs:LL:CC: +memory allocation of 4 bytes failed error: abnormal termination: the program aborted execution --> $DIR/alloc_error_handler_no_std.rs:LL:CC | diff --git a/src/tools/miri/tests/fail/panic/no_std.rs b/src/tools/miri/tests/fail/panic/no_std.rs index 26cc0b27821..4d32b6d7461 100644 --- a/src/tools/miri/tests/fail/panic/no_std.rs +++ b/src/tools/miri/tests/fail/panic/no_std.rs @@ -1,27 +1,11 @@ +//@compile-flags: -Cpanic=abort #![feature(start, core_intrinsics)] #![no_std] -//@compile-flags: -Cpanic=abort - -// Plumbing to let us use `writeln!` to host stderr: - -extern "Rust" { - fn miri_write_to_stderr(bytes: &[u8]); -} - -struct HostErr; use core::fmt::Write; -impl Write for HostErr { - fn write_str(&mut self, s: &str) -> core::fmt::Result { - unsafe { - miri_write_to_stderr(s.as_bytes()); - } - Ok(()) - } -} - -// Aaaand the test: +#[path = "../../utils/mod.no_std.rs"] +mod utils; #[start] fn start(_: isize, _: *const *const u8) -> isize { @@ -30,6 +14,6 @@ fn start(_: isize, _: *const *const u8) -> isize { #[panic_handler] fn panic_handler(panic_info: &core::panic::PanicInfo) -> ! { - writeln!(HostErr, "{panic_info}").ok(); + writeln!(utils::MiriStderr, "{panic_info}").ok(); core::intrinsics::abort(); //~ ERROR: the program aborted execution } diff --git a/src/tools/miri/tests/pass/no_std.rs b/src/tools/miri/tests/pass/no_std.rs index 3c98ee50aa9..fc1c16f5fb9 100644 --- a/src/tools/miri/tests/pass/no_std.rs +++ b/src/tools/miri/tests/pass/no_std.rs @@ -2,30 +2,14 @@ #![feature(start)] #![no_std] -// Plumbing to let us use `writeln!` to host stdout: - -extern "Rust" { - fn miri_write_to_stdout(bytes: &[u8]); -} - -struct Host; - use core::fmt::Write; -impl Write for Host { - fn write_str(&mut self, s: &str) -> core::fmt::Result { - unsafe { - miri_write_to_stdout(s.as_bytes()); - } - Ok(()) - } -} - -// Aaaand the test: +#[path = "../utils/mod.no_std.rs"] +mod utils; #[start] fn start(_: isize, _: *const *const u8) -> isize { - writeln!(Host, "hello, world!").unwrap(); + writeln!(utils::MiriStdout, "hello, world!").unwrap(); 0 } diff --git a/src/tools/miri/tests/utils/io.rs b/src/tools/miri/tests/utils/io.rs new file mode 100644 index 00000000000..e3eaa6c468a --- /dev/null +++ b/src/tools/miri/tests/utils/io.rs @@ -0,0 +1,25 @@ +use core::fmt::{self, Write}; + +use super::miri_extern; + +pub struct MiriStderr; + +impl Write for MiriStderr { + fn write_str(&mut self, s: &str) -> fmt::Result { + unsafe { + miri_extern::miri_write_to_stderr(s.as_bytes()); + } + Ok(()) + } +} + +pub struct MiriStdout; + +impl Write for MiriStdout { + fn write_str(&mut self, s: &str) -> fmt::Result { + unsafe { + miri_extern::miri_write_to_stdout(s.as_bytes()); + } + Ok(()) + } +} diff --git a/src/tools/miri/tests/utils/miri_extern.rs b/src/tools/miri/tests/utils/miri_extern.rs index e2983f6c71a..d6c43b18821 100644 --- a/src/tools/miri/tests/utils/miri_extern.rs +++ b/src/tools/miri/tests/utils/miri_extern.rs @@ -133,8 +133,8 @@ extern "Rust" { /// with a null terminator. /// Returns 0 if the `out` buffer was large enough, and the required size otherwise. pub fn miri_host_to_target_path( - path: *const std::ffi::c_char, - out: *mut std::ffi::c_char, + path: *const core::ffi::c_char, + out: *mut core::ffi::c_char, out_size: usize, ) -> usize; diff --git a/src/tools/miri/tests/utils/mod.no_std.rs b/src/tools/miri/tests/utils/mod.no_std.rs new file mode 100644 index 00000000000..aaf2bf50c4e --- /dev/null +++ b/src/tools/miri/tests/utils/mod.no_std.rs @@ -0,0 +1,11 @@ +#![allow(dead_code)] +#![allow(unused_imports)] + +#[macro_use] +mod macros; + +mod io; +mod miri_extern; + +pub use self::io::*; +pub use self::miri_extern::*; diff --git a/src/tools/miri/tests/utils/mod.rs b/src/tools/miri/tests/utils/mod.rs index cb9380f5753..138ada4e20d 100644 --- a/src/tools/miri/tests/utils/mod.rs +++ b/src/tools/miri/tests/utils/mod.rs @@ -5,9 +5,11 @@ mod macros; mod fs; +mod io; mod miri_extern; pub use self::fs::*; +pub use self::io::*; pub use self::miri_extern::*; pub fn run_provenance_gc() { From 9f156d38a554eae9a07d8bd436243f005597b1c7 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Wed, 17 Apr 2024 09:44:28 +0200 Subject: [PATCH 11/33] no need to use miri's native stderr here --- src/tools/miri/tests/pass/tree_borrows/reserved.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/tools/miri/tests/pass/tree_borrows/reserved.rs b/src/tools/miri/tests/pass/tree_borrows/reserved.rs index 87ce91a809f..f93cac8361e 100644 --- a/src/tools/miri/tests/pass/tree_borrows/reserved.rs +++ b/src/tools/miri/tests/pass/tree_borrows/reserved.rs @@ -27,9 +27,8 @@ fn main() { } } -unsafe fn print(msg: &str) { - utils::miri_write_to_stderr(msg.as_bytes()); - utils::miri_write_to_stderr("\n".as_bytes()); +fn print(msg: &str) { + eprintln!("{msg}"); } unsafe fn read_second(x: &mut T, y: *mut u8) { From 6a108a7477b16b9c89ae71585b9a4310dfdfd4ab Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Wed, 17 Apr 2024 20:38:15 +0200 Subject: [PATCH 12/33] Preparing for merge from rustc --- src/tools/miri/rust-version | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tools/miri/rust-version b/src/tools/miri/rust-version index bd87405da3d..f8f3016b15c 100644 --- a/src/tools/miri/rust-version +++ b/src/tools/miri/rust-version @@ -1 +1 @@ -803e33a4460c82581bd01d4008d0f44aef1ddfe8 +c45dee5efd0c042e9d1e24559ebd0d6424d8aa70 From 7c3c2716ff46fb3276c16bdaea226e5a5abc08c0 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Wed, 17 Apr 2024 21:20:25 +0200 Subject: [PATCH 13/33] fmt --- src/tools/miri/src/alloc_addresses/mod.rs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/tools/miri/src/alloc_addresses/mod.rs b/src/tools/miri/src/alloc_addresses/mod.rs index b4983656adc..3e00e6037c2 100644 --- a/src/tools/miri/src/alloc_addresses/mod.rs +++ b/src/tools/miri/src/alloc_addresses/mod.rs @@ -141,11 +141,7 @@ trait EvalContextExtPriv<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { } } - fn addr_from_alloc_id( - &self, - alloc_id: AllocId, - _kind: MemoryKind, - ) -> InterpResult<'tcx, u64> { + fn addr_from_alloc_id(&self, alloc_id: AllocId, _kind: MemoryKind) -> InterpResult<'tcx, u64> { let ecx = self.eval_context_ref(); let mut global_state = ecx.machine.alloc_addresses.borrow_mut(); let global_state = &mut *global_state; From 5c352a4e7571d09a2cc5fe9c5bc86b02d62b49d8 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Thu, 18 Apr 2024 10:23:22 +0200 Subject: [PATCH 14/33] add test for Drop terminator on non-drop type --- .../tests/pass/drop_type_without_drop_glue.rs | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) create mode 100644 src/tools/miri/tests/pass/drop_type_without_drop_glue.rs diff --git a/src/tools/miri/tests/pass/drop_type_without_drop_glue.rs b/src/tools/miri/tests/pass/drop_type_without_drop_glue.rs new file mode 100644 index 00000000000..43ddc8a4d8b --- /dev/null +++ b/src/tools/miri/tests/pass/drop_type_without_drop_glue.rs @@ -0,0 +1,21 @@ +#![feature(custom_mir, core_intrinsics, strict_provenance)] +use std::intrinsics::mir::*; + +// The `Drop` terminator on a type with no drop glue should be a NOP. + +#[custom_mir(dialect = "runtime", phase = "optimized")] +fn drop_in_place_with_terminator(ptr: *mut i32) { + mir! { + { + Drop(*ptr, ReturnTo(after_call), UnwindContinue()) + } + after_call = { + Return() + } + } +} + +pub fn main() { + drop_in_place_with_terminator(std::ptr::without_provenance_mut(0)); + drop_in_place_with_terminator(std::ptr::without_provenance_mut(1)); +} From 62e84d5ca09018212f13da67867fba692649125b Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Thu, 18 Apr 2024 10:58:49 +0200 Subject: [PATCH 15/33] move read_byte_slice to general helpers file, next to read_c_str --- src/tools/miri/src/helpers.rs | 17 ++++++++++++++++- src/tools/miri/src/shims/foreign_items.rs | 20 +++++--------------- 2 files changed, 21 insertions(+), 16 deletions(-) diff --git a/src/tools/miri/src/helpers.rs b/src/tools/miri/src/helpers.rs index e2c6769ccb5..784568ec569 100644 --- a/src/tools/miri/src/helpers.rs +++ b/src/tools/miri/src/helpers.rs @@ -912,10 +912,25 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { }) } + /// Read bytes from a byte slice. + fn read_byte_slice<'a>( + &'a self, + slice: &ImmTy<'tcx, Provenance>, + ) -> InterpResult<'tcx, &'a [u8]> + where + 'mir: 'a, + { + let this = self.eval_context_ref(); + let (ptr, len) = slice.to_scalar_pair(); + let ptr = ptr.to_pointer(this)?; + let len = len.to_target_usize(this)?; + let bytes = this.read_bytes_ptr_strip_provenance(ptr, Size::from_bytes(len))?; + Ok(bytes) + } + /// Read a sequence of bytes until the first null terminator. fn read_c_str<'a>(&'a self, ptr: Pointer>) -> InterpResult<'tcx, &'a [u8]> where - 'tcx: 'a, 'mir: 'a, { let this = self.eval_context_ref(); diff --git a/src/tools/miri/src/shims/foreign_items.rs b/src/tools/miri/src/shims/foreign_items.rs index e6fc29a5ae0..acbc36b05b7 100644 --- a/src/tools/miri/src/shims/foreign_items.rs +++ b/src/tools/miri/src/shims/foreign_items.rs @@ -305,19 +305,6 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { impl<'mir, 'tcx: 'mir> EvalContextExtPriv<'mir, 'tcx> for crate::MiriInterpCx<'mir, 'tcx> {} trait EvalContextExtPriv<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { - /// Read bytes from a `(ptr, len)` argument - fn read_byte_slice<'i>(&'i self, bytes: &OpTy<'tcx, Provenance>) -> InterpResult<'tcx, &'i [u8]> - where - 'mir: 'i, - { - let this = self.eval_context_ref(); - let (ptr, len) = this.read_immediate(bytes)?.to_scalar_pair(); - let ptr = ptr.to_pointer(this)?; - let len = len.to_target_usize(this)?; - let bytes = this.read_bytes_ptr_strip_provenance(ptr, Size::from_bytes(len))?; - Ok(bytes) - } - /// Returns the minimum alignment for the target architecture for allocations of the given size. fn min_align(&self, size: u64, kind: MiriMemoryKind) -> Align { let this = self.eval_context_ref(); @@ -466,7 +453,9 @@ trait EvalContextExtPriv<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { let [ptr, nth_parent, name] = this.check_shim(abi, Abi::Rust, link_name, args)?; let ptr = this.read_pointer(ptr)?; let nth_parent = this.read_scalar(nth_parent)?.to_u8()?; - let name = this.read_byte_slice(name)?; + let name = this.read_immediate(name)?; + + let name = this.read_byte_slice(&name)?; // We must make `name` owned because we need to // end the shared borrow from `read_byte_slice` before we can // start the mutable borrow for `give_pointer_debug_name`. @@ -527,7 +516,8 @@ trait EvalContextExtPriv<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { // README for details. "miri_write_to_stdout" | "miri_write_to_stderr" => { let [msg] = this.check_shim(abi, Abi::Rust, link_name, args)?; - let msg = this.read_byte_slice(msg)?; + let msg = this.read_immediate(msg)?; + let msg = this.read_byte_slice(&msg)?; // Note: we're ignoring errors writing to host stdout/stderr. let _ignore = match link_name.as_str() { "miri_write_to_stdout" => std::io::stdout().write_all(msg), From 5ef38aca35b7fd2645d82abfb8ab740e9fd01972 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Thu, 18 Apr 2024 11:12:58 +0200 Subject: [PATCH 16/33] add test checking that we do run MIR validation --- src/tools/miri/tests/panic/mir-validation.rs | 21 +++++++++++++++++++ .../miri/tests/panic/mir-validation.stderr | 21 +++++++++++++++++++ 2 files changed, 42 insertions(+) create mode 100644 src/tools/miri/tests/panic/mir-validation.rs create mode 100644 src/tools/miri/tests/panic/mir-validation.stderr diff --git a/src/tools/miri/tests/panic/mir-validation.rs b/src/tools/miri/tests/panic/mir-validation.rs new file mode 100644 index 00000000000..5e207c26096 --- /dev/null +++ b/src/tools/miri/tests/panic/mir-validation.rs @@ -0,0 +1,21 @@ +//! Ensure that the MIR validator runs on Miri's input. +//@normalize-stderr-test: "\n +[0-9]+:[^\n]+" -> "" +//@normalize-stderr-test: "\n +at [^\n]+" -> "" +//@normalize-stderr-test: "\n +\[\.\.\. omitted [0-9]+ frames? \.\.\.\]" -> "" +//@normalize-stderr-test: "\n[ =]*note:.*" -> "" +#![feature(custom_mir, core_intrinsics)] +use core::intrinsics::mir::*; + +#[custom_mir(dialect = "runtime", phase = "optimized")] +pub fn main() { + mir! { + let x: i32; + let tuple: (*mut i32,); + { + tuple.0 = core::ptr::addr_of_mut!(x); + // Deref at the wrong place! + *(tuple.0) = 1; + Return() + } + } +} diff --git a/src/tools/miri/tests/panic/mir-validation.stderr b/src/tools/miri/tests/panic/mir-validation.stderr new file mode 100644 index 00000000000..243fed020e7 --- /dev/null +++ b/src/tools/miri/tests/panic/mir-validation.stderr @@ -0,0 +1,21 @@ +thread 'rustc' panicked at compiler/rustc_const_eval/src/transform/validate.rs:LL:CC: +broken MIR in Item(DefId(0:4 ~ mir_validation[fad2]::main)) (after phase change to runtime-optimized) at bb0[1]: +(*(_2.0: *mut i32)), has deref at the wrong place +stack backtrace: + +error: the compiler unexpectedly panicked. this is a bug. + + + + +query stack during panic: +#0 [optimized_mir] optimizing MIR for `main` +end of query stack + +Miri caused an ICE during evaluation. Here's the interpreter backtrace at the time of the panic: + --> RUSTLIB/core/src/ops/function.rs:LL:CC + | +LL | extern "rust-call" fn call_once(self, args: Args) -> Self::Output; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + From 73e333ac44b27bee0de86920de72b3097feb648b Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Thu, 18 Apr 2024 10:32:44 +0200 Subject: [PATCH 17/33] make realloc with a size of zero fail --- src/tools/miri/src/shims/foreign_items.rs | 6 ++++-- src/tools/miri/tests/fail-dep/realloc-zero.rs | 10 ++++++++++ src/tools/miri/tests/fail-dep/realloc-zero.stderr | 15 +++++++++++++++ src/tools/miri/tests/pass-dep/malloc.rs | 5 ++--- 4 files changed, 31 insertions(+), 5 deletions(-) create mode 100644 src/tools/miri/tests/fail-dep/realloc-zero.rs create mode 100644 src/tools/miri/tests/fail-dep/realloc-zero.stderr diff --git a/src/tools/miri/src/shims/foreign_items.rs b/src/tools/miri/src/shims/foreign_items.rs index acbc36b05b7..6a6ad33e52f 100644 --- a/src/tools/miri/src/shims/foreign_items.rs +++ b/src/tools/miri/src/shims/foreign_items.rs @@ -278,6 +278,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { let this = self.eval_context_mut(); let new_align = this.min_align(new_size, kind); if this.ptr_is_null(old_ptr)? { + // Here we must behave like `malloc`. if new_size == 0 { Ok(Pointer::null()) } else { @@ -287,8 +288,9 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { } } else { if new_size == 0 { - this.deallocate_ptr(old_ptr, None, kind.into())?; - Ok(Pointer::null()) + // C, in their infinite wisdom, made this UB. + // + throw_ub_format!("`realloc` with a size of zero"); } else { let new_ptr = this.reallocate_ptr( old_ptr, diff --git a/src/tools/miri/tests/fail-dep/realloc-zero.rs b/src/tools/miri/tests/fail-dep/realloc-zero.rs new file mode 100644 index 00000000000..1482798e90c --- /dev/null +++ b/src/tools/miri/tests/fail-dep/realloc-zero.rs @@ -0,0 +1,10 @@ +//@ignore-target-windows: No libc on Windows + +fn main() { + unsafe { + let p1 = libc::malloc(20); + // C made this UB... + let p2 = libc::realloc(p1, 0); //~ERROR: `realloc` with a size of zero + assert!(p2.is_null()); + } +} diff --git a/src/tools/miri/tests/fail-dep/realloc-zero.stderr b/src/tools/miri/tests/fail-dep/realloc-zero.stderr new file mode 100644 index 00000000000..749a61f7396 --- /dev/null +++ b/src/tools/miri/tests/fail-dep/realloc-zero.stderr @@ -0,0 +1,15 @@ +error: Undefined Behavior: `realloc` with a size of zero + --> $DIR/realloc-zero.rs:LL:CC + | +LL | let p2 = libc::realloc(p1, 0); + | ^^^^^^^^^^^^^^^^^^^^ `realloc` with a size of zero + | + = 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 + = note: BACKTRACE: + = note: inside `main` at $DIR/realloc-zero.rs:LL:CC + +note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace + +error: aborting due to 1 previous error + diff --git a/src/tools/miri/tests/pass-dep/malloc.rs b/src/tools/miri/tests/pass-dep/malloc.rs index f5e014c000d..35cd137931f 100644 --- a/src/tools/miri/tests/pass-dep/malloc.rs +++ b/src/tools/miri/tests/pass-dep/malloc.rs @@ -34,9 +34,8 @@ fn main() { } unsafe { - let p1 = libc::malloc(20); - - let p2 = libc::realloc(p1, 0); + // Realloc with size 0 is okay for the null pointer + let p2 = libc::realloc(ptr::null_mut(), 0); assert!(p2.is_null()); } From 5ff9b2b8651255550eaa815d3c4927967c6b34f7 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Thu, 18 Apr 2024 10:42:46 +0200 Subject: [PATCH 18/33] move allocator shim logic into its own file --- src/tools/miri/src/shims/alloc.rs | 152 ++++++++++++++++++ src/tools/miri/src/shims/foreign_items.rs | 151 +---------------- src/tools/miri/src/shims/mod.rs | 1 + .../miri/src/shims/unix/foreign_items.rs | 1 + src/tools/miri/src/shims/unix/fs.rs | 29 ++-- .../miri/src/shims/windows/foreign_items.rs | 1 + 6 files changed, 178 insertions(+), 157 deletions(-) create mode 100644 src/tools/miri/src/shims/alloc.rs diff --git a/src/tools/miri/src/shims/alloc.rs b/src/tools/miri/src/shims/alloc.rs new file mode 100644 index 00000000000..b5ae06c2a49 --- /dev/null +++ b/src/tools/miri/src/shims/alloc.rs @@ -0,0 +1,152 @@ +use std::iter; + +use rustc_ast::expand::allocator::AllocatorKind; +use rustc_target::abi::{Align, Size}; + +use crate::*; +use shims::foreign_items::EmulateForeignItemResult; + +/// Check some basic requirements for this allocation request: +/// non-zero size, power-of-two alignment. +pub(super) fn check_alloc_request<'tcx>(size: u64, align: u64) -> InterpResult<'tcx> { + if size == 0 { + throw_ub_format!("creating allocation with size 0"); + } + if !align.is_power_of_two() { + throw_ub_format!("creating allocation with non-power-of-two alignment {}", align); + } + Ok(()) +} + +impl<'mir, 'tcx: 'mir> EvalContextExt<'mir, 'tcx> for crate::MiriInterpCx<'mir, 'tcx> {} +pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { + /// Returns the minimum alignment for the target architecture for allocations of the given size. + fn min_align(&self, size: u64, kind: MiriMemoryKind) -> Align { + let this = self.eval_context_ref(); + // List taken from `library/std/src/sys/pal/common/alloc.rs`. + // This list should be kept in sync with the one from libstd. + let min_align = match this.tcx.sess.target.arch.as_ref() { + "x86" | "arm" | "mips" | "mips32r6" | "powerpc" | "powerpc64" | "wasm32" => 8, + "x86_64" | "aarch64" | "mips64" | "mips64r6" | "s390x" | "sparc64" | "loongarch64" => + 16, + arch => bug!("unsupported target architecture for malloc: `{}`", arch), + }; + // Windows always aligns, even small allocations. + // Source: + // But jemalloc does not, so for the C heap we only align if the allocation is sufficiently big. + if kind == MiriMemoryKind::WinHeap || size >= min_align { + return Align::from_bytes(min_align).unwrap(); + } + // We have `size < min_align`. Round `size` *down* to the next power of two and use that. + fn prev_power_of_two(x: u64) -> u64 { + let next_pow2 = x.next_power_of_two(); + if next_pow2 == x { + // x *is* a power of two, just use that. + x + } else { + // x is between two powers, so next = 2*prev. + next_pow2 / 2 + } + } + Align::from_bytes(prev_power_of_two(size)).unwrap() + } + + /// Emulates calling the internal __rust_* allocator functions + fn emulate_allocator( + &mut self, + default: impl FnOnce(&mut MiriInterpCx<'mir, 'tcx>) -> InterpResult<'tcx>, + ) -> InterpResult<'tcx, EmulateForeignItemResult> { + let this = self.eval_context_mut(); + + let Some(allocator_kind) = this.tcx.allocator_kind(()) else { + // in real code, this symbol does not exist without an allocator + return Ok(EmulateForeignItemResult::NotSupported); + }; + + match allocator_kind { + AllocatorKind::Global => { + // When `#[global_allocator]` is used, `__rust_*` is defined by the macro expansion + // of this attribute. As such we have to call an exported Rust function, + // and not execute any Miri shim. Somewhat unintuitively doing so is done + // by returning `NotSupported`, which triggers the `lookup_exported_symbol` + // fallback case in `emulate_foreign_item`. + return Ok(EmulateForeignItemResult::NotSupported); + } + AllocatorKind::Default => { + default(this)?; + Ok(EmulateForeignItemResult::NeedsJumping) + } + } + } + + fn malloc( + &mut self, + size: u64, + zero_init: bool, + kind: MiriMemoryKind, + ) -> InterpResult<'tcx, Pointer>> { + let this = self.eval_context_mut(); + if size == 0 { + Ok(Pointer::null()) + } else { + let align = this.min_align(size, kind); + let ptr = this.allocate_ptr(Size::from_bytes(size), align, kind.into())?; + if zero_init { + // We just allocated this, the access is definitely in-bounds and fits into our address space. + this.write_bytes_ptr( + ptr.into(), + iter::repeat(0u8).take(usize::try_from(size).unwrap()), + ) + .unwrap(); + } + Ok(ptr.into()) + } + } + + fn free( + &mut self, + ptr: Pointer>, + kind: MiriMemoryKind, + ) -> InterpResult<'tcx> { + let this = self.eval_context_mut(); + if !this.ptr_is_null(ptr)? { + this.deallocate_ptr(ptr, None, kind.into())?; + } + Ok(()) + } + + fn realloc( + &mut self, + old_ptr: Pointer>, + new_size: u64, + kind: MiriMemoryKind, + ) -> InterpResult<'tcx, Pointer>> { + let this = self.eval_context_mut(); + let new_align = this.min_align(new_size, kind); + if this.ptr_is_null(old_ptr)? { + // Here we must behave like `malloc`. + if new_size == 0 { + Ok(Pointer::null()) + } else { + let new_ptr = + this.allocate_ptr(Size::from_bytes(new_size), new_align, kind.into())?; + Ok(new_ptr.into()) + } + } else { + if new_size == 0 { + // C, in their infinite wisdom, made this UB. + // + throw_ub_format!("`realloc` with a size of zero"); + } else { + let new_ptr = this.reallocate_ptr( + old_ptr, + None, + Size::from_bytes(new_size), + new_align, + kind.into(), + )?; + Ok(new_ptr.into()) + } + } + } +} diff --git a/src/tools/miri/src/shims/foreign_items.rs b/src/tools/miri/src/shims/foreign_items.rs index 6a6ad33e52f..636361148a4 100644 --- a/src/tools/miri/src/shims/foreign_items.rs +++ b/src/tools/miri/src/shims/foreign_items.rs @@ -1,7 +1,7 @@ use std::{collections::hash_map::Entry, io::Write, iter, path::Path}; use rustc_apfloat::Float; -use rustc_ast::expand::allocator::{alloc_error_handler_name, AllocatorKind}; +use rustc_ast::expand::allocator::alloc_error_handler_name; use rustc_hir::{def::DefKind, def_id::CrateNum}; use rustc_middle::middle::codegen_fn_attrs::CodegenFnAttrFlags; use rustc_middle::mir; @@ -12,6 +12,7 @@ use rustc_target::{ spec::abi::Abi, }; +use super::alloc::{check_alloc_request, EvalContextExt as _}; use super::backtrace::EvalContextExt as _; use crate::*; use helpers::{ToHost, ToSoft}; @@ -232,140 +233,10 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { Some(instance) => Ok(Some((this.load_mir(instance.def, None)?, instance))), } } - - fn malloc( - &mut self, - size: u64, - zero_init: bool, - kind: MiriMemoryKind, - ) -> InterpResult<'tcx, Pointer>> { - let this = self.eval_context_mut(); - if size == 0 { - Ok(Pointer::null()) - } else { - let align = this.min_align(size, kind); - let ptr = this.allocate_ptr(Size::from_bytes(size), align, kind.into())?; - if zero_init { - // We just allocated this, the access is definitely in-bounds and fits into our address space. - this.write_bytes_ptr( - ptr.into(), - iter::repeat(0u8).take(usize::try_from(size).unwrap()), - ) - .unwrap(); - } - Ok(ptr.into()) - } - } - - fn free( - &mut self, - ptr: Pointer>, - kind: MiriMemoryKind, - ) -> InterpResult<'tcx> { - let this = self.eval_context_mut(); - if !this.ptr_is_null(ptr)? { - this.deallocate_ptr(ptr, None, kind.into())?; - } - Ok(()) - } - - fn realloc( - &mut self, - old_ptr: Pointer>, - new_size: u64, - kind: MiriMemoryKind, - ) -> InterpResult<'tcx, Pointer>> { - let this = self.eval_context_mut(); - let new_align = this.min_align(new_size, kind); - if this.ptr_is_null(old_ptr)? { - // Here we must behave like `malloc`. - if new_size == 0 { - Ok(Pointer::null()) - } else { - let new_ptr = - this.allocate_ptr(Size::from_bytes(new_size), new_align, kind.into())?; - Ok(new_ptr.into()) - } - } else { - if new_size == 0 { - // C, in their infinite wisdom, made this UB. - // - throw_ub_format!("`realloc` with a size of zero"); - } else { - let new_ptr = this.reallocate_ptr( - old_ptr, - None, - Size::from_bytes(new_size), - new_align, - kind.into(), - )?; - Ok(new_ptr.into()) - } - } - } } impl<'mir, 'tcx: 'mir> EvalContextExtPriv<'mir, 'tcx> for crate::MiriInterpCx<'mir, 'tcx> {} trait EvalContextExtPriv<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { - /// Returns the minimum alignment for the target architecture for allocations of the given size. - fn min_align(&self, size: u64, kind: MiriMemoryKind) -> Align { - let this = self.eval_context_ref(); - // List taken from `library/std/src/sys/pal/common/alloc.rs`. - // This list should be kept in sync with the one from libstd. - let min_align = match this.tcx.sess.target.arch.as_ref() { - "x86" | "arm" | "mips" | "mips32r6" | "powerpc" | "powerpc64" | "wasm32" => 8, - "x86_64" | "aarch64" | "mips64" | "mips64r6" | "s390x" | "sparc64" | "loongarch64" => - 16, - arch => bug!("unsupported target architecture for malloc: `{}`", arch), - }; - // Windows always aligns, even small allocations. - // Source: - // But jemalloc does not, so for the C heap we only align if the allocation is sufficiently big. - if kind == MiriMemoryKind::WinHeap || size >= min_align { - return Align::from_bytes(min_align).unwrap(); - } - // We have `size < min_align`. Round `size` *down* to the next power of two and use that. - fn prev_power_of_two(x: u64) -> u64 { - let next_pow2 = x.next_power_of_two(); - if next_pow2 == x { - // x *is* a power of two, just use that. - x - } else { - // x is between two powers, so next = 2*prev. - next_pow2 / 2 - } - } - Align::from_bytes(prev_power_of_two(size)).unwrap() - } - - /// Emulates calling the internal __rust_* allocator functions - fn emulate_allocator( - &mut self, - default: impl FnOnce(&mut MiriInterpCx<'mir, 'tcx>) -> InterpResult<'tcx>, - ) -> InterpResult<'tcx, EmulateForeignItemResult> { - let this = self.eval_context_mut(); - - let Some(allocator_kind) = this.tcx.allocator_kind(()) else { - // in real code, this symbol does not exist without an allocator - return Ok(EmulateForeignItemResult::NotSupported); - }; - - match allocator_kind { - AllocatorKind::Global => { - // When `#[global_allocator]` is used, `__rust_*` is defined by the macro expansion - // of this attribute. As such we have to call an exported Rust function, - // and not execute any Miri shim. Somewhat unintuitively doing so is done - // by returning `NotSupported`, which triggers the `lookup_exported_symbol` - // fallback case in `emulate_foreign_item`. - return Ok(EmulateForeignItemResult::NotSupported); - } - AllocatorKind::Default => { - default(this)?; - Ok(EmulateForeignItemResult::NeedsJumping) - } - } - } - fn emulate_foreign_item_inner( &mut self, link_name: Symbol, @@ -612,7 +483,7 @@ trait EvalContextExtPriv<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { let size = this.read_target_usize(size)?; let align = this.read_target_usize(align)?; - Self::check_alloc_request(size, align)?; + check_alloc_request(size, align)?; let memory_kind = match link_name.as_str() { "__rust_alloc" => MiriMemoryKind::Rust, @@ -646,7 +517,7 @@ trait EvalContextExtPriv<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { let size = this.read_target_usize(size)?; let align = this.read_target_usize(align)?; - Self::check_alloc_request(size, align)?; + check_alloc_request(size, align)?; let ptr = this.allocate_ptr( Size::from_bytes(size), @@ -710,7 +581,7 @@ trait EvalContextExtPriv<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { let new_size = this.read_target_usize(new_size)?; // No need to check old_size; we anyway check that they match the allocation. - Self::check_alloc_request(new_size, align)?; + check_alloc_request(new_size, align)?; let align = Align::from_bytes(align).unwrap(); let new_ptr = this.reallocate_ptr( @@ -1102,16 +973,4 @@ trait EvalContextExtPriv<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { // i.e., if we actually emulated the function with one of the shims. Ok(EmulateForeignItemResult::NeedsJumping) } - - /// Check some basic requirements for this allocation request: - /// non-zero size, power-of-two alignment. - fn check_alloc_request(size: u64, align: u64) -> InterpResult<'tcx> { - if size == 0 { - throw_ub_format!("creating allocation with size 0"); - } - if !align.is_power_of_two() { - throw_ub_format!("creating allocation with non-power-of-two alignment {}", align); - } - Ok(()) - } } diff --git a/src/tools/miri/src/shims/mod.rs b/src/tools/miri/src/shims/mod.rs index ea6120f7579..85c9a202f7d 100644 --- a/src/tools/miri/src/shims/mod.rs +++ b/src/tools/miri/src/shims/mod.rs @@ -1,5 +1,6 @@ #![warn(clippy::arithmetic_side_effects)] +mod alloc; mod backtrace; #[cfg(target_os = "linux")] pub mod ffi_support; diff --git a/src/tools/miri/src/shims/unix/foreign_items.rs b/src/tools/miri/src/shims/unix/foreign_items.rs index 3a56aa9138b..c72d3bb3df4 100644 --- a/src/tools/miri/src/shims/unix/foreign_items.rs +++ b/src/tools/miri/src/shims/unix/foreign_items.rs @@ -6,6 +6,7 @@ use rustc_span::Symbol; use rustc_target::abi::{Align, Size}; use rustc_target::spec::abi::Abi; +use crate::shims::alloc::EvalContextExt as _; use crate::shims::unix::*; use crate::*; use shims::foreign_items::EmulateForeignItemResult; diff --git a/src/tools/miri/src/shims/unix/fs.rs b/src/tools/miri/src/shims/unix/fs.rs index 31076fdfaf6..ebf9f43c19e 100644 --- a/src/tools/miri/src/shims/unix/fs.rs +++ b/src/tools/miri/src/shims/unix/fs.rs @@ -196,13 +196,12 @@ struct OpenDir { read_dir: ReadDir, /// The most recent entry returned by readdir(). /// Will be freed by the next call. - entry: Pointer>, + entry: Option>>, } impl OpenDir { fn new(read_dir: ReadDir) -> Self { - // We rely on `free` being a NOP on null pointers. - Self { read_dir, entry: Pointer::null() } + Self { read_dir, entry: None } } } @@ -924,8 +923,12 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { let d_name_offset = dirent64_layout.fields.offset(4 /* d_name */).bytes(); let size = d_name_offset.checked_add(name_len).unwrap(); - let entry = - this.malloc(size, /*zero_init:*/ false, MiriMemoryKind::Runtime)?; + let entry = this.allocate_ptr( + Size::from_bytes(size), + dirent64_layout.align.abi, + MiriMemoryKind::Runtime.into(), + )?; + let entry: Pointer> = entry.into(); // If the host is a Unix system, fill in the inode number with its real value. // If not, use 0 as a fallback value. @@ -949,23 +952,25 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { let name_ptr = entry.offset(Size::from_bytes(d_name_offset), this)?; this.write_bytes_ptr(name_ptr, name_bytes.iter().copied())?; - entry + Some(entry) } None => { // end of stream: return NULL - Pointer::null() + None } Some(Err(e)) => { this.set_last_error_from_io_error(e.kind())?; - Pointer::null() + None } }; let open_dir = this.machine.dirs.streams.get_mut(&dirp).unwrap(); let old_entry = std::mem::replace(&mut open_dir.entry, entry); - this.free(old_entry, MiriMemoryKind::Runtime)?; + if let Some(old_entry) = old_entry { + this.deallocate_ptr(old_entry, None, MiriMemoryKind::Runtime.into())?; + } - Ok(Scalar::from_maybe_pointer(entry, this)) + Ok(Scalar::from_maybe_pointer(entry.unwrap_or_else(Pointer::null), this)) } fn macos_fbsd_readdir_r( @@ -1106,7 +1111,9 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { } if let Some(open_dir) = this.machine.dirs.streams.remove(&dirp) { - this.free(open_dir.entry, MiriMemoryKind::Runtime)?; + if let Some(entry) = open_dir.entry { + this.deallocate_ptr(entry, None, MiriMemoryKind::Runtime.into())?; + } drop(open_dir); Ok(0) } else { diff --git a/src/tools/miri/src/shims/windows/foreign_items.rs b/src/tools/miri/src/shims/windows/foreign_items.rs index de80df3c80d..ec4c6101487 100644 --- a/src/tools/miri/src/shims/windows/foreign_items.rs +++ b/src/tools/miri/src/shims/windows/foreign_items.rs @@ -8,6 +8,7 @@ use rustc_span::Symbol; use rustc_target::abi::Size; use rustc_target::spec::abi::Abi; +use crate::shims::alloc::EvalContextExt as _; use crate::shims::os_str::bytes_to_os_str; use crate::*; use shims::foreign_items::EmulateForeignItemResult; From 48fd549cd3604e657dfd7542b40df45e76c02a0b Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 16 Apr 2024 15:23:00 +0200 Subject: [PATCH 19/33] when an address gets reused, establish a happens-before link in the data race model --- src/tools/miri/src/alloc_addresses/mod.rs | 42 ++++++++----- .../miri/src/alloc_addresses/reuse_pool.rs | 39 ++++++++---- src/tools/miri/src/concurrency/mod.rs | 2 + src/tools/miri/src/concurrency/thread.rs | 6 ++ src/tools/miri/src/helpers.rs | 6 +- src/tools/miri/src/machine.rs | 7 +-- .../address_reuse_happens_before.rs | 60 +++++++++++++++++++ src/tools/miri/tests/pass/weak_memory/weak.rs | 3 + 8 files changed, 129 insertions(+), 36 deletions(-) create mode 100644 src/tools/miri/tests/pass/concurrency/address_reuse_happens_before.rs diff --git a/src/tools/miri/src/alloc_addresses/mod.rs b/src/tools/miri/src/alloc_addresses/mod.rs index 3e00e6037c2..50142d6b5a8 100644 --- a/src/tools/miri/src/alloc_addresses/mod.rs +++ b/src/tools/miri/src/alloc_addresses/mod.rs @@ -14,7 +14,8 @@ use rustc_span::Span; use rustc_target::abi::{Align, HasDataLayout, Size}; use crate::*; -use reuse_pool::ReusePool; + +use self::reuse_pool::ReusePool; #[derive(Copy, Clone, Debug, PartialEq, Eq)] pub enum ProvenanceMode { @@ -159,9 +160,12 @@ trait EvalContextExtPriv<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { assert!(!matches!(kind, AllocKind::Dead)); // This allocation does not have a base address yet, pick or reuse one. - let base_addr = if let Some(reuse_addr) = + let base_addr = if let Some((reuse_addr, clock)) = global_state.reuse.take_addr(&mut *rng, size, align) { + if let Some(data_race) = &ecx.machine.data_race { + data_race.validate_lock_acquire(&clock, ecx.get_active_thread()); + } reuse_addr } else { // We have to pick a fresh address. @@ -329,14 +333,11 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { } } -impl GlobalStateInner { - pub fn free_alloc_id( - &mut self, - rng: &mut impl Rng, - dead_id: AllocId, - size: Size, - align: Align, - ) { +impl<'mir, 'tcx> MiriMachine<'mir, 'tcx> { + pub fn free_alloc_id(&mut self, dead_id: AllocId, size: Size, align: Align) { + let global_state = self.alloc_addresses.get_mut(); + let rng = self.rng.get_mut(); + // We can *not* remove this from `base_addr`, since the interpreter design requires that we // be able to retrieve an AllocId + offset for any memory access *before* we check if the // access is valid. Specifically, `ptr_get_alloc` is called on each attempt at a memory @@ -349,15 +350,26 @@ impl GlobalStateInner { // returns a dead allocation. // To avoid a linear scan we first look up the address in `base_addr`, and then find it in // `int_to_ptr_map`. - let addr = *self.base_addr.get(&dead_id).unwrap(); - let pos = self.int_to_ptr_map.binary_search_by_key(&addr, |(addr, _)| *addr).unwrap(); - let removed = self.int_to_ptr_map.remove(pos); + let addr = *global_state.base_addr.get(&dead_id).unwrap(); + let pos = + global_state.int_to_ptr_map.binary_search_by_key(&addr, |(addr, _)| *addr).unwrap(); + let removed = global_state.int_to_ptr_map.remove(pos); assert_eq!(removed, (addr, dead_id)); // double-check that we removed the right thing // We can also remove it from `exposed`, since this allocation can anyway not be returned by // `alloc_id_from_addr` any more. - self.exposed.remove(&dead_id); + global_state.exposed.remove(&dead_id); // Also remember this address for future reuse. - self.reuse.add_addr(rng, addr, size, align) + global_state.reuse.add_addr(rng, addr, size, align, || { + let mut clock = concurrency::VClock::default(); + if let Some(data_race) = &self.data_race { + data_race.validate_lock_release( + &mut clock, + self.threads.get_active_thread_id(), + self.threads.active_thread_ref().current_span(), + ); + } + clock + }) } } diff --git a/src/tools/miri/src/alloc_addresses/reuse_pool.rs b/src/tools/miri/src/alloc_addresses/reuse_pool.rs index 8374d0ec605..9af4bdac4cf 100644 --- a/src/tools/miri/src/alloc_addresses/reuse_pool.rs +++ b/src/tools/miri/src/alloc_addresses/reuse_pool.rs @@ -4,6 +4,8 @@ use rand::Rng; use rustc_target::abi::{Align, Size}; +use crate::concurrency::VClock; + const MAX_POOL_SIZE: usize = 64; // Just use fair coins, until we have evidence that other numbers are better. @@ -21,7 +23,10 @@ pub struct ReusePool { /// /// Each of these maps has at most MAX_POOL_SIZE elements, and since alignment is limited to /// less than 64 different possible value, that bounds the overall size of the pool. - pool: Vec>, + /// + /// We also store the clock from the thread that donated this pool element, + /// to ensure synchronization with the thread that picks up this address. + pool: Vec>, } impl ReusePool { @@ -29,7 +34,7 @@ impl ReusePool { ReusePool { pool: vec![] } } - fn subpool(&mut self, align: Align) -> &mut Vec<(u64, Size)> { + fn subpool(&mut self, align: Align) -> &mut Vec<(u64, Size, VClock)> { let pool_idx: usize = align.bytes().trailing_zeros().try_into().unwrap(); if self.pool.len() <= pool_idx { self.pool.resize(pool_idx + 1, Vec::new()); @@ -37,26 +42,38 @@ impl ReusePool { &mut self.pool[pool_idx] } - pub fn add_addr(&mut self, rng: &mut impl Rng, addr: u64, size: Size, align: Align) { + pub fn add_addr( + &mut self, + rng: &mut impl Rng, + addr: u64, + size: Size, + align: Align, + clock: impl FnOnce() -> VClock, + ) { // Let's see if we even want to remember this address. if !rng.gen_bool(ADDR_REMEMBER_CHANCE) { return; } // Determine the pool to add this to, and where in the pool to put it. let subpool = self.subpool(align); - let pos = subpool.partition_point(|(_addr, other_size)| *other_size < size); + let pos = subpool.partition_point(|(_addr, other_size, _)| *other_size < size); // Make sure the pool does not grow too big. if subpool.len() >= MAX_POOL_SIZE { // Pool full. Replace existing element, or last one if this would be even bigger. let clamped_pos = pos.min(subpool.len() - 1); - subpool[clamped_pos] = (addr, size); + subpool[clamped_pos] = (addr, size, clock()); return; } // Add address to pool, at the right position. - subpool.insert(pos, (addr, size)); + subpool.insert(pos, (addr, size, clock())); } - pub fn take_addr(&mut self, rng: &mut impl Rng, size: Size, align: Align) -> Option { + pub fn take_addr( + &mut self, + rng: &mut impl Rng, + size: Size, + align: Align, + ) -> Option<(u64, VClock)> { // Determine whether we'll even attempt a reuse. if !rng.gen_bool(ADDR_TAKE_CHANCE) { return None; @@ -65,9 +82,9 @@ impl ReusePool { let subpool = self.subpool(align); // Let's see if we can find something of the right size. We want to find the full range of // such items, beginning with the first, so we can't use `binary_search_by_key`. - let begin = subpool.partition_point(|(_addr, other_size)| *other_size < size); + let begin = subpool.partition_point(|(_addr, other_size, _)| *other_size < size); let mut end = begin; - while let Some((_addr, other_size)) = subpool.get(end) { + while let Some((_addr, other_size, _)) = subpool.get(end) { if *other_size != size { break; } @@ -80,8 +97,8 @@ impl ReusePool { // Pick a random element with the desired size. let idx = rng.gen_range(begin..end); // Remove it from the pool and return. - let (chosen_addr, chosen_size) = subpool.remove(idx); + let (chosen_addr, chosen_size, clock) = subpool.remove(idx); debug_assert!(chosen_size >= size && chosen_addr % align.bytes() == 0); - Some(chosen_addr) + Some((chosen_addr, clock)) } } diff --git a/src/tools/miri/src/concurrency/mod.rs b/src/tools/miri/src/concurrency/mod.rs index 45903107f17..15e1a94d6db 100644 --- a/src/tools/miri/src/concurrency/mod.rs +++ b/src/tools/miri/src/concurrency/mod.rs @@ -6,3 +6,5 @@ pub mod init_once; pub mod thread; mod vector_clock; pub mod weak_memory; + +pub use vector_clock::VClock; diff --git a/src/tools/miri/src/concurrency/thread.rs b/src/tools/miri/src/concurrency/thread.rs index e28e5f83697..2fabd39a744 100644 --- a/src/tools/miri/src/concurrency/thread.rs +++ b/src/tools/miri/src/concurrency/thread.rs @@ -223,6 +223,12 @@ impl<'mir, 'tcx> Thread<'mir, 'tcx> { // empty stacks. self.top_user_relevant_frame.or_else(|| self.stack.len().checked_sub(1)) } + + pub fn current_span(&self) -> Span { + self.top_user_relevant_frame() + .map(|frame_idx| self.stack[frame_idx].current_span()) + .unwrap_or(rustc_span::DUMMY_SP) + } } impl<'mir, 'tcx> std::fmt::Debug for Thread<'mir, 'tcx> { diff --git a/src/tools/miri/src/helpers.rs b/src/tools/miri/src/helpers.rs index e2c6769ccb5..17ae2dd91a0 100644 --- a/src/tools/miri/src/helpers.rs +++ b/src/tools/miri/src/helpers.rs @@ -1265,9 +1265,7 @@ impl<'mir, 'tcx> MiriMachine<'mir, 'tcx> { /// This function is backed by a cache, and can be assumed to be very fast. /// It will work even when the stack is empty. pub fn current_span(&self) -> Span { - self.top_user_relevant_frame() - .map(|frame_idx| self.stack()[frame_idx].current_span()) - .unwrap_or(rustc_span::DUMMY_SP) + self.threads.active_thread_ref().current_span() } /// Returns the span of the *caller* of the current operation, again @@ -1279,7 +1277,7 @@ impl<'mir, 'tcx> MiriMachine<'mir, 'tcx> { // We need to go down at least to the caller (len - 2), or however // far we have to go to find a frame in a local crate which is also not #[track_caller]. let frame_idx = self.top_user_relevant_frame().unwrap(); - let frame_idx = cmp::min(frame_idx, self.stack().len().checked_sub(2).unwrap()); + let frame_idx = cmp::min(frame_idx, self.stack().len().saturating_sub(2)); self.stack()[frame_idx].current_span() } diff --git a/src/tools/miri/src/machine.rs b/src/tools/miri/src/machine.rs index 0bfc59e67db..d7c762fe1a9 100644 --- a/src/tools/miri/src/machine.rs +++ b/src/tools/miri/src/machine.rs @@ -1303,12 +1303,7 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for MiriMachine<'mir, 'tcx> { { *deallocated_at = Some(machine.current_span()); } - machine.alloc_addresses.get_mut().free_alloc_id( - machine.rng.get_mut(), - alloc_id, - size, - align, - ); + machine.free_alloc_id(alloc_id, size, align); Ok(()) } diff --git a/src/tools/miri/tests/pass/concurrency/address_reuse_happens_before.rs b/src/tools/miri/tests/pass/concurrency/address_reuse_happens_before.rs new file mode 100644 index 00000000000..c0de1b48263 --- /dev/null +++ b/src/tools/miri/tests/pass/concurrency/address_reuse_happens_before.rs @@ -0,0 +1,60 @@ +//! Regression test for : +//! When the address gets reused, there should be a happens-before relation. +#![feature(strict_provenance)] +#![feature(sync_unsafe_cell)] + +use std::cell::SyncUnsafeCell; +use std::sync::atomic::{AtomicUsize, Ordering::Relaxed}; +use std::thread; + +static ADDR: AtomicUsize = AtomicUsize::new(0); +static VAL: SyncUnsafeCell = SyncUnsafeCell::new(0); + +fn addr() -> usize { + let alloc = Box::new(42); + <*const i32>::addr(&*alloc) +} + +fn thread1() { + unsafe { + VAL.get().write(42); + } + let alloc = addr(); + ADDR.store(alloc, Relaxed); +} + +fn thread2() -> bool { + // We try to get an allocation at the same address as the global `ADDR`. If we fail too often, + // just bail. `main` will try again with a different allocation. + for _ in 0..16 { + let alloc = addr(); + let addr = ADDR.load(Relaxed); + if alloc == addr { + // We got a reuse! + // If the new allocation is at the same address as the old one, there must be a + // happens-before relationship between them. Therefore, we can read VAL without racing + // and must observe the write above. + let val = unsafe { VAL.get().read() }; + assert_eq!(val, 42); + return true; + } + } + + false +} + +fn main() { + let mut success = false; + while !success { + let t1 = thread::spawn(thread1); + let t2 = thread::spawn(thread2); + t1.join().unwrap(); + success = t2.join().unwrap(); + + // Reset everything. + ADDR.store(0, Relaxed); + unsafe { + VAL.get().write(0); + } + } +} diff --git a/src/tools/miri/tests/pass/weak_memory/weak.rs b/src/tools/miri/tests/pass/weak_memory/weak.rs index e10ccc277f6..dac63eeeb0b 100644 --- a/src/tools/miri/tests/pass/weak_memory/weak.rs +++ b/src/tools/miri/tests/pass/weak_memory/weak.rs @@ -37,6 +37,8 @@ fn relaxed() -> bool { let x = static_atomic(0); let j1 = spawn(move || { x.store(1, Relaxed); + // Preemption is disabled, so the store above will never be the + // latest store visible to another thread. x.store(2, Relaxed); }); @@ -138,6 +140,7 @@ fn faa_replaced_by_load() -> bool { } /// Asserts that the function returns true at least once in 100 runs +#[track_caller] fn assert_once(f: fn() -> bool) { assert!(std::iter::repeat_with(|| f()).take(100).any(|x| x)); } From d7916fc8cd52997ad7727f9cf5a9d863057e90fc Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Thu, 18 Apr 2024 13:20:01 +0200 Subject: [PATCH 20/33] do not reuse stack addresses; make reuse rate configurable --- src/tools/miri/README.md | 5 +++++ src/tools/miri/src/alloc_addresses/mod.rs | 14 +++++++----- .../miri/src/alloc_addresses/reuse_pool.rs | 22 ++++++++++--------- src/tools/miri/src/bin/miri.rs | 14 ++++++++++++ src/tools/miri/src/eval.rs | 3 +++ src/tools/miri/src/machine.rs | 4 ++-- 6 files changed, 45 insertions(+), 17 deletions(-) diff --git a/src/tools/miri/README.md b/src/tools/miri/README.md index 948f1ee6c63..f01ce52f056 100644 --- a/src/tools/miri/README.md +++ b/src/tools/miri/README.md @@ -295,6 +295,11 @@ up the sysroot. If you are using `miri` (the Miri driver) directly, see the Miri adds its own set of `-Z` flags, which are usually set via the `MIRIFLAGS` environment variable. We first document the most relevant and most commonly used flags: +* `-Zmiri-address-reuse-rate=` changes the probability that a freed *non-stack* allocation + will be added to the pool for address reuse, and the probability that a new *non-stack* allocation + will be taken from the pool. Stack allocations never get added to or taken from the pool. The + default is `0.5`. Note that a very high reuse rate can mask concurrency bugs as address + reuse induces synchronization between threads. * `-Zmiri-compare-exchange-weak-failure-rate=` changes the failure rate of `compare_exchange_weak` operations. The default is `0.8` (so 4 out of 5 weak ops will fail). You can change it to any value between `0.0` and `1.0`, where `1.0` means it diff --git a/src/tools/miri/src/alloc_addresses/mod.rs b/src/tools/miri/src/alloc_addresses/mod.rs index 50142d6b5a8..247f8292978 100644 --- a/src/tools/miri/src/alloc_addresses/mod.rs +++ b/src/tools/miri/src/alloc_addresses/mod.rs @@ -78,7 +78,7 @@ impl GlobalStateInner { GlobalStateInner { int_to_ptr_map: Vec::default(), base_addr: FxHashMap::default(), - reuse: ReusePool::new(), + reuse: ReusePool::new(config.address_reuse_rate), exposed: FxHashSet::default(), next_base_addr: stack_addr, provenance_mode: config.provenance_mode, @@ -142,7 +142,11 @@ trait EvalContextExtPriv<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { } } - fn addr_from_alloc_id(&self, alloc_id: AllocId, _kind: MemoryKind) -> InterpResult<'tcx, u64> { + fn addr_from_alloc_id( + &self, + alloc_id: AllocId, + memory_kind: MemoryKind, + ) -> InterpResult<'tcx, u64> { let ecx = self.eval_context_ref(); let mut global_state = ecx.machine.alloc_addresses.borrow_mut(); let global_state = &mut *global_state; @@ -161,7 +165,7 @@ trait EvalContextExtPriv<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { // This allocation does not have a base address yet, pick or reuse one. let base_addr = if let Some((reuse_addr, clock)) = - global_state.reuse.take_addr(&mut *rng, size, align) + global_state.reuse.take_addr(&mut *rng, size, align, memory_kind) { if let Some(data_race) = &ecx.machine.data_race { data_race.validate_lock_acquire(&clock, ecx.get_active_thread()); @@ -334,7 +338,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { } impl<'mir, 'tcx> MiriMachine<'mir, 'tcx> { - pub fn free_alloc_id(&mut self, dead_id: AllocId, size: Size, align: Align) { + pub fn free_alloc_id(&mut self, dead_id: AllocId, size: Size, align: Align, kind: MemoryKind) { let global_state = self.alloc_addresses.get_mut(); let rng = self.rng.get_mut(); @@ -359,7 +363,7 @@ impl<'mir, 'tcx> MiriMachine<'mir, 'tcx> { // `alloc_id_from_addr` any more. global_state.exposed.remove(&dead_id); // Also remember this address for future reuse. - global_state.reuse.add_addr(rng, addr, size, align, || { + global_state.reuse.add_addr(rng, addr, size, align, kind, || { let mut clock = concurrency::VClock::default(); if let Some(data_race) = &self.data_race { data_race.validate_lock_release( diff --git a/src/tools/miri/src/alloc_addresses/reuse_pool.rs b/src/tools/miri/src/alloc_addresses/reuse_pool.rs index 9af4bdac4cf..3b2f0269ebe 100644 --- a/src/tools/miri/src/alloc_addresses/reuse_pool.rs +++ b/src/tools/miri/src/alloc_addresses/reuse_pool.rs @@ -4,20 +4,17 @@ use rand::Rng; use rustc_target::abi::{Align, Size}; -use crate::concurrency::VClock; +use crate::{concurrency::VClock, MemoryKind}; const MAX_POOL_SIZE: usize = 64; -// Just use fair coins, until we have evidence that other numbers are better. -const ADDR_REMEMBER_CHANCE: f64 = 0.5; -const ADDR_TAKE_CHANCE: f64 = 0.5; - /// The pool strikes a balance between exploring more possible executions and making it more likely /// to find bugs. The hypothesis is that bugs are more likely to occur when reuse happens for /// allocations with the same layout, since that can trigger e.g. ABA issues in a concurrent data /// structure. Therefore we only reuse allocations when size and alignment match exactly. #[derive(Debug)] pub struct ReusePool { + address_reuse_rate: f64, /// The i-th element in `pool` stores allocations of alignment `2^i`. We store these reusable /// allocations as address-size pairs, the list must be sorted by the size. /// @@ -30,8 +27,8 @@ pub struct ReusePool { } impl ReusePool { - pub fn new() -> Self { - ReusePool { pool: vec![] } + pub fn new(address_reuse_rate: f64) -> Self { + ReusePool { address_reuse_rate, pool: vec![] } } fn subpool(&mut self, align: Align) -> &mut Vec<(u64, Size, VClock)> { @@ -48,10 +45,14 @@ impl ReusePool { addr: u64, size: Size, align: Align, + kind: MemoryKind, clock: impl FnOnce() -> VClock, ) { // Let's see if we even want to remember this address. - if !rng.gen_bool(ADDR_REMEMBER_CHANCE) { + // We don't remember stack addresses: there's a lot of them (so the perf impact is big), + // and we only want to reuse stack slots within the same thread or else we'll add a lot of + // undesired synchronization. + if kind == MemoryKind::Stack || !rng.gen_bool(self.address_reuse_rate) { return; } // Determine the pool to add this to, and where in the pool to put it. @@ -73,9 +74,10 @@ impl ReusePool { rng: &mut impl Rng, size: Size, align: Align, + kind: MemoryKind, ) -> Option<(u64, VClock)> { - // Determine whether we'll even attempt a reuse. - if !rng.gen_bool(ADDR_TAKE_CHANCE) { + // Determine whether we'll even attempt a reuse. As above, we don't do reuse for stack addresses. + if kind == MemoryKind::Stack || !rng.gen_bool(self.address_reuse_rate) { return None; } // Determine the pool to take this from. diff --git a/src/tools/miri/src/bin/miri.rs b/src/tools/miri/src/bin/miri.rs index 3f7a965e9df..263a78257f6 100644 --- a/src/tools/miri/src/bin/miri.rs +++ b/src/tools/miri/src/bin/miri.rs @@ -542,6 +542,20 @@ fn main() { miri_config.tracked_alloc_ids.extend(ids); } else if arg == "-Zmiri-track-alloc-accesses" { miri_config.track_alloc_accesses = true; + } else if let Some(param) = arg.strip_prefix("-Zmiri-address-reuse-rate=") { + let rate = match param.parse::() { + Ok(rate) if rate >= 0.0 && rate <= 1.0 => rate, + Ok(_) => + show_error!( + "-Zmiri-compare-exchange-weak-failure-rate must be between `0.0` and `1.0`" + ), + Err(err) => + show_error!( + "-Zmiri-compare-exchange-weak-failure-rate requires a `f64` between `0.0` and `1.0`: {}", + err + ), + }; + miri_config.address_reuse_rate = rate; } else if let Some(param) = arg.strip_prefix("-Zmiri-compare-exchange-weak-failure-rate=") { let rate = match param.parse::() { Ok(rate) if rate >= 0.0 && rate <= 1.0 => rate, diff --git a/src/tools/miri/src/eval.rs b/src/tools/miri/src/eval.rs index df0ede1e1b6..457fe740d66 100644 --- a/src/tools/miri/src/eval.rs +++ b/src/tools/miri/src/eval.rs @@ -150,6 +150,8 @@ pub struct MiriConfig { pub page_size: Option, /// Whether to collect a backtrace when each allocation is created, just in case it leaks. pub collect_leak_backtraces: bool, + /// Probability for address reuse. + pub address_reuse_rate: f64, } impl Default for MiriConfig { @@ -186,6 +188,7 @@ impl Default for MiriConfig { num_cpus: 1, page_size: None, collect_leak_backtraces: true, + address_reuse_rate: 0.5, } } } diff --git a/src/tools/miri/src/machine.rs b/src/tools/miri/src/machine.rs index d7c762fe1a9..eec162a87ed 100644 --- a/src/tools/miri/src/machine.rs +++ b/src/tools/miri/src/machine.rs @@ -1282,7 +1282,7 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for MiriMachine<'mir, 'tcx> { (alloc_id, prove_extra): (AllocId, Self::ProvenanceExtra), size: Size, align: Align, - _kind: MemoryKind, + kind: MemoryKind, ) -> InterpResult<'tcx> { if machine.tracked_alloc_ids.contains(&alloc_id) { machine.emit_diagnostic(NonHaltingDiagnostic::FreedAlloc(alloc_id)); @@ -1303,7 +1303,7 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for MiriMachine<'mir, 'tcx> { { *deallocated_at = Some(machine.current_span()); } - machine.free_alloc_id(alloc_id, size, align); + machine.free_alloc_id(alloc_id, size, align, kind); Ok(()) } From 5067dd21b6e89fe74e588b8a11a0c8d3dd127029 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Thu, 18 Apr 2024 22:17:10 +0200 Subject: [PATCH 21/33] comment clarification and typo fix --- src/tools/miri/src/shims/os_str.rs | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/src/tools/miri/src/shims/os_str.rs b/src/tools/miri/src/shims/os_str.rs index 0409e31d65a..925a35beb62 100644 --- a/src/tools/miri/src/shims/os_str.rs +++ b/src/tools/miri/src/shims/os_str.rs @@ -260,6 +260,8 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { let this = self.eval_context_ref(); let target_os = &this.tcx.sess.target.os; + // Below we assume that everything non-Windows works like Unix, at least + // when it comes to file system path conventions. #[cfg(windows)] return if target_os == "windows" { // Windows-on-Windows, all fine. @@ -297,6 +299,12 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { { converted.remove(0); } + // If the path starts with `\\`, it is a magic Windows path. Conveniently, paths + // starting with `//` on Unix are also magic where the first component can have + // "application-specific" meaning, which is reflected e.g. by `path::absolute` + // leaving leading `//` alone (but normalizing leading `///` to `/`). So we + // don't have to do anything, the magic Windows path should work mostly fine as + // a magic Unix path. } } Cow::Owned(OsString::from_wide(&converted)) @@ -324,13 +332,15 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { { converted.remove(0); } - // If this start withs a `\` but not a `\\`, then for Windows this is a relative - // path. But the host path is absolute as it started with `/`. We add `\\?` so - // it starts with `\\?\` which is some magic path on Windows that *is* - // considered absolute. + // If this starts withs a `\` but not a `\\`, then for Windows this is a + // relative path (relative to "the root of the current directory", e.g. the + // drive letter). But the host path on Unix is absolute as it starts with `/`. else if converted.get(0).copied() == Some(b'\\') && converted.get(1).copied() != Some(b'\\') { + // We add `\\?` so it starts with `\\?\` which is some magic path on Windows + // that *is* considered absolute. This way we store the absolute host path + // in something that looks like an absolute path to the (Windows) target. converted.splice(0..0, b"\\\\?".iter().copied()); } } From 9b419a1b83b41d22b400c67af82da596ce77de1a Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Thu, 18 Apr 2024 22:59:28 +0200 Subject: [PATCH 22/33] when reusing an address, most of the time only reuse from the current thread --- src/tools/miri/README.md | 9 +- src/tools/miri/src/alloc_addresses/mod.rs | 17 ++- .../miri/src/alloc_addresses/reuse_pool.rs | 49 +++++-- src/tools/miri/src/bin/miri.rs | 129 +++++++----------- src/tools/miri/src/eval.rs | 3 + .../both_borrows/retag_data_race_write.rs | 2 + .../tests/fail/data_race/alloc_read_race.rs | 2 + .../tests/fail/data_race/alloc_write_race.rs | 2 + .../data_race/atomic_read_na_write_race1.rs | 2 + .../data_race/atomic_read_na_write_race2.rs | 2 + .../data_race/atomic_write_na_read_race1.rs | 2 + .../data_race/atomic_write_na_read_race2.rs | 2 + .../data_race/atomic_write_na_write_race1.rs | 2 + .../data_race/atomic_write_na_write_race2.rs | 2 + .../data_race/dangling_thread_async_race.rs | 2 + .../fail/data_race/dangling_thread_race.rs | 2 + .../fail/data_race/dealloc_read_race1.rs | 2 + .../fail/data_race/dealloc_read_race2.rs | 2 + .../fail/data_race/dealloc_read_race_stack.rs | 2 + .../fail/data_race/dealloc_write_race1.rs | 2 + .../fail/data_race/dealloc_write_race2.rs | 2 + .../data_race/dealloc_write_race_stack.rs | 2 + .../data_race/enable_after_join_to_main.rs | 2 + .../tests/fail/data_race/fence_after_load.rs | 3 + .../tests/fail/data_race/mixed_size_read.rs | 3 + .../tests/fail/data_race/mixed_size_write.rs | 3 + .../tests/fail/data_race/read_read_race1.rs | 3 + .../tests/fail/data_race/read_read_race2.rs | 3 + .../tests/fail/data_race/read_write_race.rs | 2 + .../fail/data_race/read_write_race_stack.rs | 2 + .../fail/data_race/relax_acquire_race.rs | 2 + .../tests/fail/data_race/release_seq_race.rs | 2 + .../data_race/release_seq_race_same_thread.rs | 2 + .../miri/tests/fail/data_race/rmw_race.rs | 2 + .../tests/fail/data_race/stack_pop_race.rs | 3 + .../tests/fail/data_race/write_write_race.rs | 2 + .../fail/data_race/write_write_race_stack.rs | 2 + .../retag_data_race_protected_read.rs | 3 +- .../stacked_borrows/retag_data_race_read.rs | 3 +- .../fail/weak_memory/racing_mixed_size.rs | 3 +- .../weak_memory/racing_mixed_size_read.rs | 3 +- .../address_reuse_happens_before.rs | 1 + .../concurrency/disable_data_race_detector.rs | 2 + 43 files changed, 183 insertions(+), 109 deletions(-) diff --git a/src/tools/miri/README.md b/src/tools/miri/README.md index f01ce52f056..4254b9bb67d 100644 --- a/src/tools/miri/README.md +++ b/src/tools/miri/README.md @@ -298,8 +298,13 @@ environment variable. We first document the most relevant and most commonly used * `-Zmiri-address-reuse-rate=` changes the probability that a freed *non-stack* allocation will be added to the pool for address reuse, and the probability that a new *non-stack* allocation will be taken from the pool. Stack allocations never get added to or taken from the pool. The - default is `0.5`. Note that a very high reuse rate can mask concurrency bugs as address - reuse induces synchronization between threads. + default is `0.5`. +* `-Zmiri-address-reuse-cross-thread-rate=` changes the probability that an allocation which + attempts to reuse a previously freed block of memory will also consider blocks freed by *other + threads*. The default is `0.1`, which means by default, in 90% of the cases where an address reuse + attempt is made, only addresses from the same thread will be considered. Reusing an address from + another thread induces synchronization between those threads, which can mask data races and weak + memory bugs. * `-Zmiri-compare-exchange-weak-failure-rate=` changes the failure rate of `compare_exchange_weak` operations. The default is `0.8` (so 4 out of 5 weak ops will fail). You can change it to any value between `0.0` and `1.0`, where `1.0` means it diff --git a/src/tools/miri/src/alloc_addresses/mod.rs b/src/tools/miri/src/alloc_addresses/mod.rs index 247f8292978..612649c90c6 100644 --- a/src/tools/miri/src/alloc_addresses/mod.rs +++ b/src/tools/miri/src/alloc_addresses/mod.rs @@ -78,7 +78,7 @@ impl GlobalStateInner { GlobalStateInner { int_to_ptr_map: Vec::default(), base_addr: FxHashMap::default(), - reuse: ReusePool::new(config.address_reuse_rate), + reuse: ReusePool::new(config), exposed: FxHashSet::default(), next_base_addr: stack_addr, provenance_mode: config.provenance_mode, @@ -164,9 +164,13 @@ trait EvalContextExtPriv<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { assert!(!matches!(kind, AllocKind::Dead)); // This allocation does not have a base address yet, pick or reuse one. - let base_addr = if let Some((reuse_addr, clock)) = - global_state.reuse.take_addr(&mut *rng, size, align, memory_kind) - { + let base_addr = if let Some((reuse_addr, clock)) = global_state.reuse.take_addr( + &mut *rng, + size, + align, + memory_kind, + ecx.get_active_thread(), + ) { if let Some(data_race) = &ecx.machine.data_race { data_race.validate_lock_acquire(&clock, ecx.get_active_thread()); } @@ -363,12 +367,13 @@ impl<'mir, 'tcx> MiriMachine<'mir, 'tcx> { // `alloc_id_from_addr` any more. global_state.exposed.remove(&dead_id); // Also remember this address for future reuse. - global_state.reuse.add_addr(rng, addr, size, align, kind, || { + let thread = self.threads.get_active_thread_id(); + global_state.reuse.add_addr(rng, addr, size, align, kind, thread, || { let mut clock = concurrency::VClock::default(); if let Some(data_race) = &self.data_race { data_race.validate_lock_release( &mut clock, - self.threads.get_active_thread_id(), + thread, self.threads.active_thread_ref().current_span(), ); } diff --git a/src/tools/miri/src/alloc_addresses/reuse_pool.rs b/src/tools/miri/src/alloc_addresses/reuse_pool.rs index 3b2f0269ebe..fd0b24cc29b 100644 --- a/src/tools/miri/src/alloc_addresses/reuse_pool.rs +++ b/src/tools/miri/src/alloc_addresses/reuse_pool.rs @@ -4,7 +4,7 @@ use rand::Rng; use rustc_target::abi::{Align, Size}; -use crate::{concurrency::VClock, MemoryKind}; +use crate::{concurrency::VClock, MemoryKind, MiriConfig, ThreadId}; const MAX_POOL_SIZE: usize = 64; @@ -15,23 +15,28 @@ const MAX_POOL_SIZE: usize = 64; #[derive(Debug)] pub struct ReusePool { address_reuse_rate: f64, + address_reuse_cross_thread_rate: f64, /// The i-th element in `pool` stores allocations of alignment `2^i`. We store these reusable - /// allocations as address-size pairs, the list must be sorted by the size. + /// allocations as address-size pairs, the list must be sorted by the size and then the thread ID. /// /// Each of these maps has at most MAX_POOL_SIZE elements, and since alignment is limited to /// less than 64 different possible value, that bounds the overall size of the pool. /// - /// We also store the clock from the thread that donated this pool element, + /// We also store the ID and the data-race clock of the thread that donated this pool element, /// to ensure synchronization with the thread that picks up this address. - pool: Vec>, + pool: Vec>, } impl ReusePool { - pub fn new(address_reuse_rate: f64) -> Self { - ReusePool { address_reuse_rate, pool: vec![] } + pub fn new(config: &MiriConfig) -> Self { + ReusePool { + address_reuse_rate: config.address_reuse_rate, + address_reuse_cross_thread_rate: config.address_reuse_cross_thread_rate, + pool: vec![], + } } - fn subpool(&mut self, align: Align) -> &mut Vec<(u64, Size, VClock)> { + fn subpool(&mut self, align: Align) -> &mut Vec<(u64, Size, ThreadId, VClock)> { let pool_idx: usize = align.bytes().trailing_zeros().try_into().unwrap(); if self.pool.len() <= pool_idx { self.pool.resize(pool_idx + 1, Vec::new()); @@ -46,6 +51,7 @@ impl ReusePool { size: Size, align: Align, kind: MemoryKind, + thread: ThreadId, clock: impl FnOnce() -> VClock, ) { // Let's see if we even want to remember this address. @@ -55,18 +61,21 @@ impl ReusePool { if kind == MemoryKind::Stack || !rng.gen_bool(self.address_reuse_rate) { return; } + let clock = clock(); // Determine the pool to add this to, and where in the pool to put it. let subpool = self.subpool(align); - let pos = subpool.partition_point(|(_addr, other_size, _)| *other_size < size); + let pos = subpool.partition_point(|(_addr, other_size, other_thread, _)| { + (*other_size, *other_thread) < (size, thread) + }); // Make sure the pool does not grow too big. if subpool.len() >= MAX_POOL_SIZE { // Pool full. Replace existing element, or last one if this would be even bigger. let clamped_pos = pos.min(subpool.len() - 1); - subpool[clamped_pos] = (addr, size, clock()); + subpool[clamped_pos] = (addr, size, thread, clock); return; } // Add address to pool, at the right position. - subpool.insert(pos, (addr, size, clock())); + subpool.insert(pos, (addr, size, thread, clock)); } pub fn take_addr( @@ -75,21 +84,32 @@ impl ReusePool { size: Size, align: Align, kind: MemoryKind, + thread: ThreadId, ) -> Option<(u64, VClock)> { // Determine whether we'll even attempt a reuse. As above, we don't do reuse for stack addresses. if kind == MemoryKind::Stack || !rng.gen_bool(self.address_reuse_rate) { return None; } + let cross_thread_reuse = rng.gen_bool(self.address_reuse_cross_thread_rate); // Determine the pool to take this from. let subpool = self.subpool(align); // Let's see if we can find something of the right size. We want to find the full range of - // such items, beginning with the first, so we can't use `binary_search_by_key`. - let begin = subpool.partition_point(|(_addr, other_size, _)| *other_size < size); + // such items, beginning with the first, so we can't use `binary_search_by_key`. If we do + // *not* want to consider other thread's allocations, we effectively use the lexicographic + // order on `(size, thread)`. + let begin = subpool.partition_point(|(_addr, other_size, other_thread, _)| { + *other_size < size + || (*other_size == size && !cross_thread_reuse && *other_thread < thread) + }); let mut end = begin; - while let Some((_addr, other_size, _)) = subpool.get(end) { + while let Some((_addr, other_size, other_thread, _)) = subpool.get(end) { if *other_size != size { break; } + if !cross_thread_reuse && *other_thread != thread { + // We entered the allocations of another thread. + break; + } end += 1; } if end == begin { @@ -99,8 +119,9 @@ impl ReusePool { // Pick a random element with the desired size. let idx = rng.gen_range(begin..end); // Remove it from the pool and return. - let (chosen_addr, chosen_size, clock) = subpool.remove(idx); + let (chosen_addr, chosen_size, chosen_thread, clock) = subpool.remove(idx); debug_assert!(chosen_size >= size && chosen_addr % align.bytes() == 0); + debug_assert!(cross_thread_reuse || chosen_thread == thread); Some((chosen_addr, clock)) } } diff --git a/src/tools/miri/src/bin/miri.rs b/src/tools/miri/src/bin/miri.rs index 263a78257f6..db2cd01ce0b 100644 --- a/src/tools/miri/src/bin/miri.rs +++ b/src/tools/miri/src/bin/miri.rs @@ -307,6 +307,15 @@ fn parse_comma_list(input: &str) -> Result, T::Err> { input.split(',').map(str::parse::).collect() } +/// Parses the input as a float in the range from 0.0 to 1.0 (inclusive). +fn parse_rate(input: &str) -> Result { + match input.parse::() { + Ok(rate) if rate >= 0.0 && rate <= 1.0 => Ok(rate), + Ok(_) => Err("must be between `0.0` and `1.0`"), + Err(_) => Err("requires a `f64` between `0.0` and `1.0`"), + } +} + #[cfg(any(target_os = "linux", target_os = "macos"))] fn jemalloc_magic() { // These magic runes are copied from @@ -499,14 +508,9 @@ fn main() { } else if let Some(param) = arg.strip_prefix("-Zmiri-env-forward=") { miri_config.forwarded_env_vars.push(param.to_owned()); } else if let Some(param) = arg.strip_prefix("-Zmiri-track-pointer-tag=") { - let ids: Vec = match parse_comma_list(param) { - Ok(ids) => ids, - Err(err) => - show_error!( - "-Zmiri-track-pointer-tag requires a comma separated list of valid `u64` arguments: {}", - err - ), - }; + let ids: Vec = parse_comma_list(param).unwrap_or_else(|err| { + show_error!("-Zmiri-track-pointer-tag requires a comma separated list of valid `u64` arguments: {err}") + }); for id in ids.into_iter().map(miri::BorTag::new) { if let Some(id) = id { miri_config.tracked_pointer_tags.insert(id); @@ -515,14 +519,9 @@ fn main() { } } } else if let Some(param) = arg.strip_prefix("-Zmiri-track-call-id=") { - let ids: Vec = match parse_comma_list(param) { - Ok(ids) => ids, - Err(err) => - show_error!( - "-Zmiri-track-call-id requires a comma separated list of valid `u64` arguments: {}", - err - ), - }; + let ids: Vec = parse_comma_list(param).unwrap_or_else(|err| { + show_error!("-Zmiri-track-call-id requires a comma separated list of valid `u64` arguments: {err}") + }); for id in ids.into_iter().map(miri::CallId::new) { if let Some(id) = id { miri_config.tracked_call_ids.insert(id); @@ -531,70 +530,37 @@ fn main() { } } } else if let Some(param) = arg.strip_prefix("-Zmiri-track-alloc-id=") { - let ids: Vec = match parse_comma_list::>(param) { - Ok(ids) => ids.into_iter().map(miri::AllocId).collect(), - Err(err) => - show_error!( - "-Zmiri-track-alloc-id requires a comma separated list of valid non-zero `u64` arguments: {}", - err - ), - }; - miri_config.tracked_alloc_ids.extend(ids); + let ids = parse_comma_list::>(param).unwrap_or_else(|err| { + show_error!("-Zmiri-track-alloc-id requires a comma separated list of valid non-zero `u64` arguments: {err}") + }); + miri_config.tracked_alloc_ids.extend(ids.into_iter().map(miri::AllocId)); } else if arg == "-Zmiri-track-alloc-accesses" { miri_config.track_alloc_accesses = true; } else if let Some(param) = arg.strip_prefix("-Zmiri-address-reuse-rate=") { - let rate = match param.parse::() { - Ok(rate) if rate >= 0.0 && rate <= 1.0 => rate, - Ok(_) => - show_error!( - "-Zmiri-compare-exchange-weak-failure-rate must be between `0.0` and `1.0`" - ), - Err(err) => - show_error!( - "-Zmiri-compare-exchange-weak-failure-rate requires a `f64` between `0.0` and `1.0`: {}", - err - ), - }; - miri_config.address_reuse_rate = rate; + miri_config.address_reuse_rate = parse_rate(param) + .unwrap_or_else(|err| show_error!("-Zmiri-address-reuse-rate {err}")); + } else if let Some(param) = arg.strip_prefix("-Zmiri-address-reuse-cross-thread-rate=") { + miri_config.address_reuse_cross_thread_rate = parse_rate(param) + .unwrap_or_else(|err| show_error!("-Zmiri-address-reuse-cross-thread-rate {err}")); } else if let Some(param) = arg.strip_prefix("-Zmiri-compare-exchange-weak-failure-rate=") { - let rate = match param.parse::() { - Ok(rate) if rate >= 0.0 && rate <= 1.0 => rate, - Ok(_) => - show_error!( - "-Zmiri-compare-exchange-weak-failure-rate must be between `0.0` and `1.0`" - ), - Err(err) => - show_error!( - "-Zmiri-compare-exchange-weak-failure-rate requires a `f64` between `0.0` and `1.0`: {}", - err - ), - }; - miri_config.cmpxchg_weak_failure_rate = rate; + miri_config.cmpxchg_weak_failure_rate = parse_rate(param).unwrap_or_else(|err| { + show_error!("-Zmiri-compare-exchange-weak-failure-rate {err}") + }); } else if let Some(param) = arg.strip_prefix("-Zmiri-preemption-rate=") { - let rate = match param.parse::() { - Ok(rate) if rate >= 0.0 && rate <= 1.0 => rate, - Ok(_) => show_error!("-Zmiri-preemption-rate must be between `0.0` and `1.0`"), - Err(err) => - show_error!( - "-Zmiri-preemption-rate requires a `f64` between `0.0` and `1.0`: {}", - err - ), - }; - miri_config.preemption_rate = rate; + miri_config.preemption_rate = + parse_rate(param).unwrap_or_else(|err| show_error!("-Zmiri-preemption-rate {err}")); } else if arg == "-Zmiri-report-progress" { // This makes it take a few seconds between progress reports on my laptop. miri_config.report_progress = Some(1_000_000); } else if let Some(param) = arg.strip_prefix("-Zmiri-report-progress=") { - let interval = match param.parse::() { - Ok(i) => i, - Err(err) => show_error!("-Zmiri-report-progress requires a `u32`: {}", err), - }; + let interval = param.parse::().unwrap_or_else(|err| { + show_error!("-Zmiri-report-progress requires a `u32`: {}", err) + }); miri_config.report_progress = Some(interval); } else if let Some(param) = arg.strip_prefix("-Zmiri-provenance-gc=") { - let interval = match param.parse::() { - Ok(i) => i, - Err(err) => show_error!("-Zmiri-provenance-gc requires a `u32`: {}", err), - }; + let interval = param.parse::().unwrap_or_else(|err| { + show_error!("-Zmiri-provenance-gc requires a `u32`: {}", err) + }); miri_config.gc_interval = interval; } else if let Some(param) = arg.strip_prefix("-Zmiri-measureme=") { miri_config.measureme_out = Some(param.to_string()); @@ -619,23 +585,20 @@ fn main() { show_error!("-Zmiri-extern-so-file `{}` does not exist", filename); } } else if let Some(param) = arg.strip_prefix("-Zmiri-num-cpus=") { - let num_cpus = match param.parse::() { - Ok(i) => i, - Err(err) => show_error!("-Zmiri-num-cpus requires a `u32`: {}", err), - }; - + let num_cpus = param + .parse::() + .unwrap_or_else(|err| show_error!("-Zmiri-num-cpus requires a `u32`: {}", err)); miri_config.num_cpus = num_cpus; } else if let Some(param) = arg.strip_prefix("-Zmiri-force-page-size=") { - let page_size = match param.parse::() { - Ok(i) => - if i.is_power_of_two() { - i * 1024 - } else { - show_error!("-Zmiri-force-page-size requires a power of 2: {}", i) - }, - Err(err) => show_error!("-Zmiri-force-page-size requires a `u64`: {}", err), + let page_size = param.parse::().unwrap_or_else(|err| { + show_error!("-Zmiri-force-page-size requires a `u64`: {}", err) + }); + // Convert from kilobytes to bytes. + let page_size = if page_size.is_power_of_two() { + page_size * 1024 + } else { + show_error!("-Zmiri-force-page-size requires a power of 2: {page_size}"); }; - miri_config.page_size = Some(page_size); } else { // Forward to rustc. diff --git a/src/tools/miri/src/eval.rs b/src/tools/miri/src/eval.rs index 457fe740d66..45dadb50f4b 100644 --- a/src/tools/miri/src/eval.rs +++ b/src/tools/miri/src/eval.rs @@ -152,6 +152,8 @@ pub struct MiriConfig { pub collect_leak_backtraces: bool, /// Probability for address reuse. pub address_reuse_rate: f64, + /// Probability for address reuse across threads. + pub address_reuse_cross_thread_rate: f64, } impl Default for MiriConfig { @@ -189,6 +191,7 @@ impl Default for MiriConfig { page_size: None, collect_leak_backtraces: true, address_reuse_rate: 0.5, + address_reuse_cross_thread_rate: 0.1, } } } diff --git a/src/tools/miri/tests/fail/both_borrows/retag_data_race_write.rs b/src/tools/miri/tests/fail/both_borrows/retag_data_race_write.rs index 3edaf10f3dc..0061679eaa4 100644 --- a/src/tools/miri/tests/fail/both_borrows/retag_data_race_write.rs +++ b/src/tools/miri/tests/fail/both_borrows/retag_data_race_write.rs @@ -1,6 +1,8 @@ //! Make sure that a retag acts like a write for the data race model. //@revisions: stack tree //@compile-flags: -Zmiri-preemption-rate=0 +// Avoid accidental synchronization via address reuse inside `thread::spawn`. +//@compile-flags: -Zmiri-address-reuse-cross-thread-rate=0 //@[tree]compile-flags: -Zmiri-tree-borrows #[derive(Copy, Clone)] struct SendPtr(*mut u8); diff --git a/src/tools/miri/tests/fail/data_race/alloc_read_race.rs b/src/tools/miri/tests/fail/data_race/alloc_read_race.rs index 2cf36606907..c85c0ebe244 100644 --- a/src/tools/miri/tests/fail/data_race/alloc_read_race.rs +++ b/src/tools/miri/tests/fail/data_race/alloc_read_race.rs @@ -1,4 +1,6 @@ //@compile-flags: -Zmiri-disable-weak-memory-emulation -Zmiri-preemption-rate=0 -Zmiri-disable-stacked-borrows +// Avoid accidental synchronization via address reuse inside `thread::spawn`. +//@compile-flags: -Zmiri-address-reuse-cross-thread-rate=0 #![feature(new_uninit)] use std::mem::MaybeUninit; diff --git a/src/tools/miri/tests/fail/data_race/alloc_write_race.rs b/src/tools/miri/tests/fail/data_race/alloc_write_race.rs index e95e0e1a841..9e2a430dd94 100644 --- a/src/tools/miri/tests/fail/data_race/alloc_write_race.rs +++ b/src/tools/miri/tests/fail/data_race/alloc_write_race.rs @@ -1,4 +1,6 @@ //@compile-flags: -Zmiri-disable-weak-memory-emulation -Zmiri-preemption-rate=0 -Zmiri-disable-stacked-borrows +// Avoid accidental synchronization via address reuse inside `thread::spawn`. +//@compile-flags: -Zmiri-address-reuse-cross-thread-rate=0 #![feature(new_uninit)] use std::ptr::null_mut; diff --git a/src/tools/miri/tests/fail/data_race/atomic_read_na_write_race1.rs b/src/tools/miri/tests/fail/data_race/atomic_read_na_write_race1.rs index a256267bcda..4003892f0a6 100644 --- a/src/tools/miri/tests/fail/data_race/atomic_read_na_write_race1.rs +++ b/src/tools/miri/tests/fail/data_race/atomic_read_na_write_race1.rs @@ -1,5 +1,7 @@ // We want to control preemption here. Stacked borrows interferes by having its own accesses. //@compile-flags: -Zmiri-preemption-rate=0 -Zmiri-disable-stacked-borrows +// Avoid accidental synchronization via address reuse inside `thread::spawn`. +//@compile-flags: -Zmiri-address-reuse-cross-thread-rate=0 use std::sync::atomic::{AtomicUsize, Ordering}; use std::thread::spawn; diff --git a/src/tools/miri/tests/fail/data_race/atomic_read_na_write_race2.rs b/src/tools/miri/tests/fail/data_race/atomic_read_na_write_race2.rs index cc6a0742c23..8bceba9380a 100644 --- a/src/tools/miri/tests/fail/data_race/atomic_read_na_write_race2.rs +++ b/src/tools/miri/tests/fail/data_race/atomic_read_na_write_race2.rs @@ -1,5 +1,7 @@ // We want to control preemption here. Stacked borrows interferes by having its own accesses. //@compile-flags: -Zmiri-preemption-rate=0 -Zmiri-disable-stacked-borrows +// Avoid accidental synchronization via address reuse inside `thread::spawn`. +//@compile-flags: -Zmiri-address-reuse-cross-thread-rate=0 use std::sync::atomic::AtomicUsize; use std::sync::atomic::Ordering; diff --git a/src/tools/miri/tests/fail/data_race/atomic_write_na_read_race1.rs b/src/tools/miri/tests/fail/data_race/atomic_write_na_read_race1.rs index 7392781e6c6..1a2746a26f4 100644 --- a/src/tools/miri/tests/fail/data_race/atomic_write_na_read_race1.rs +++ b/src/tools/miri/tests/fail/data_race/atomic_write_na_read_race1.rs @@ -1,5 +1,7 @@ // We want to control preemption here. Stacked borrows interferes by having its own accesses. //@compile-flags: -Zmiri-preemption-rate=0 -Zmiri-disable-stacked-borrows +// Avoid accidental synchronization via address reuse inside `thread::spawn`. +//@compile-flags: -Zmiri-address-reuse-cross-thread-rate=0 use std::sync::atomic::AtomicUsize; use std::sync::atomic::Ordering; diff --git a/src/tools/miri/tests/fail/data_race/atomic_write_na_read_race2.rs b/src/tools/miri/tests/fail/data_race/atomic_write_na_read_race2.rs index f681ce0c051..e0876a93fdd 100644 --- a/src/tools/miri/tests/fail/data_race/atomic_write_na_read_race2.rs +++ b/src/tools/miri/tests/fail/data_race/atomic_write_na_read_race2.rs @@ -1,5 +1,7 @@ // We want to control preemption here. Stacked borrows interferes by having its own accesses. //@compile-flags: -Zmiri-preemption-rate=0 -Zmiri-disable-stacked-borrows +// Avoid accidental synchronization via address reuse inside `thread::spawn`. +//@compile-flags: -Zmiri-address-reuse-cross-thread-rate=0 use std::sync::atomic::{AtomicUsize, Ordering}; use std::thread::spawn; diff --git a/src/tools/miri/tests/fail/data_race/atomic_write_na_write_race1.rs b/src/tools/miri/tests/fail/data_race/atomic_write_na_write_race1.rs index 47a3ef5d168..1010216a497 100644 --- a/src/tools/miri/tests/fail/data_race/atomic_write_na_write_race1.rs +++ b/src/tools/miri/tests/fail/data_race/atomic_write_na_write_race1.rs @@ -1,5 +1,7 @@ // We want to control preemption here. Stacked borrows interferes by having its own accesses. //@compile-flags: -Zmiri-preemption-rate=0 -Zmiri-disable-stacked-borrows +// Avoid accidental synchronization via address reuse inside `thread::spawn`. +//@compile-flags: -Zmiri-address-reuse-cross-thread-rate=0 use std::sync::atomic::{AtomicUsize, Ordering}; use std::thread::spawn; diff --git a/src/tools/miri/tests/fail/data_race/atomic_write_na_write_race2.rs b/src/tools/miri/tests/fail/data_race/atomic_write_na_write_race2.rs index 8bba4a88924..b494bd3a003 100644 --- a/src/tools/miri/tests/fail/data_race/atomic_write_na_write_race2.rs +++ b/src/tools/miri/tests/fail/data_race/atomic_write_na_write_race2.rs @@ -1,5 +1,7 @@ // We want to control preemption here. Stacked borrows interferes by having its own accesses. //@compile-flags: -Zmiri-preemption-rate=0 -Zmiri-disable-stacked-borrows +// Avoid accidental synchronization via address reuse inside `thread::spawn`. +//@compile-flags: -Zmiri-address-reuse-cross-thread-rate=0 use std::sync::atomic::AtomicUsize; use std::sync::atomic::Ordering; diff --git a/src/tools/miri/tests/fail/data_race/dangling_thread_async_race.rs b/src/tools/miri/tests/fail/data_race/dangling_thread_async_race.rs index 5b9005606e0..dffafe3cfaa 100644 --- a/src/tools/miri/tests/fail/data_race/dangling_thread_async_race.rs +++ b/src/tools/miri/tests/fail/data_race/dangling_thread_async_race.rs @@ -1,5 +1,7 @@ // We want to control preemption here. Stacked borrows interferes by having its own accesses. //@compile-flags: -Zmiri-preemption-rate=0 -Zmiri-disable-stacked-borrows +// Avoid accidental synchronization via address reuse. +//@compile-flags: -Zmiri-address-reuse-cross-thread-rate=0 use std::mem; use std::thread::{sleep, spawn}; diff --git a/src/tools/miri/tests/fail/data_race/dangling_thread_race.rs b/src/tools/miri/tests/fail/data_race/dangling_thread_race.rs index 91c1191e036..8dc35c7ea72 100644 --- a/src/tools/miri/tests/fail/data_race/dangling_thread_race.rs +++ b/src/tools/miri/tests/fail/data_race/dangling_thread_race.rs @@ -1,5 +1,7 @@ // We want to control preemption here. Stacked borrows interferes by having its own accesses. //@compile-flags: -Zmiri-preemption-rate=0 -Zmiri-disable-stacked-borrows +// Avoid accidental synchronization via address reuse. +//@compile-flags: -Zmiri-address-reuse-cross-thread-rate=0 use std::mem; use std::thread::{sleep, spawn}; diff --git a/src/tools/miri/tests/fail/data_race/dealloc_read_race1.rs b/src/tools/miri/tests/fail/data_race/dealloc_read_race1.rs index 5928e471760..f174909e9d5 100644 --- a/src/tools/miri/tests/fail/data_race/dealloc_read_race1.rs +++ b/src/tools/miri/tests/fail/data_race/dealloc_read_race1.rs @@ -1,5 +1,7 @@ // We want to control preemption here. Stacked borrows interferes by having its own accesses. //@compile-flags: -Zmiri-preemption-rate=0 -Zmiri-disable-stacked-borrows +// Avoid accidental synchronization via address reuse inside `thread::spawn`. +//@compile-flags: -Zmiri-address-reuse-cross-thread-rate=0 use std::thread::spawn; diff --git a/src/tools/miri/tests/fail/data_race/dealloc_read_race2.rs b/src/tools/miri/tests/fail/data_race/dealloc_read_race2.rs index c5f82cc9a74..1edfbf5e61c 100644 --- a/src/tools/miri/tests/fail/data_race/dealloc_read_race2.rs +++ b/src/tools/miri/tests/fail/data_race/dealloc_read_race2.rs @@ -1,5 +1,7 @@ // We want to control preemption here. Stacked borrows interferes by having its own accesses. //@compile-flags: -Zmiri-preemption-rate=0 -Zmiri-disable-stacked-borrows +// Avoid accidental synchronization via address reuse inside `thread::spawn`. +//@compile-flags: -Zmiri-address-reuse-cross-thread-rate=0 use std::thread::spawn; diff --git a/src/tools/miri/tests/fail/data_race/dealloc_read_race_stack.rs b/src/tools/miri/tests/fail/data_race/dealloc_read_race_stack.rs index 1095f1e4e82..c67e03d362b 100644 --- a/src/tools/miri/tests/fail/data_race/dealloc_read_race_stack.rs +++ b/src/tools/miri/tests/fail/data_race/dealloc_read_race_stack.rs @@ -1,4 +1,6 @@ //@compile-flags: -Zmiri-disable-weak-memory-emulation -Zmiri-preemption-rate=0 -Zmiri-disable-stacked-borrows +// Avoid accidental synchronization via address reuse inside `thread::spawn`. +//@compile-flags: -Zmiri-address-reuse-cross-thread-rate=0 use std::ptr::null_mut; use std::sync::atomic::{AtomicPtr, Ordering}; diff --git a/src/tools/miri/tests/fail/data_race/dealloc_write_race1.rs b/src/tools/miri/tests/fail/data_race/dealloc_write_race1.rs index b5911e5111b..7605f1911db 100644 --- a/src/tools/miri/tests/fail/data_race/dealloc_write_race1.rs +++ b/src/tools/miri/tests/fail/data_race/dealloc_write_race1.rs @@ -1,5 +1,7 @@ // We want to control preemption here. Stacked borrows interferes by having its own accesses. //@compile-flags: -Zmiri-preemption-rate=0 -Zmiri-disable-stacked-borrows +// Avoid accidental synchronization via address reuse inside `thread::spawn`. +//@compile-flags: -Zmiri-address-reuse-cross-thread-rate=0 use std::thread::spawn; diff --git a/src/tools/miri/tests/fail/data_race/dealloc_write_race2.rs b/src/tools/miri/tests/fail/data_race/dealloc_write_race2.rs index 7a2c882f7ec..4f3819bd636 100644 --- a/src/tools/miri/tests/fail/data_race/dealloc_write_race2.rs +++ b/src/tools/miri/tests/fail/data_race/dealloc_write_race2.rs @@ -1,5 +1,7 @@ // We want to control preemption here. Stacked borrows interferes by having its own accesses. //@compile-flags: -Zmiri-preemption-rate=0 -Zmiri-disable-stacked-borrows +// Avoid accidental synchronization via address reuse inside `thread::spawn`. +//@compile-flags: -Zmiri-address-reuse-cross-thread-rate=0 use std::thread::spawn; diff --git a/src/tools/miri/tests/fail/data_race/dealloc_write_race_stack.rs b/src/tools/miri/tests/fail/data_race/dealloc_write_race_stack.rs index 5ee4cc04a8f..8e63bc1dc7b 100644 --- a/src/tools/miri/tests/fail/data_race/dealloc_write_race_stack.rs +++ b/src/tools/miri/tests/fail/data_race/dealloc_write_race_stack.rs @@ -1,4 +1,6 @@ //@compile-flags: -Zmiri-disable-weak-memory-emulation -Zmiri-preemption-rate=0 -Zmiri-disable-stacked-borrows +// Avoid accidental synchronization via address reuse inside `thread::spawn`. +//@compile-flags: -Zmiri-address-reuse-cross-thread-rate=0 use std::ptr::null_mut; use std::sync::atomic::{AtomicPtr, Ordering}; diff --git a/src/tools/miri/tests/fail/data_race/enable_after_join_to_main.rs b/src/tools/miri/tests/fail/data_race/enable_after_join_to_main.rs index f2da45d7275..53050608d27 100644 --- a/src/tools/miri/tests/fail/data_race/enable_after_join_to_main.rs +++ b/src/tools/miri/tests/fail/data_race/enable_after_join_to_main.rs @@ -1,5 +1,7 @@ // We want to control preemption here. Stacked borrows interferes by having its own accesses. //@compile-flags: -Zmiri-preemption-rate=0 -Zmiri-disable-stacked-borrows +// Avoid accidental synchronization via address reuse inside `thread::spawn`. +//@compile-flags: -Zmiri-address-reuse-cross-thread-rate=0 use std::thread::spawn; diff --git a/src/tools/miri/tests/fail/data_race/fence_after_load.rs b/src/tools/miri/tests/fail/data_race/fence_after_load.rs index 683e3b9c7ac..92cb4ccccf5 100644 --- a/src/tools/miri/tests/fail/data_race/fence_after_load.rs +++ b/src/tools/miri/tests/fail/data_race/fence_after_load.rs @@ -1,5 +1,8 @@ // We want to control preemption here. Stacked borrows interferes by having its own accesses. //@compile-flags: -Zmiri-preemption-rate=0 -Zmiri-disable-stacked-borrows +// Avoid accidental synchronization via address reuse inside `thread::spawn`. +//@compile-flags: -Zmiri-address-reuse-cross-thread-rate=0 + use std::sync::atomic::{fence, AtomicUsize, Ordering}; use std::sync::Arc; use std::thread; diff --git a/src/tools/miri/tests/fail/data_race/mixed_size_read.rs b/src/tools/miri/tests/fail/data_race/mixed_size_read.rs index 091a47070ba..61af972b3dc 100644 --- a/src/tools/miri/tests/fail/data_race/mixed_size_read.rs +++ b/src/tools/miri/tests/fail/data_race/mixed_size_read.rs @@ -1,4 +1,7 @@ //@compile-flags: -Zmiri-preemption-rate=0.0 -Zmiri-disable-weak-memory-emulation +// Avoid accidental synchronization via address reuse inside `thread::spawn`. +//@compile-flags: -Zmiri-address-reuse-cross-thread-rate=0 + use std::sync::atomic::{AtomicU16, AtomicU8, Ordering}; use std::thread; diff --git a/src/tools/miri/tests/fail/data_race/mixed_size_write.rs b/src/tools/miri/tests/fail/data_race/mixed_size_write.rs index 49fb6c1d5c3..12e51bb9429 100644 --- a/src/tools/miri/tests/fail/data_race/mixed_size_write.rs +++ b/src/tools/miri/tests/fail/data_race/mixed_size_write.rs @@ -1,4 +1,7 @@ //@compile-flags: -Zmiri-preemption-rate=0.0 -Zmiri-disable-weak-memory-emulation +// Avoid accidental synchronization via address reuse inside `thread::spawn`. +//@compile-flags: -Zmiri-address-reuse-cross-thread-rate=0 + use std::sync::atomic::{AtomicU16, AtomicU8, Ordering}; use std::thread; diff --git a/src/tools/miri/tests/fail/data_race/read_read_race1.rs b/src/tools/miri/tests/fail/data_race/read_read_race1.rs index f66b5ca3d53..02aa4e4b716 100644 --- a/src/tools/miri/tests/fail/data_race/read_read_race1.rs +++ b/src/tools/miri/tests/fail/data_race/read_read_race1.rs @@ -1,4 +1,7 @@ //@compile-flags: -Zmiri-preemption-rate=0.0 +// Avoid accidental synchronization via address reuse inside `thread::spawn`. +//@compile-flags: -Zmiri-address-reuse-cross-thread-rate=0 + use std::sync::atomic::{AtomicU16, Ordering}; use std::thread; diff --git a/src/tools/miri/tests/fail/data_race/read_read_race2.rs b/src/tools/miri/tests/fail/data_race/read_read_race2.rs index d87b667d912..3b94c9143f3 100644 --- a/src/tools/miri/tests/fail/data_race/read_read_race2.rs +++ b/src/tools/miri/tests/fail/data_race/read_read_race2.rs @@ -1,4 +1,7 @@ //@compile-flags: -Zmiri-preemption-rate=0.0 +// Avoid accidental synchronization via address reuse inside `thread::spawn`. +//@compile-flags: -Zmiri-address-reuse-cross-thread-rate=0 + use std::sync::atomic::{AtomicU16, Ordering}; use std::thread; diff --git a/src/tools/miri/tests/fail/data_race/read_write_race.rs b/src/tools/miri/tests/fail/data_race/read_write_race.rs index 70971b59ffe..adf19dda9d3 100644 --- a/src/tools/miri/tests/fail/data_race/read_write_race.rs +++ b/src/tools/miri/tests/fail/data_race/read_write_race.rs @@ -1,5 +1,7 @@ // We want to control preemption here. Stacked borrows interferes by having its own accesses. //@compile-flags: -Zmiri-preemption-rate=0 -Zmiri-disable-stacked-borrows +// Avoid accidental synchronization via address reuse inside `thread::spawn`. +//@compile-flags: -Zmiri-address-reuse-cross-thread-rate=0 use std::thread::spawn; diff --git a/src/tools/miri/tests/fail/data_race/read_write_race_stack.rs b/src/tools/miri/tests/fail/data_race/read_write_race_stack.rs index 9fec3ceee07..f411767f7b5 100644 --- a/src/tools/miri/tests/fail/data_race/read_write_race_stack.rs +++ b/src/tools/miri/tests/fail/data_race/read_write_race_stack.rs @@ -1,4 +1,6 @@ //@compile-flags: -Zmiri-disable-weak-memory-emulation -Zmiri-preemption-rate=0 -Zmiri-disable-stacked-borrows +// Avoid accidental synchronization via address reuse inside `thread::spawn`. +//@compile-flags: -Zmiri-address-reuse-cross-thread-rate=0 use std::ptr::null_mut; use std::sync::atomic::{AtomicPtr, Ordering}; diff --git a/src/tools/miri/tests/fail/data_race/relax_acquire_race.rs b/src/tools/miri/tests/fail/data_race/relax_acquire_race.rs index be4450794ca..c4f94380822 100644 --- a/src/tools/miri/tests/fail/data_race/relax_acquire_race.rs +++ b/src/tools/miri/tests/fail/data_race/relax_acquire_race.rs @@ -1,4 +1,6 @@ //@compile-flags: -Zmiri-disable-weak-memory-emulation -Zmiri-preemption-rate=0 -Zmiri-disable-stacked-borrows +// Avoid accidental synchronization via address reuse inside `thread::spawn`. +//@compile-flags: -Zmiri-address-reuse-cross-thread-rate=0 use std::sync::atomic::{AtomicUsize, Ordering}; use std::thread::spawn; diff --git a/src/tools/miri/tests/fail/data_race/release_seq_race.rs b/src/tools/miri/tests/fail/data_race/release_seq_race.rs index 9810832413e..f03ab3efa06 100644 --- a/src/tools/miri/tests/fail/data_race/release_seq_race.rs +++ b/src/tools/miri/tests/fail/data_race/release_seq_race.rs @@ -1,4 +1,6 @@ //@compile-flags: -Zmiri-disable-weak-memory-emulation -Zmiri-preemption-rate=0 -Zmiri-disable-stacked-borrows +// Avoid accidental synchronization via address reuse inside `thread::spawn`. +//@compile-flags: -Zmiri-address-reuse-cross-thread-rate=0 use std::sync::atomic::{AtomicUsize, Ordering}; use std::thread::{sleep, spawn}; diff --git a/src/tools/miri/tests/fail/data_race/release_seq_race_same_thread.rs b/src/tools/miri/tests/fail/data_race/release_seq_race_same_thread.rs index 93cbc2a57d6..88ae01b3ca1 100644 --- a/src/tools/miri/tests/fail/data_race/release_seq_race_same_thread.rs +++ b/src/tools/miri/tests/fail/data_race/release_seq_race_same_thread.rs @@ -1,4 +1,6 @@ //@compile-flags: -Zmiri-disable-weak-memory-emulation -Zmiri-preemption-rate=0 -Zmiri-disable-stacked-borrows +// Avoid accidental synchronization via address reuse inside `thread::spawn`. +//@compile-flags: -Zmiri-address-reuse-cross-thread-rate=0 use std::sync::atomic::{AtomicUsize, Ordering}; use std::thread::spawn; diff --git a/src/tools/miri/tests/fail/data_race/rmw_race.rs b/src/tools/miri/tests/fail/data_race/rmw_race.rs index 982e9c1c410..d738caa1058 100644 --- a/src/tools/miri/tests/fail/data_race/rmw_race.rs +++ b/src/tools/miri/tests/fail/data_race/rmw_race.rs @@ -1,4 +1,6 @@ //@compile-flags: -Zmiri-disable-weak-memory-emulation -Zmiri-preemption-rate=0 -Zmiri-disable-stacked-borrows +// Avoid accidental synchronization via address reuse inside `thread::spawn`. +//@compile-flags: -Zmiri-address-reuse-cross-thread-rate=0 use std::sync::atomic::{AtomicUsize, Ordering}; use std::thread::spawn; 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 68d82bc30a5..762a8e51f69 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,7 @@ //@compile-flags: -Zmiri-preemption-rate=0 -Zmiri-disable-stacked-borrows +// Avoid accidental synchronization via address reuse inside `thread::spawn`. +//@compile-flags: -Zmiri-address-reuse-cross-thread-rate=0 + use std::thread; #[derive(Copy, Clone)] diff --git a/src/tools/miri/tests/fail/data_race/write_write_race.rs b/src/tools/miri/tests/fail/data_race/write_write_race.rs index e8924702af8..993d8d25b4c 100644 --- a/src/tools/miri/tests/fail/data_race/write_write_race.rs +++ b/src/tools/miri/tests/fail/data_race/write_write_race.rs @@ -1,5 +1,7 @@ // We want to control preemption here. //@compile-flags: -Zmiri-preemption-rate=0 -Zmiri-disable-stacked-borrows +// Avoid accidental synchronization via address reuse inside `thread::spawn`. +//@compile-flags: -Zmiri-address-reuse-cross-thread-rate=0 use std::thread::spawn; diff --git a/src/tools/miri/tests/fail/data_race/write_write_race_stack.rs b/src/tools/miri/tests/fail/data_race/write_write_race_stack.rs index 984ae2ee83d..8070a7f4fc2 100644 --- a/src/tools/miri/tests/fail/data_race/write_write_race_stack.rs +++ b/src/tools/miri/tests/fail/data_race/write_write_race_stack.rs @@ -1,4 +1,6 @@ //@compile-flags: -Zmiri-disable-weak-memory-emulation -Zmiri-preemption-rate=0 -Zmiri-disable-stacked-borrows +// Avoid accidental synchronization via address reuse inside `thread::spawn`. +//@compile-flags: -Zmiri-address-reuse-cross-thread-rate=0 use std::ptr::null_mut; use std::sync::atomic::{AtomicPtr, Ordering}; diff --git a/src/tools/miri/tests/fail/stacked_borrows/retag_data_race_protected_read.rs b/src/tools/miri/tests/fail/stacked_borrows/retag_data_race_protected_read.rs index 3de517055ec..a6ee7b40c34 100644 --- a/src/tools/miri/tests/fail/stacked_borrows/retag_data_race_protected_read.rs +++ b/src/tools/miri/tests/fail/stacked_borrows/retag_data_race_protected_read.rs @@ -1,4 +1,5 @@ -//@compile-flags: -Zmiri-preemption-rate=0 +// Avoid accidental synchronization via address reuse. +//@compile-flags: -Zmiri-preemption-rate=0 -Zmiri-address-reuse-cross-thread-rate=0 use std::thread; #[derive(Copy, Clone)] diff --git a/src/tools/miri/tests/fail/stacked_borrows/retag_data_race_read.rs b/src/tools/miri/tests/fail/stacked_borrows/retag_data_race_read.rs index 25c92ddf6ca..949f659e7e8 100644 --- a/src/tools/miri/tests/fail/stacked_borrows/retag_data_race_read.rs +++ b/src/tools/miri/tests/fail/stacked_borrows/retag_data_race_read.rs @@ -1,5 +1,6 @@ //! Make sure that a retag acts like a read for the data race model. -//@compile-flags: -Zmiri-preemption-rate=0 +// Avoid accidental synchronization via address reuse. +//@compile-flags: -Zmiri-preemption-rate=0 -Zmiri-address-reuse-cross-thread-rate=0 #[derive(Copy, Clone)] struct SendPtr(*mut u8); diff --git a/src/tools/miri/tests/fail/weak_memory/racing_mixed_size.rs b/src/tools/miri/tests/fail/weak_memory/racing_mixed_size.rs index dfe9397a4c4..1193dddc577 100644 --- a/src/tools/miri/tests/fail/weak_memory/racing_mixed_size.rs +++ b/src/tools/miri/tests/fail/weak_memory/racing_mixed_size.rs @@ -1,5 +1,6 @@ // We want to control preemption here. -//@compile-flags: -Zmiri-preemption-rate=0 +// Avoid accidental synchronization via address reuse. +//@compile-flags: -Zmiri-preemption-rate=0 -Zmiri-address-reuse-cross-thread-rate=0 #![feature(core_intrinsics)] diff --git a/src/tools/miri/tests/fail/weak_memory/racing_mixed_size_read.rs b/src/tools/miri/tests/fail/weak_memory/racing_mixed_size_read.rs index b946a75c3ab..0a0e372f1f3 100644 --- a/src/tools/miri/tests/fail/weak_memory/racing_mixed_size_read.rs +++ b/src/tools/miri/tests/fail/weak_memory/racing_mixed_size_read.rs @@ -1,5 +1,6 @@ // We want to control preemption here. -//@compile-flags: -Zmiri-preemption-rate=0 +// Avoid accidental synchronization via address reuse. +//@compile-flags: -Zmiri-preemption-rate=0 -Zmiri-address-reuse-cross-thread-rate=0 use std::sync::atomic::Ordering::*; use std::sync::atomic::{AtomicU16, AtomicU32}; diff --git a/src/tools/miri/tests/pass/concurrency/address_reuse_happens_before.rs b/src/tools/miri/tests/pass/concurrency/address_reuse_happens_before.rs index c0de1b48263..cfc1ef7ae45 100644 --- a/src/tools/miri/tests/pass/concurrency/address_reuse_happens_before.rs +++ b/src/tools/miri/tests/pass/concurrency/address_reuse_happens_before.rs @@ -1,5 +1,6 @@ //! Regression test for : //! When the address gets reused, there should be a happens-before relation. +//@compile-flags: -Zmiri-address-reuse-cross-thread-rate=1.0 #![feature(strict_provenance)] #![feature(sync_unsafe_cell)] diff --git a/src/tools/miri/tests/pass/concurrency/disable_data_race_detector.rs b/src/tools/miri/tests/pass/concurrency/disable_data_race_detector.rs index 049b5e7f498..354a4bef932 100644 --- a/src/tools/miri/tests/pass/concurrency/disable_data_race_detector.rs +++ b/src/tools/miri/tests/pass/concurrency/disable_data_race_detector.rs @@ -1,4 +1,6 @@ //@compile-flags: -Zmiri-disable-data-race-detector +// Avoid non-determinism +//@compile-flags: -Zmiri-preemption-rate=0 -Zmiri-address-reuse-cross-thread-rate=0 use std::thread::spawn; From c1cf0a3e382a3cfa698dc9fbafc2220189994373 Mon Sep 17 00:00:00 2001 From: The Miri Cronjob Bot Date: Fri, 19 Apr 2024 05:05:48 +0000 Subject: [PATCH 23/33] Preparing for merge from rustc --- src/tools/miri/rust-version | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tools/miri/rust-version b/src/tools/miri/rust-version index f8f3016b15c..f0fa47208a1 100644 --- a/src/tools/miri/rust-version +++ b/src/tools/miri/rust-version @@ -1 +1 @@ -c45dee5efd0c042e9d1e24559ebd0d6424d8aa70 +fa0068b5412baecc932772dda72c0621bfa7ab00 From b9ed4cd70a3d8d4e540dae894589a5e37bc444fb Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 19 Apr 2024 09:17:37 +0200 Subject: [PATCH 24/33] share code between win-to-unix and unix-to-win path conversion --- src/tools/miri/src/shims/os_str.rs | 148 +++++++++++++---------------- 1 file changed, 67 insertions(+), 81 deletions(-) diff --git a/src/tools/miri/src/shims/os_str.rs b/src/tools/miri/src/shims/os_str.rs index 925a35beb62..3e8c35d48ae 100644 --- a/src/tools/miri/src/shims/os_str.rs +++ b/src/tools/miri/src/shims/os_str.rs @@ -251,7 +251,6 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { this.alloc_os_str_as_wide_str(&os_str, memkind) } - #[allow(clippy::get_first)] fn convert_path<'a>( &self, os_str: Cow<'a, OsStr>, @@ -260,6 +259,65 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { let this = self.eval_context_ref(); let target_os = &this.tcx.sess.target.os; + /// Adjust a Windows path to Unix conventions such that it un-does everything that + /// `unix_to_windows` did, and such that if the Windows input path was absolute, then the + /// Unix output path is absolute. + fn windows_to_unix(path: &mut Vec) + where + T: From + Copy + Eq, + { + let sep = T::from(b'/'); + // Make sure all path separators are `/`. + for c in path.iter_mut() { + if *c == b'\\'.into() { + *c = sep; + } + } + // If this starts with `//?/`, it was probably produced by `unix_to_windows`` and we + // remove the `//?` that got added to get the Unix path back out. + if path.get(0..4) == Some(&[sep, sep, b'?'.into(), sep]) { + // Remove first 3 characters. It still starts with `/` so it is absolute on Unix. + path.splice(0..3, std::iter::empty()); + } + // If it starts with a drive letter (`X:/`), convert it to an absolute Unix path. + else if path.get(1..3) == Some(&[b':'.into(), sep]) { + // We add a `/` at the beginning, to store the absolute Windows + // path in something that looks like an absolute Unix path. + path.insert(0, sep); + } + } + + /// Adjust a Unix path to Windows conventions such that it un-does everything that + /// `windows_to_unix` did, and such that if the Unix input path was absolute, then the + /// Windows output path is absolute. + fn unix_to_windows(path: &mut Vec) + where + T: From + Copy + Eq, + { + let sep = T::from(b'\\'); + // Make sure all path separators are `\`. + for c in path.iter_mut() { + if *c == b'/'.into() { + *c = sep; + } + } + // If the path is `\X:\`, the leading separator was probably added by `windows_to_unix` + // and we should get rid of it again. + if path.get(2..4) == Some(&[b':'.into(), sep]) && path[0] == sep { + // The new path is still absolute on Windows. + path.remove(0); + } + // If this starts withs a `\` but not a `\\`, then this was absolute on Unix but is + // relative on Windows (relative to "the root of the current directory", e.g. the + // drive letter). + else if path.first() == Some(&sep) && path.get(1) != Some(&sep) { + // We add `\\?` so it starts with `\\?\` which is some magic path on Windows + // that *is* considered absolute. This way we store the absolute Unix path + // in something that looks like an absolute Windows path. + path.splice(0..0, [sep, sep, b'?'.into()]); + } + } + // Below we assume that everything non-Windows works like Unix, at least // when it comes to file system path conventions. #[cfg(windows)] @@ -268,102 +326,30 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { os_str } else { // Unix target, Windows host. - let (from, to) = match direction { - PathConversion::HostToTarget => ('\\', '/'), - PathConversion::TargetToHost => ('/', '\\'), - }; - let mut converted = os_str - .encode_wide() - .map(|wchar| if wchar == from as u16 { to as u16 } else { wchar }) - .collect::>(); - // We also have to ensure that absolute paths remain absolute. + let mut path: Vec = os_str.encode_wide().collect(); match direction { PathConversion::HostToTarget => { - // If this is an absolute Windows path that starts with a drive letter (`C:/...` - // after separator conversion), it would not be considered absolute by Unix - // target code. - if converted.get(1).copied() == Some(b':' as u16) - && converted.get(2).copied() == Some(b'/' as u16) - { - // We add a `/` at the beginning, to store the absolute Windows - // path in something that looks like an absolute Unix path. - converted.insert(0, b'/' as u16); - } + windows_to_unix(&mut path); } PathConversion::TargetToHost => { - // If the path is `\C:\`, the leading backslash was probably added by the above code - // and we should get rid of it again. - if converted.get(0).copied() == Some(b'\\' as u16) - && converted.get(2).copied() == Some(b':' as u16) - && converted.get(3).copied() == Some(b'\\' as u16) - { - converted.remove(0); - } - // If the path starts with `\\`, it is a magic Windows path. Conveniently, paths - // starting with `//` on Unix are also magic where the first component can have - // "application-specific" meaning, which is reflected e.g. by `path::absolute` - // leaving leading `//` alone (but normalizing leading `///` to `/`). So we - // don't have to do anything, the magic Windows path should work mostly fine as - // a magic Unix path. + unix_to_windows(&mut path); } } - Cow::Owned(OsString::from_wide(&converted)) + Cow::Owned(OsString::from_wide(&path)) }; #[cfg(unix)] return if target_os == "windows" { // Windows target, Unix host. - let (from, to) = match direction { - PathConversion::HostToTarget => (b'/', b'\\'), - PathConversion::TargetToHost => (b'\\', b'/'), - }; - let mut converted = os_str - .as_bytes() - .iter() - .map(|&wchar| if wchar == from { to } else { wchar }) - .collect::>(); - // We also have to ensure that absolute paths remain absolute. + let mut path: Vec = os_str.into_owned().into_encoded_bytes(); match direction { PathConversion::HostToTarget => { - // If the path is `/C:/`, the leading backslash was probably added by the below - // driver letter handling and we should get rid of it again. - if converted.get(0).copied() == Some(b'\\') - && converted.get(2).copied() == Some(b':') - && converted.get(3).copied() == Some(b'\\') - { - converted.remove(0); - } - // If this starts withs a `\` but not a `\\`, then for Windows this is a - // relative path (relative to "the root of the current directory", e.g. the - // drive letter). But the host path on Unix is absolute as it starts with `/`. - else if converted.get(0).copied() == Some(b'\\') - && converted.get(1).copied() != Some(b'\\') - { - // We add `\\?` so it starts with `\\?\` which is some magic path on Windows - // that *is* considered absolute. This way we store the absolute host path - // in something that looks like an absolute path to the (Windows) target. - converted.splice(0..0, b"\\\\?".iter().copied()); - } + unix_to_windows(&mut path); } PathConversion::TargetToHost => { - // If this starts with `//?/`, it was probably produced by the above code and we - // remove the `//?` that got added to get the Unix path back out. - if converted.get(0).copied() == Some(b'/') - && converted.get(1).copied() == Some(b'/') - && converted.get(2).copied() == Some(b'?') - && converted.get(3).copied() == Some(b'/') - { - // Remove first 3 characters - converted.splice(0..3, std::iter::empty()); - } - // If it starts with a drive letter, convert it to an absolute Unix path. - else if converted.get(1).copied() == Some(b':') - && converted.get(2).copied() == Some(b'/') - { - converted.insert(0, b'/'); - } + windows_to_unix(&mut path); } } - Cow::Owned(OsString::from_vec(converted)) + Cow::Owned(OsString::from_vec(path)) } else { // Unix-on-Unix, all is fine. os_str From fecd7fc937ad2d019f92d5b5c07d73ae206deaae Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 19 Apr 2024 10:59:27 +0200 Subject: [PATCH 25/33] make test not leak rustc crate hash --- src/tools/miri/tests/panic/mir-validation.rs | 1 + src/tools/miri/tests/panic/mir-validation.stderr | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/tools/miri/tests/panic/mir-validation.rs b/src/tools/miri/tests/panic/mir-validation.rs index 5e207c26096..fe618f6c819 100644 --- a/src/tools/miri/tests/panic/mir-validation.rs +++ b/src/tools/miri/tests/panic/mir-validation.rs @@ -3,6 +3,7 @@ //@normalize-stderr-test: "\n +at [^\n]+" -> "" //@normalize-stderr-test: "\n +\[\.\.\. omitted [0-9]+ frames? \.\.\.\]" -> "" //@normalize-stderr-test: "\n[ =]*note:.*" -> "" +//@normalize-stderr-test: "DefId\([^()]*\)" -> "DefId" #![feature(custom_mir, core_intrinsics)] use core::intrinsics::mir::*; diff --git a/src/tools/miri/tests/panic/mir-validation.stderr b/src/tools/miri/tests/panic/mir-validation.stderr index 243fed020e7..d158c996dc3 100644 --- a/src/tools/miri/tests/panic/mir-validation.stderr +++ b/src/tools/miri/tests/panic/mir-validation.stderr @@ -1,5 +1,5 @@ thread 'rustc' panicked at compiler/rustc_const_eval/src/transform/validate.rs:LL:CC: -broken MIR in Item(DefId(0:4 ~ mir_validation[fad2]::main)) (after phase change to runtime-optimized) at bb0[1]: +broken MIR in Item(DefId) (after phase change to runtime-optimized) at bb0[1]: (*(_2.0: *mut i32)), has deref at the wrong place stack backtrace: From 29e41fbc30cf0448b858c6e7caace872f5767b05 Mon Sep 17 00:00:00 2001 From: The Miri Cronjob Bot Date: Sat, 20 Apr 2024 05:05:23 +0000 Subject: [PATCH 26/33] Preparing for merge from rustc --- src/tools/miri/rust-version | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tools/miri/rust-version b/src/tools/miri/rust-version index f0fa47208a1..a60acf44a40 100644 --- a/src/tools/miri/rust-version +++ b/src/tools/miri/rust-version @@ -1 +1 @@ -fa0068b5412baecc932772dda72c0621bfa7ab00 +c8d19a92aa9022eb690899cf6d54fd23cb6877e5 From 8e1f18dce89b82a090195ddc8e5a6e8e98fc33fb Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 20 Apr 2024 08:26:42 +0200 Subject: [PATCH 27/33] fix clippy warning --- src/tools/miri/src/borrow_tracker/tree_borrows/exhaustive.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/tools/miri/src/borrow_tracker/tree_borrows/exhaustive.rs b/src/tools/miri/src/borrow_tracker/tree_borrows/exhaustive.rs index daf3590358f..d50a22a9104 100644 --- a/src/tools/miri/src/borrow_tracker/tree_borrows/exhaustive.rs +++ b/src/tools/miri/src/borrow_tracker/tree_borrows/exhaustive.rs @@ -2,7 +2,6 @@ //! (These are used in Tree Borrows `#[test]`s for thorough verification //! of the behavior of the state machine of permissions, //! but the contents of this file are extremely generic) -#![cfg(test)] pub trait Exhaustive: Sized { fn exhaustive() -> Box>; From 7952316aff4b72cc2674decc9247c55548267a5a Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 20 Apr 2024 08:26:48 +0200 Subject: [PATCH 28/33] re-bless tests --- src/tools/miri/tests/panic/alloc_error_handler_hook.stderr | 1 + src/tools/miri/tests/panic/alloc_error_handler_panic.stderr | 1 + 2 files changed, 2 insertions(+) diff --git a/src/tools/miri/tests/panic/alloc_error_handler_hook.stderr b/src/tools/miri/tests/panic/alloc_error_handler_hook.stderr index 01c286fe3d2..5b309ed09bb 100644 --- a/src/tools/miri/tests/panic/alloc_error_handler_hook.stderr +++ b/src/tools/miri/tests/panic/alloc_error_handler_hook.stderr @@ -1,4 +1,5 @@ thread 'main' panicked at $DIR/alloc_error_handler_hook.rs:LL:CC: alloc error hook called note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace +note: in Miri, you may have to set `-Zmiri-env-forward=RUST_BACKTRACE` for the environment variable to have an effect yes we are unwinding! diff --git a/src/tools/miri/tests/panic/alloc_error_handler_panic.stderr b/src/tools/miri/tests/panic/alloc_error_handler_panic.stderr index 202325468bf..3d5457799f6 100644 --- a/src/tools/miri/tests/panic/alloc_error_handler_panic.stderr +++ b/src/tools/miri/tests/panic/alloc_error_handler_panic.stderr @@ -1,4 +1,5 @@ thread 'main' panicked at RUSTLIB/std/src/alloc.rs:LL:CC: memory allocation of 4 bytes failed note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace +note: in Miri, you may have to set `-Zmiri-env-forward=RUST_BACKTRACE` for the environment variable to have an effect yes we are unwinding! From 7dcfb54dc625f4937d583d96de3dc41af73d2f17 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 20 Apr 2024 09:01:53 +0200 Subject: [PATCH 29/33] data_race: make the release/acquire API more clear --- src/tools/miri/src/alloc_addresses/mod.rs | 16 ++--- src/tools/miri/src/concurrency/data_race.rs | 71 +++++++++------------ src/tools/miri/src/concurrency/init_once.rs | 12 ++-- src/tools/miri/src/concurrency/sync.rs | 62 +++++++----------- 4 files changed, 67 insertions(+), 94 deletions(-) diff --git a/src/tools/miri/src/alloc_addresses/mod.rs b/src/tools/miri/src/alloc_addresses/mod.rs index 612649c90c6..d59a2cee94e 100644 --- a/src/tools/miri/src/alloc_addresses/mod.rs +++ b/src/tools/miri/src/alloc_addresses/mod.rs @@ -13,7 +13,7 @@ use rustc_data_structures::fx::{FxHashMap, FxHashSet}; use rustc_span::Span; use rustc_target::abi::{Align, HasDataLayout, Size}; -use crate::*; +use crate::{concurrency::VClock, *}; use self::reuse_pool::ReusePool; @@ -172,7 +172,7 @@ trait EvalContextExtPriv<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { ecx.get_active_thread(), ) { if let Some(data_race) = &ecx.machine.data_race { - data_race.validate_lock_acquire(&clock, ecx.get_active_thread()); + data_race.acquire_clock(&clock, ecx.get_active_thread()); } reuse_addr } else { @@ -369,15 +369,13 @@ impl<'mir, 'tcx> MiriMachine<'mir, 'tcx> { // Also remember this address for future reuse. let thread = self.threads.get_active_thread_id(); global_state.reuse.add_addr(rng, addr, size, align, kind, thread, || { - let mut clock = concurrency::VClock::default(); if let Some(data_race) = &self.data_race { - data_race.validate_lock_release( - &mut clock, - thread, - self.threads.active_thread_ref().current_span(), - ); + data_race + .release_clock(thread, self.threads.active_thread_ref().current_span()) + .clone() + } else { + VClock::default() } - clock }) } } diff --git a/src/tools/miri/src/concurrency/data_race.rs b/src/tools/miri/src/concurrency/data_race.rs index 95049b91cba..6c34716b592 100644 --- a/src/tools/miri/src/concurrency/data_race.rs +++ b/src/tools/miri/src/concurrency/data_race.rs @@ -1701,49 +1701,34 @@ impl GlobalState { format!("thread `{thread_name}`") } - /// Acquire a lock, express that the previous call of - /// `validate_lock_release` must happen before this. + /// Acquire the given clock into the given thread, establishing synchronization with + /// the moment when that clock snapshot was taken via `release_clock`. /// As this is an acquire operation, the thread timestamp is not /// incremented. - pub fn validate_lock_acquire(&self, lock: &VClock, thread: ThreadId) { - let (_, mut clocks) = self.load_thread_state_mut(thread); + pub fn acquire_clock(&self, lock: &VClock, thread: ThreadId) { + let (_, mut clocks) = self.thread_state_mut(thread); clocks.clock.join(lock); } - /// Release a lock handle, express that this happens-before - /// any subsequent calls to `validate_lock_acquire`. - /// For normal locks this should be equivalent to `validate_lock_release_shared` - /// since an acquire operation should have occurred before, however - /// for futex & condvar operations this is not the case and this - /// operation must be used. - pub fn validate_lock_release(&self, lock: &mut VClock, thread: ThreadId, current_span: Span) { - let (index, mut clocks) = self.load_thread_state_mut(thread); - lock.clone_from(&clocks.clock); - clocks.increment_clock(index, current_span); - } - - /// Release a lock handle, express that this happens-before - /// any subsequent calls to `validate_lock_acquire` as well - /// as any previous calls to this function after any - /// `validate_lock_release` calls. - /// For normal locks this should be equivalent to `validate_lock_release`. - /// This function only exists for joining over the set of concurrent readers - /// in a read-write lock and should not be used for anything else. - pub fn validate_lock_release_shared( - &self, - lock: &mut VClock, - thread: ThreadId, - current_span: Span, - ) { - let (index, mut clocks) = self.load_thread_state_mut(thread); - lock.join(&clocks.clock); + /// Returns the `release` clock of the given thread. + /// Other threads can acquire this clock in the future to establish synchronization + /// with this program point. + pub fn release_clock(&self, thread: ThreadId, current_span: Span) -> Ref<'_, VClock> { + // We increment the clock each time this happens, to ensure no two releases + // can be confused with each other. + let (index, mut clocks) = self.thread_state_mut(thread); clocks.increment_clock(index, current_span); + drop(clocks); + // To return a read-only view, we need to release the RefCell + // and borrow it again. + let (_index, clocks) = self.thread_state(thread); + Ref::map(clocks, |c| &c.clock) } /// Load the vector index used by the given thread as well as the set of vector clocks /// used by the thread. #[inline] - fn load_thread_state_mut(&self, thread: ThreadId) -> (VectorIdx, RefMut<'_, ThreadClockSet>) { + fn thread_state_mut(&self, thread: ThreadId) -> (VectorIdx, RefMut<'_, ThreadClockSet>) { let index = self.thread_info.borrow()[thread] .vector_index .expect("Loading thread state for thread with no assigned vector"); @@ -1752,6 +1737,18 @@ impl GlobalState { (index, clocks) } + /// Load the vector index used by the given thread as well as the set of vector clocks + /// used by the thread. + #[inline] + fn thread_state(&self, thread: ThreadId) -> (VectorIdx, Ref<'_, ThreadClockSet>) { + let index = self.thread_info.borrow()[thread] + .vector_index + .expect("Loading thread state for thread with no assigned vector"); + let ref_vector = self.vector_clocks.borrow(); + let clocks = Ref::map(ref_vector, |vec| &vec[index]); + (index, clocks) + } + /// Load the current vector clock in use and the current set of thread clocks /// in use for the vector. #[inline] @@ -1759,10 +1756,7 @@ impl GlobalState { &self, thread_mgr: &ThreadManager<'_, '_>, ) -> (VectorIdx, Ref<'_, ThreadClockSet>) { - let index = self.current_index(thread_mgr); - let ref_vector = self.vector_clocks.borrow(); - let clocks = Ref::map(ref_vector, |vec| &vec[index]); - (index, clocks) + self.thread_state(thread_mgr.get_active_thread_id()) } /// Load the current vector clock in use and the current set of thread clocks @@ -1772,10 +1766,7 @@ impl GlobalState { &self, thread_mgr: &ThreadManager<'_, '_>, ) -> (VectorIdx, RefMut<'_, ThreadClockSet>) { - let index = self.current_index(thread_mgr); - let ref_vector = self.vector_clocks.borrow_mut(); - let clocks = RefMut::map(ref_vector, |vec| &mut vec[index]); - (index, clocks) + self.thread_state_mut(thread_mgr.get_active_thread_id()) } /// Return the current thread, should be the same diff --git a/src/tools/miri/src/concurrency/init_once.rs b/src/tools/miri/src/concurrency/init_once.rs index 9dbea08f3ef..a01b59c9165 100644 --- a/src/tools/miri/src/concurrency/init_once.rs +++ b/src/tools/miri/src/concurrency/init_once.rs @@ -41,7 +41,7 @@ pub enum InitOnceStatus { pub(super) struct InitOnce<'mir, 'tcx> { status: InitOnceStatus, waiters: VecDeque>, - data_race: VClock, + clock: VClock, } impl<'mir, 'tcx> VisitProvenance for InitOnce<'mir, 'tcx> { @@ -61,10 +61,8 @@ trait EvalContextExtPriv<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { let current_thread = this.get_active_thread(); if let Some(data_race) = &this.machine.data_race { - data_race.validate_lock_acquire( - &this.machine.threads.sync.init_onces[id].data_race, - current_thread, - ); + data_race + .acquire_clock(&this.machine.threads.sync.init_onces[id].clock, current_thread); } } @@ -176,7 +174,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { // Each complete happens-before the end of the wait if let Some(data_race) = &this.machine.data_race { - data_race.validate_lock_release(&mut init_once.data_race, current_thread, current_span); + init_once.clock.clone_from(&data_race.release_clock(current_thread, current_span)); } // Wake up everyone. @@ -202,7 +200,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { // Each complete happens-before the end of the wait if let Some(data_race) = &this.machine.data_race { - data_race.validate_lock_release(&mut init_once.data_race, current_thread, current_span); + init_once.clock.clone_from(&data_race.release_clock(current_thread, current_span)); } // Wake up one waiting thread, so they can go ahead and try to init this. diff --git a/src/tools/miri/src/concurrency/sync.rs b/src/tools/miri/src/concurrency/sync.rs index 0a428715690..d3cef8bf5f3 100644 --- a/src/tools/miri/src/concurrency/sync.rs +++ b/src/tools/miri/src/concurrency/sync.rs @@ -69,12 +69,8 @@ struct Mutex { lock_count: usize, /// The queue of threads waiting for this mutex. queue: VecDeque, - /// Data race handle. This tracks the happens-before - /// relationship between each mutex access. It is - /// released to during unlock and acquired from during - /// locking, and therefore stores the clock of the last - /// thread to release this mutex. - data_race: VClock, + /// Mutex clock. This tracks the moment of the last unlock. + clock: VClock, } declare_id!(RwLockId); @@ -91,7 +87,7 @@ struct RwLock { writer_queue: VecDeque, /// The queue of reader threads waiting for this lock. reader_queue: VecDeque, - /// Data race handle for writers. Tracks the happens-before + /// Data race clock for writers. Tracks the happens-before /// ordering between each write access to a rwlock and is updated /// after a sequence of concurrent readers to track the happens- /// before ordering between the set of previous readers and @@ -99,8 +95,8 @@ struct RwLock { /// Contains the clock of the last thread to release a writer /// lock or the joined clock of the set of last threads to release /// shared reader locks. - data_race: VClock, - /// Data race handle for readers. This is temporary storage + clock_unlocked: VClock, + /// Data race clock for readers. This is temporary storage /// for the combined happens-before ordering for between all /// concurrent readers and the next writer, and the value /// is stored to the main data_race variable once all @@ -110,7 +106,7 @@ struct RwLock { /// add happens-before orderings between shared reader /// locks. /// This is only relevant when there is an active reader. - data_race_reader: VClock, + clock_current_readers: VClock, } declare_id!(CondvarId); @@ -132,8 +128,8 @@ struct Condvar { /// between a cond-var signal and a cond-var /// wait during a non-spurious signal event. /// Contains the clock of the last thread to - /// perform a futex-signal. - data_race: VClock, + /// perform a condvar-signal. + clock: VClock, } /// The futex state. @@ -145,7 +141,7 @@ struct Futex { /// during a non-spurious wake event. /// Contains the clock of the last thread to /// perform a futex-wake. - data_race: VClock, + clock: VClock, } /// A thread waiting on a futex. @@ -346,7 +342,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { } mutex.lock_count = mutex.lock_count.checked_add(1).unwrap(); if let Some(data_race) = &this.machine.data_race { - data_race.validate_lock_acquire(&mutex.data_race, thread); + data_race.acquire_clock(&mutex.clock, thread); } } @@ -373,11 +369,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { // The mutex is completely unlocked. Try transferring ownership // to another thread. if let Some(data_race) = &this.machine.data_race { - data_race.validate_lock_release( - &mut mutex.data_race, - current_owner, - current_span, - ); + mutex.clock.clone_from(&data_race.release_clock(current_owner, current_span)); } this.mutex_dequeue_and_lock(id); } @@ -448,7 +440,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { let count = rwlock.readers.entry(reader).or_insert(0); *count = count.checked_add(1).expect("the reader counter overflowed"); if let Some(data_race) = &this.machine.data_race { - data_race.validate_lock_acquire(&rwlock.data_race, reader); + data_race.acquire_clock(&rwlock.clock_unlocked, reader); } } @@ -474,20 +466,16 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { } if let Some(data_race) = &this.machine.data_race { // Add this to the shared-release clock of all concurrent readers. - data_race.validate_lock_release_shared( - &mut rwlock.data_race_reader, - reader, - current_span, - ); + rwlock.clock_current_readers.join(&data_race.release_clock(reader, current_span)); } // The thread was a reader. If the lock is not held any more, give it to a writer. if this.rwlock_is_locked(id).not() { // All the readers are finished, so set the writer data-race handle to the value - // of the union of all reader data race handles, since the set of readers - // happen-before the writers + // of the union of all reader data race handles, since the set of readers + // happen-before the writers let rwlock = &mut this.machine.threads.sync.rwlocks[id]; - rwlock.data_race.clone_from(&rwlock.data_race_reader); + rwlock.clock_unlocked.clone_from(&rwlock.clock_current_readers); this.rwlock_dequeue_and_lock_writer(id); } true @@ -511,7 +499,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { let rwlock = &mut this.machine.threads.sync.rwlocks[id]; rwlock.writer = Some(writer); if let Some(data_race) = &this.machine.data_race { - data_race.validate_lock_acquire(&rwlock.data_race, writer); + data_race.acquire_clock(&rwlock.clock_unlocked, writer); } } @@ -530,11 +518,9 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { trace!("rwlock_writer_unlock: {:?} unlocked by {:?}", id, expected_writer); // Release memory to next lock holder. if let Some(data_race) = &this.machine.data_race { - data_race.validate_lock_release( - &mut rwlock.data_race, - current_writer, - current_span, - ); + rwlock + .clock_unlocked + .clone_from(&*data_race.release_clock(current_writer, current_span)); } // The thread was a writer. // @@ -611,11 +597,11 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { // Each condvar signal happens-before the end of the condvar wake if let Some(data_race) = data_race { - data_race.validate_lock_release(&mut condvar.data_race, current_thread, current_span); + condvar.clock.clone_from(&*data_race.release_clock(current_thread, current_span)); } condvar.waiters.pop_front().map(|waiter| { if let Some(data_race) = data_race { - data_race.validate_lock_acquire(&condvar.data_race, waiter.thread); + data_race.acquire_clock(&condvar.clock, waiter.thread); } (waiter.thread, waiter.lock) }) @@ -645,14 +631,14 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { // Each futex-wake happens-before the end of the futex wait if let Some(data_race) = data_race { - data_race.validate_lock_release(&mut futex.data_race, current_thread, current_span); + futex.clock.clone_from(&*data_race.release_clock(current_thread, current_span)); } // Wake up the first thread in the queue that matches any of the bits in the bitset. futex.waiters.iter().position(|w| w.bitset & bitset != 0).map(|i| { let waiter = futex.waiters.remove(i).unwrap(); if let Some(data_race) = data_race { - data_race.validate_lock_acquire(&futex.data_race, waiter.thread); + data_race.acquire_clock(&futex.clock, waiter.thread); } waiter.thread }) From cb1b4a6977cfe1149005a3131a08d0bbce456833 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 20 Apr 2024 09:08:09 +0200 Subject: [PATCH 30/33] reuse_pool: only do acquire_clock if we reused from a different thread --- src/tools/miri/src/alloc_addresses/mod.rs | 4 +++- src/tools/miri/src/alloc_addresses/reuse_pool.rs | 6 ++++-- src/tools/miri/src/concurrency/vector_clock.rs | 2 +- 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/tools/miri/src/alloc_addresses/mod.rs b/src/tools/miri/src/alloc_addresses/mod.rs index d59a2cee94e..2bbb34c9a4b 100644 --- a/src/tools/miri/src/alloc_addresses/mod.rs +++ b/src/tools/miri/src/alloc_addresses/mod.rs @@ -171,7 +171,9 @@ trait EvalContextExtPriv<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { memory_kind, ecx.get_active_thread(), ) { - if let Some(data_race) = &ecx.machine.data_race { + if let Some(clock) = clock + && let Some(data_race) = &ecx.machine.data_race + { data_race.acquire_clock(&clock, ecx.get_active_thread()); } reuse_addr diff --git a/src/tools/miri/src/alloc_addresses/reuse_pool.rs b/src/tools/miri/src/alloc_addresses/reuse_pool.rs index fd0b24cc29b..77fc9f53f9e 100644 --- a/src/tools/miri/src/alloc_addresses/reuse_pool.rs +++ b/src/tools/miri/src/alloc_addresses/reuse_pool.rs @@ -78,6 +78,7 @@ impl ReusePool { subpool.insert(pos, (addr, size, thread, clock)); } + /// Returns the address to use and optionally a clock we have to synchronize with. pub fn take_addr( &mut self, rng: &mut impl Rng, @@ -85,7 +86,7 @@ impl ReusePool { align: Align, kind: MemoryKind, thread: ThreadId, - ) -> Option<(u64, VClock)> { + ) -> Option<(u64, Option)> { // Determine whether we'll even attempt a reuse. As above, we don't do reuse for stack addresses. if kind == MemoryKind::Stack || !rng.gen_bool(self.address_reuse_rate) { return None; @@ -122,6 +123,7 @@ impl ReusePool { let (chosen_addr, chosen_size, chosen_thread, clock) = subpool.remove(idx); debug_assert!(chosen_size >= size && chosen_addr % align.bytes() == 0); debug_assert!(cross_thread_reuse || chosen_thread == thread); - Some((chosen_addr, clock)) + // No synchronization needed if we reused from the current thread. + Some((chosen_addr, if chosen_thread == thread { None } else { Some(clock) })) } } diff --git a/src/tools/miri/src/concurrency/vector_clock.rs b/src/tools/miri/src/concurrency/vector_clock.rs index fe719943dcb..0178b01e0b8 100644 --- a/src/tools/miri/src/concurrency/vector_clock.rs +++ b/src/tools/miri/src/concurrency/vector_clock.rs @@ -152,7 +152,7 @@ impl VClock { } /// Get a mutable slice to the internal vector with minimum `min_len` - /// elements, to preserve invariants this vector must modify + /// elements. To preserve invariants, the caller must modify /// the `min_len`-1 nth element to a non-zero value #[inline] fn get_mut_with_min_len(&mut self, min_len: usize) -> &mut [VTimestamp] { From 8cab0d561716e4b1991683c608a0beda7ad6bd81 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 20 Apr 2024 09:24:22 +0200 Subject: [PATCH 31/33] restrict VClock API surface --- src/tools/miri/src/concurrency/data_race.rs | 6 +- .../miri/src/concurrency/vector_clock.rs | 64 ++++++++++++------- 2 files changed, 44 insertions(+), 26 deletions(-) diff --git a/src/tools/miri/src/concurrency/data_race.rs b/src/tools/miri/src/concurrency/data_race.rs index 6c34716b592..2281609a049 100644 --- a/src/tools/miri/src/concurrency/data_race.rs +++ b/src/tools/miri/src/concurrency/data_race.rs @@ -547,9 +547,9 @@ impl MemoryCellClocks { ) -> Result<(), DataRace> { trace!("Unsynchronized read with vectors: {:#?} :: {:#?}", self, thread_clocks); if !current_span.is_dummy() { - thread_clocks.clock[index].span = current_span; + thread_clocks.clock.index_mut(index).span = current_span; } - thread_clocks.clock[index].set_read_type(read_type); + thread_clocks.clock.index_mut(index).set_read_type(read_type); if self.write_was_before(&thread_clocks.clock) { let race_free = if let Some(atomic) = self.atomic() { // We must be ordered-after all atomic accesses, reads and writes. @@ -577,7 +577,7 @@ impl MemoryCellClocks { ) -> Result<(), DataRace> { trace!("Unsynchronized write with vectors: {:#?} :: {:#?}", self, thread_clocks); if !current_span.is_dummy() { - thread_clocks.clock[index].span = current_span; + thread_clocks.clock.index_mut(index).span = current_span; } if self.write_was_before(&thread_clocks.clock) && self.read <= thread_clocks.clock { let race_free = if let Some(atomic) = self.atomic() { diff --git a/src/tools/miri/src/concurrency/vector_clock.rs b/src/tools/miri/src/concurrency/vector_clock.rs index 0178b01e0b8..2cd3d031b1e 100644 --- a/src/tools/miri/src/concurrency/vector_clock.rs +++ b/src/tools/miri/src/concurrency/vector_clock.rs @@ -4,7 +4,7 @@ use smallvec::SmallVec; use std::{ cmp::Ordering, fmt::Debug, - ops::{Index, IndexMut, Shr}, + ops::{Index, Shr}, }; use super::data_race::NaReadType; @@ -92,7 +92,7 @@ impl VTimestamp { } #[inline] - pub fn set_read_type(&mut self, read_type: NaReadType) { + pub(super) fn set_read_type(&mut self, read_type: NaReadType) { self.time_and_read_type = Self::encode_time_and_read_type(self.time(), read_type); } @@ -138,7 +138,7 @@ pub struct VClock(SmallVec<[VTimestamp; SMALL_VECTOR]>); impl VClock { /// Create a new vector-clock containing all zeros except /// for a value at the given index - pub fn new_with_index(index: VectorIdx, timestamp: VTimestamp) -> VClock { + pub(super) fn new_with_index(index: VectorIdx, timestamp: VTimestamp) -> VClock { let len = index.index() + 1; let mut vec = smallvec::smallvec![VTimestamp::ZERO; len]; vec[index.index()] = timestamp; @@ -147,10 +147,16 @@ impl VClock { /// Load the internal timestamp slice in the vector clock #[inline] - pub fn as_slice(&self) -> &[VTimestamp] { + pub(super) fn as_slice(&self) -> &[VTimestamp] { + debug_assert!(!self.0.last().is_some_and(|t| t.time() == 0)); self.0.as_slice() } + #[inline] + pub(super) fn index_mut(&mut self, index: VectorIdx) -> &mut VTimestamp { + self.0.as_mut_slice().get_mut(index.to_u32() as usize).unwrap() + } + /// Get a mutable slice to the internal vector with minimum `min_len` /// elements. To preserve invariants, the caller must modify /// the `min_len`-1 nth element to a non-zero value @@ -166,7 +172,7 @@ impl VClock { /// Increment the vector clock at a known index /// this will panic if the vector index overflows #[inline] - pub fn increment_index(&mut self, idx: VectorIdx, current_span: Span) { + pub(super) fn increment_index(&mut self, idx: VectorIdx, current_span: Span) { let idx = idx.index(); let mut_slice = self.get_mut_with_min_len(idx + 1); let idx_ref = &mut mut_slice[idx]; @@ -190,28 +196,36 @@ impl VClock { } } - /// Set the element at the current index of the vector - pub fn set_at_index(&mut self, other: &Self, idx: VectorIdx) { + /// Set the element at the current index of the vector. May only increase elements. + pub(super) fn set_at_index(&mut self, other: &Self, idx: VectorIdx) { + let new_timestamp = other[idx]; + // Setting to 0 is different, since the last element cannot be 0. + if new_timestamp.time() == 0 { + if idx.index() >= self.0.len() { + // This index does not even exist yet in our clock. Just do nothing. + return; + } + // This changes an existing element. Since it can only increase, that + // can never make the last element 0. + } + let mut_slice = self.get_mut_with_min_len(idx.index() + 1); + let mut_timestamp = &mut mut_slice[idx.index()]; - let prev_span = mut_slice[idx.index()].span; + let prev_span = mut_timestamp.span; - mut_slice[idx.index()] = other[idx]; + assert!(*mut_timestamp <= new_timestamp, "set_at_index: may only increase the timestamp"); + *mut_timestamp = new_timestamp; - let span = &mut mut_slice[idx.index()].span; + let span = &mut mut_timestamp.span; *span = span.substitute_dummy(prev_span); } /// Set the vector to the all-zero vector #[inline] - pub fn set_zero_vector(&mut self) { + pub(super) fn set_zero_vector(&mut self) { self.0.clear(); } - - /// Return if this vector is the all-zero vector - pub fn is_zero_vector(&self) -> bool { - self.0.is_empty() - } } impl Clone for VClock { @@ -407,13 +421,6 @@ impl Index for VClock { } } -impl IndexMut for VClock { - #[inline] - fn index_mut(&mut self, index: VectorIdx) -> &mut VTimestamp { - self.0.as_mut_slice().get_mut(index.to_u32() as usize).unwrap() - } -} - /// Test vector clock ordering operations /// data-race detection is tested in the external /// test suite @@ -553,4 +560,15 @@ mod tests { "Invalid alt (>=):\n l: {l:?}\n r: {r:?}" ); } + + #[test] + fn set_index_to_0() { + let mut clock1 = from_slice(&[0, 1, 2, 3]); + let clock2 = from_slice(&[0, 2, 3, 4, 0, 5]); + // Naively, this would extend clock1 with a new index and set it to 0, making + // the last index 0. Make sure that does not happen. + clock1.set_at_index(&clock2, VectorIdx(4)); + // This must not have made the last element 0. + assert!(clock1.0.last().unwrap().time() != 0); + } } From 61a7d3d52311aa272a9ae81e96fd7438d8573eb9 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 20 Apr 2024 11:42:10 +0200 Subject: [PATCH 32/33] ensure the ICE-to-file logic does not affect our test --- src/tools/miri/tests/panic/mir-validation.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/tools/miri/tests/panic/mir-validation.rs b/src/tools/miri/tests/panic/mir-validation.rs index fe618f6c819..5c1519f2eb9 100644 --- a/src/tools/miri/tests/panic/mir-validation.rs +++ b/src/tools/miri/tests/panic/mir-validation.rs @@ -1,4 +1,5 @@ //! Ensure that the MIR validator runs on Miri's input. +//@rustc-env:RUSTC_ICE=0 //@normalize-stderr-test: "\n +[0-9]+:[^\n]+" -> "" //@normalize-stderr-test: "\n +at [^\n]+" -> "" //@normalize-stderr-test: "\n +\[\.\.\. omitted [0-9]+ frames? \.\.\.\]" -> "" From ae37b6ec585628764c9af4d3c67137c981bc4ea7 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 21 Apr 2024 10:26:28 +0200 Subject: [PATCH 33/33] the mir-validation ICE test behaves strangely on Windows hosts let's just disable it there, this code is not platform-dependent anyway --- src/tools/miri/src/diagnostics.rs | 4 ++-- src/tools/miri/tests/panic/mir-validation.rs | 9 ++++++--- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/src/tools/miri/src/diagnostics.rs b/src/tools/miri/src/diagnostics.rs index d44d04e9bf8..0c0ac4c6036 100644 --- a/src/tools/miri/src/diagnostics.rs +++ b/src/tools/miri/src/diagnostics.rs @@ -291,7 +291,7 @@ pub fn report_error<'tcx, 'mir>( ValidationErrorKind::PointerAsInt { .. } | ValidationErrorKind::PartialPointer ) => { - ecx.handle_ice(); // print interpreter backtrace + ecx.handle_ice(); // print interpreter backtrace (this is outside the eval `catch_unwind`) bug!( "This validation error should be impossible in Miri: {}", format_interp_error(ecx.tcx.dcx(), e) @@ -308,7 +308,7 @@ pub fn report_error<'tcx, 'mir>( InvalidProgramInfo::AlreadyReported(_) | InvalidProgramInfo::Layout(..), ) => "post-monomorphization error", _ => { - ecx.handle_ice(); // print interpreter backtrace + ecx.handle_ice(); // print interpreter backtrace (this is outside the eval `catch_unwind`) bug!( "This error should be impossible in Miri: {}", format_interp_error(ecx.tcx.dcx(), e) diff --git a/src/tools/miri/tests/panic/mir-validation.rs b/src/tools/miri/tests/panic/mir-validation.rs index 5c1519f2eb9..f1d0ccc7d03 100644 --- a/src/tools/miri/tests/panic/mir-validation.rs +++ b/src/tools/miri/tests/panic/mir-validation.rs @@ -1,10 +1,13 @@ //! Ensure that the MIR validator runs on Miri's input. //@rustc-env:RUSTC_ICE=0 -//@normalize-stderr-test: "\n +[0-9]+:[^\n]+" -> "" -//@normalize-stderr-test: "\n +at [^\n]+" -> "" -//@normalize-stderr-test: "\n +\[\.\.\. omitted [0-9]+ frames? \.\.\.\]" -> "" +//@normalize-stderr-test: "\n +[0-9]+:.+" -> "" +//@normalize-stderr-test: "\n +at .+" -> "" +//@normalize-stderr-test: "\n +\[\.\.\. omitted [0-9]+ frames? \.\.\.\].*" -> "" //@normalize-stderr-test: "\n[ =]*note:.*" -> "" //@normalize-stderr-test: "DefId\([^()]*\)" -> "DefId" +// Somehow on rustc Windows CI, the "Miri caused an ICE" message is not shown +// and we don't even get a regular panic; rustc aborts with a different exit code instead. +//@ignore-host-windows #![feature(custom_mir, core_intrinsics)] use core::intrinsics::mir::*;