From a8889052261e23444cfde46b7aba16115625a7d6 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 31 Aug 2024 17:03:03 +0200 Subject: [PATCH] miri: treat non-memory local variables properly for data race detection --- .../rustc_const_eval/src/interpret/machine.rs | 21 +++- .../rustc_const_eval/src/interpret/memory.rs | 4 + .../rustc_const_eval/src/interpret/operand.rs | 1 + .../rustc_const_eval/src/interpret/place.rs | 31 ++++-- .../rustc_const_eval/src/interpret/stack.rs | 5 +- src/tools/miri/src/concurrency/data_race.rs | 98 +++++++++++++++++++ src/tools/miri/src/concurrency/thread.rs | 4 +- .../miri/src/concurrency/vector_clock.rs | 6 ++ src/tools/miri/src/machine.rs | 55 ++++++++++- .../data_race/local_variable_alloc_race.rs | 57 +++++++++++ .../local_variable_alloc_race.stderr | 20 ++++ .../data_race/local_variable_read_race.rs | 38 +++++++ .../data_race/local_variable_read_race.stderr | 20 ++++ .../data_race/local_variable_write_race.rs | 37 +++++++ .../local_variable_write_race.stderr | 20 ++++ .../miri/tests/pass/concurrency/data_race.rs | 34 ++++++- 16 files changed, 433 insertions(+), 18 deletions(-) create mode 100644 src/tools/miri/tests/fail/data_race/local_variable_alloc_race.rs create mode 100644 src/tools/miri/tests/fail/data_race/local_variable_alloc_race.stderr create mode 100644 src/tools/miri/tests/fail/data_race/local_variable_read_race.rs create mode 100644 src/tools/miri/tests/fail/data_race/local_variable_read_race.stderr create mode 100644 src/tools/miri/tests/fail/data_race/local_variable_write_race.rs create mode 100644 src/tools/miri/tests/fail/data_race/local_variable_write_race.stderr diff --git a/compiler/rustc_const_eval/src/interpret/machine.rs b/compiler/rustc_const_eval/src/interpret/machine.rs index 8cab3c34eed..67c99934059 100644 --- a/compiler/rustc_const_eval/src/interpret/machine.rs +++ b/compiler/rustc_const_eval/src/interpret/machine.rs @@ -540,10 +540,29 @@ pub trait Machine<'tcx>: Sized { Ok(ReturnAction::Normal) } + /// Called immediately after an "immediate" local variable is read + /// (i.e., this is called for reads that do not end up accessing addressable memory). + #[inline(always)] + fn after_local_read(_ecx: &InterpCx<'tcx, Self>, _local: mir::Local) -> InterpResult<'tcx> { + Ok(()) + } + + /// Called immediately after an "immediate" local variable is assigned a new value + /// (i.e., this is called for writes that do not end up in memory). + /// `storage_live` indicates whether this is the initial write upon `StorageLive`. + #[inline(always)] + fn after_local_write( + _ecx: &mut InterpCx<'tcx, Self>, + _local: mir::Local, + _storage_live: bool, + ) -> InterpResult<'tcx> { + Ok(()) + } + /// Called immediately after actual memory was allocated for a local /// but before the local's stack frame is updated to point to that memory. #[inline(always)] - fn after_local_allocated( + fn after_local_moved_to_memory( _ecx: &mut InterpCx<'tcx, Self>, _local: mir::Local, _mplace: &MPlaceTy<'tcx, Self::Provenance>, diff --git a/compiler/rustc_const_eval/src/interpret/memory.rs b/compiler/rustc_const_eval/src/interpret/memory.rs index d87588496c0..a65637f4978 100644 --- a/compiler/rustc_const_eval/src/interpret/memory.rs +++ b/compiler/rustc_const_eval/src/interpret/memory.rs @@ -1030,6 +1030,10 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> { ); res } + + pub(super) fn validation_in_progress(&self) -> bool { + self.memory.validation_in_progress + } } #[doc(hidden)] diff --git a/compiler/rustc_const_eval/src/interpret/operand.rs b/compiler/rustc_const_eval/src/interpret/operand.rs index b906e3422db..bdbacfd20c2 100644 --- a/compiler/rustc_const_eval/src/interpret/operand.rs +++ b/compiler/rustc_const_eval/src/interpret/operand.rs @@ -719,6 +719,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> { if matches!(op, Operand::Immediate(_)) { assert!(!layout.is_unsized()); } + M::after_local_read(self, local)?; Ok(OpTy { op, layout }) } diff --git a/compiler/rustc_const_eval/src/interpret/place.rs b/compiler/rustc_const_eval/src/interpret/place.rs index 3b14142da02..ee8e5419495 100644 --- a/compiler/rustc_const_eval/src/interpret/place.rs +++ b/compiler/rustc_const_eval/src/interpret/place.rs @@ -504,15 +504,13 @@ where &self, local: mir::Local, ) -> InterpResult<'tcx, PlaceTy<'tcx, M::Provenance>> { - // Other parts of the system rely on `Place::Local` never being unsized. - // So we eagerly check here if this local has an MPlace, and if yes we use it. let frame = self.frame(); let layout = self.layout_of_local(frame, local, None)?; let place = if layout.is_sized() { // We can just always use the `Local` for sized values. Place::Local { local, offset: None, locals_addr: frame.locals_addr() } } else { - // Unsized `Local` isn't okay (we cannot store the metadata). + // Other parts of the system rely on `Place::Local` never being unsized. match frame.locals[local].access()? { Operand::Immediate(_) => bug!(), Operand::Indirect(mplace) => Place::Ptr(*mplace), @@ -565,7 +563,10 @@ where place: &PlaceTy<'tcx, M::Provenance>, ) -> InterpResult< 'tcx, - Either, (&mut Immediate, TyAndLayout<'tcx>)>, + Either< + MPlaceTy<'tcx, M::Provenance>, + (&mut Immediate, TyAndLayout<'tcx>, mir::Local), + >, > { Ok(match place.to_place().as_mplace_or_local() { Left(mplace) => Left(mplace), @@ -584,7 +585,7 @@ where } Operand::Immediate(local_val) => { // The local still has the optimized representation. - Right((local_val, layout)) + Right((local_val, layout, local)) } } } @@ -646,9 +647,13 @@ where assert!(dest.layout().is_sized(), "Cannot write unsized immediate data"); match self.as_mplace_or_mutable_local(&dest.to_place())? { - Right((local_val, local_layout)) => { + Right((local_val, local_layout, local)) => { // Local can be updated in-place. *local_val = src; + // Call the machine hook (the data race detector needs to know about this write). + if !self.validation_in_progress() { + M::after_local_write(self, local, /*storage_live*/ false)?; + } // Double-check that the value we are storing and the local fit to each other. if cfg!(debug_assertions) { src.assert_matches_abi(local_layout.abi, self); @@ -717,8 +722,12 @@ where dest: &impl Writeable<'tcx, M::Provenance>, ) -> InterpResult<'tcx> { match self.as_mplace_or_mutable_local(&dest.to_place())? { - Right((local_val, _local_layout)) => { + Right((local_val, _local_layout, local)) => { *local_val = Immediate::Uninit; + // Call the machine hook (the data race detector needs to know about this write). + if !self.validation_in_progress() { + M::after_local_write(self, local, /*storage_live*/ false)?; + } } Left(mplace) => { let Some(mut alloc) = self.get_place_alloc_mut(&mplace)? else { @@ -737,8 +746,12 @@ where dest: &impl Writeable<'tcx, M::Provenance>, ) -> InterpResult<'tcx> { match self.as_mplace_or_mutable_local(&dest.to_place())? { - Right((local_val, _local_layout)) => { + Right((local_val, _local_layout, local)) => { local_val.clear_provenance()?; + // Call the machine hook (the data race detector needs to know about this write). + if !self.validation_in_progress() { + M::after_local_write(self, local, /*storage_live*/ false)?; + } } Left(mplace) => { let Some(mut alloc) = self.get_place_alloc_mut(&mplace)? else { @@ -944,7 +957,7 @@ where mplace.mplace, )?; } - M::after_local_allocated(self, local, &mplace)?; + M::after_local_moved_to_memory(self, local, &mplace)?; // Now we can call `access_mut` again, asserting it goes well, and actually // overwrite things. This points to the entire allocation, not just the part // the place refers to, i.e. we do this before we apply `offset`. diff --git a/compiler/rustc_const_eval/src/interpret/stack.rs b/compiler/rustc_const_eval/src/interpret/stack.rs index b6e83715e39..db418c82f66 100644 --- a/compiler/rustc_const_eval/src/interpret/stack.rs +++ b/compiler/rustc_const_eval/src/interpret/stack.rs @@ -534,8 +534,11 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> { let dest_place = self.allocate_dyn(layout, MemoryKind::Stack, meta)?; Operand::Indirect(*dest_place.mplace()) } else { - assert!(!meta.has_meta()); // we're dropping the metadata // Just make this an efficient immediate. + assert!(!meta.has_meta()); // we're dropping the metadata + // Make sure the machine knows this "write" is happening. (This is important so that + // races involving local variable allocation can be detected by Miri.) + M::after_local_write(self, local, /*storage_live*/ true)?; // Note that not calling `layout_of` here does have one real consequence: // if the type is too big, we'll only notice this when the local is actually initialized, // which is a bit too late -- we should ideally notice this already here, when the memory diff --git a/src/tools/miri/src/concurrency/data_race.rs b/src/tools/miri/src/concurrency/data_race.rs index b604fd868a0..d420301400e 100644 --- a/src/tools/miri/src/concurrency/data_race.rs +++ b/src/tools/miri/src/concurrency/data_race.rs @@ -47,6 +47,7 @@ use std::{ }; use rustc_ast::Mutability; +use rustc_data_structures::fx::FxHashMap; use rustc_data_structures::fx::FxHashSet; use rustc_index::{Idx, IndexVec}; use rustc_middle::{mir, ty::Ty}; @@ -1121,6 +1122,103 @@ impl VClockAlloc { } } +/// Vector clock state for a stack frame (tracking the local variables +/// that do not have an allocation yet). +#[derive(Debug, Default)] +pub struct FrameState { + local_clocks: RefCell>, +} + +/// Stripped-down version of [`MemoryCellClocks`] for the clocks we need to keep track +/// of in a local that does not yet have addressable memory -- and hence can only +/// be accessed from the thread its stack frame belongs to, and cannot be access atomically. +#[derive(Debug)] +struct LocalClocks { + write: VTimestamp, + write_type: NaWriteType, + read: VTimestamp, +} + +impl Default for LocalClocks { + fn default() -> Self { + Self { write: VTimestamp::ZERO, write_type: NaWriteType::Allocate, read: VTimestamp::ZERO } + } +} + +impl FrameState { + pub fn local_write(&self, local: mir::Local, storage_live: bool, machine: &MiriMachine<'_>) { + let current_span = machine.current_span(); + let global = machine.data_race.as_ref().unwrap(); + if global.race_detecting() { + let (index, mut thread_clocks) = global.active_thread_state_mut(&machine.threads); + // This should do the same things as `MemoryCellClocks::write_race_detect`. + if !current_span.is_dummy() { + thread_clocks.clock.index_mut(index).span = current_span; + } + let mut clocks = self.local_clocks.borrow_mut(); + if storage_live { + let new_clocks = LocalClocks { + write: thread_clocks.clock[index], + write_type: NaWriteType::Allocate, + read: VTimestamp::ZERO, + }; + // There might already be an entry in the map for this, if the local was previously + // live already. + clocks.insert(local, new_clocks); + } else { + // This can fail to exist if `race_detecting` was false when the allocation + // occurred, in which case we can backdate this to the beginning of time. + let clocks = clocks.entry(local).or_insert_with(Default::default); + clocks.write = thread_clocks.clock[index]; + clocks.write_type = NaWriteType::Write; + } + } + } + + pub fn local_read(&self, local: mir::Local, machine: &MiriMachine<'_>) { + let current_span = machine.current_span(); + let global = machine.data_race.as_ref().unwrap(); + if global.race_detecting() { + let (index, mut thread_clocks) = global.active_thread_state_mut(&machine.threads); + // This should do the same things as `MemoryCellClocks::read_race_detect`. + if !current_span.is_dummy() { + thread_clocks.clock.index_mut(index).span = current_span; + } + thread_clocks.clock.index_mut(index).set_read_type(NaReadType::Read); + // This can fail to exist if `race_detecting` was false when the allocation + // occurred, in which case we can backdate this to the beginning of time. + let mut clocks = self.local_clocks.borrow_mut(); + let clocks = clocks.entry(local).or_insert_with(Default::default); + clocks.read = thread_clocks.clock[index]; + } + } + + pub fn local_moved_to_memory( + &self, + local: mir::Local, + alloc: &mut VClockAlloc, + machine: &MiriMachine<'_>, + ) { + let global = machine.data_race.as_ref().unwrap(); + if global.race_detecting() { + let (index, _thread_clocks) = global.active_thread_state_mut(&machine.threads); + // Get the time the last write actually happened. This can fail to exist if + // `race_detecting` was false when the write occurred, in that case we can backdate this + // to the beginning of time. + let local_clocks = self.local_clocks.borrow_mut().remove(&local).unwrap_or_default(); + for (_mem_clocks_range, mem_clocks) in alloc.alloc_ranges.get_mut().iter_mut_all() { + // The initialization write for this already happened, just at the wrong timestamp. + // Check that the thread index matches what we expect. + assert_eq!(mem_clocks.write.0, index); + // Convert the local's clocks into memory clocks. + mem_clocks.write = (index, local_clocks.write); + mem_clocks.write_type = local_clocks.write_type; + mem_clocks.read = VClock::new_with_index(index, local_clocks.read); + } + } + } +} + impl<'tcx> EvalContextPrivExt<'tcx> for MiriInterpCx<'tcx> {} trait EvalContextPrivExt<'tcx>: MiriInterpCxExt<'tcx> { /// Temporarily allow data-races to occur. This should only be used in diff --git a/src/tools/miri/src/concurrency/thread.rs b/src/tools/miri/src/concurrency/thread.rs index 306245a843b..8c3bee83e46 100644 --- a/src/tools/miri/src/concurrency/thread.rs +++ b/src/tools/miri/src/concurrency/thread.rs @@ -530,7 +530,9 @@ impl<'tcx> ThreadManager<'tcx> { } /// Mutably borrow the stack of the active thread. - fn active_thread_stack_mut(&mut self) -> &mut Vec>> { + pub fn active_thread_stack_mut( + &mut self, + ) -> &mut Vec>> { &mut self.threads[self.active_thread].stack } pub fn all_stacks( diff --git a/src/tools/miri/src/concurrency/vector_clock.rs b/src/tools/miri/src/concurrency/vector_clock.rs index c3496bc1a0c..0968e10bbee 100644 --- a/src/tools/miri/src/concurrency/vector_clock.rs +++ b/src/tools/miri/src/concurrency/vector_clock.rs @@ -130,6 +130,9 @@ impl Ord for VTimestamp { /// also this means that there is only one unique valid length /// for each set of vector clock values and hence the PartialEq /// and Eq derivations are correct. +/// +/// This means we cannot represent a clock where the last entry is a timestamp-0 read that occurs +/// because of a retag. That's fine, all it does is risk wrong diagnostics in a extreme corner case. #[derive(PartialEq, Eq, Default, Debug)] pub struct VClock(SmallVec<[VTimestamp; SMALL_VECTOR]>); @@ -137,6 +140,9 @@ impl VClock { /// Create a new vector-clock containing all zeros except /// for a value at the given index pub(super) fn new_with_index(index: VectorIdx, timestamp: VTimestamp) -> VClock { + if timestamp.time() == 0 { + return VClock::default(); + } let len = index.index() + 1; let mut vec = smallvec::smallvec![VTimestamp::ZERO; len]; vec[index.index()] = timestamp; diff --git a/src/tools/miri/src/machine.rs b/src/tools/miri/src/machine.rs index 76b4366476d..df55902decd 100644 --- a/src/tools/miri/src/machine.rs +++ b/src/tools/miri/src/machine.rs @@ -81,24 +81,42 @@ pub struct FrameExtra<'tcx> { /// an additional bit of "salt" into the cache key. This salt is fixed per-frame /// so that within a call, a const will have a stable address. salt: usize, + + /// Data race detector per-frame data. + pub data_race: Option, } impl<'tcx> std::fmt::Debug for FrameExtra<'tcx> { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { // Omitting `timing`, it does not support `Debug`. - let FrameExtra { borrow_tracker, catch_unwind, timing: _, is_user_relevant: _, salt: _ } = - self; + let FrameExtra { + borrow_tracker, + catch_unwind, + timing: _, + is_user_relevant, + salt, + data_race, + } = self; f.debug_struct("FrameData") .field("borrow_tracker", borrow_tracker) .field("catch_unwind", catch_unwind) + .field("is_user_relevant", is_user_relevant) + .field("salt", salt) + .field("data_race", data_race) .finish() } } impl VisitProvenance for FrameExtra<'_> { fn visit_provenance(&self, visit: &mut VisitWith<'_>) { - let FrameExtra { catch_unwind, borrow_tracker, timing: _, is_user_relevant: _, salt: _ } = - self; + let FrameExtra { + catch_unwind, + borrow_tracker, + timing: _, + is_user_relevant: _, + salt: _, + data_race: _, + } = self; catch_unwind.visit_provenance(visit); borrow_tracker.visit_provenance(visit); @@ -1446,6 +1464,7 @@ impl<'tcx> Machine<'tcx> for MiriMachine<'tcx> { timing, is_user_relevant: ecx.machine.is_user_relevant(&frame), salt: ecx.machine.rng.borrow_mut().gen::() % ADDRS_PER_ANON_GLOBAL, + data_race: ecx.machine.data_race.as_ref().map(|_| data_race::FrameState::default()), }; Ok(frame.with_extra(extra)) @@ -1551,7 +1570,25 @@ impl<'tcx> Machine<'tcx> for MiriMachine<'tcx> { res } - fn after_local_allocated( + fn after_local_read(ecx: &InterpCx<'tcx, Self>, local: mir::Local) -> InterpResult<'tcx> { + if let Some(data_race) = &ecx.frame().extra.data_race { + data_race.local_read(local, &ecx.machine); + } + Ok(()) + } + + fn after_local_write( + ecx: &mut InterpCx<'tcx, Self>, + local: mir::Local, + storage_live: bool, + ) -> InterpResult<'tcx> { + if let Some(data_race) = &ecx.frame().extra.data_race { + data_race.local_write(local, storage_live, &ecx.machine); + } + Ok(()) + } + + fn after_local_moved_to_memory( ecx: &mut InterpCx<'tcx, Self>, local: mir::Local, mplace: &MPlaceTy<'tcx>, @@ -1559,9 +1596,17 @@ impl<'tcx> Machine<'tcx> for MiriMachine<'tcx> { let Some(Provenance::Concrete { alloc_id, .. }) = mplace.ptr().provenance else { panic!("after_local_allocated should only be called on fresh allocations"); }; + // Record the span where this was allocated: the declaration of the local. let local_decl = &ecx.frame().body().local_decls[local]; let span = local_decl.source_info.span; ecx.machine.allocation_spans.borrow_mut().insert(alloc_id, (span, None)); + // The data race system has to fix the clocks used for this write. + let (alloc_info, machine) = ecx.get_alloc_extra_mut(alloc_id)?; + if let Some(data_race) = + &machine.threads.active_thread_stack().last().unwrap().extra.data_race + { + data_race.local_moved_to_memory(local, alloc_info.data_race.as_mut().unwrap(), machine); + } Ok(()) } diff --git a/src/tools/miri/tests/fail/data_race/local_variable_alloc_race.rs b/src/tools/miri/tests/fail/data_race/local_variable_alloc_race.rs new file mode 100644 index 00000000000..751a308a399 --- /dev/null +++ b/src/tools/miri/tests/fail/data_race/local_variable_alloc_race.rs @@ -0,0 +1,57 @@ +//@compile-flags: -Zmiri-preemption-rate=0.0 -Zmiri-disable-weak-memory-emulation +#![feature(core_intrinsics)] +#![feature(custom_mir)] + +use std::intrinsics::mir::*; +use std::sync::atomic::Ordering::*; +use std::sync::atomic::*; +use std::thread::JoinHandle; + +static P: AtomicPtr = AtomicPtr::new(core::ptr::null_mut()); + +fn spawn_thread() -> JoinHandle<()> { + std::thread::spawn(|| { + while P.load(Relaxed).is_null() { + std::hint::spin_loop(); + } + unsafe { + // Initialize `*P`. + let ptr = P.load(Relaxed); + *ptr = 127; + //~^ ERROR: Data race detected between (1) creating a new allocation on thread `main` and (2) non-atomic write on thread `unnamed-1` + } + }) +} + +fn finish(t: JoinHandle<()>, val_ptr: *mut u8) { + P.store(val_ptr, Relaxed); + + // Wait for the thread to be done. + t.join().unwrap(); + + // Read initialized value. + assert_eq!(unsafe { *val_ptr }, 127); +} + +#[custom_mir(dialect = "runtime", phase = "optimized")] +fn main() { + mir! { + let t; + let val; + let val_ptr; + let _ret; + { + Call(t = spawn_thread(), ReturnTo(after_spawn), UnwindContinue()) + } + after_spawn = { + // This races with the write in the other thread. + StorageLive(val); + + val_ptr = &raw mut val; + Call(_ret = finish(t, val_ptr), ReturnTo(done), UnwindContinue()) + } + done = { + Return() + } + } +} diff --git a/src/tools/miri/tests/fail/data_race/local_variable_alloc_race.stderr b/src/tools/miri/tests/fail/data_race/local_variable_alloc_race.stderr new file mode 100644 index 00000000000..f46eb078a51 --- /dev/null +++ b/src/tools/miri/tests/fail/data_race/local_variable_alloc_race.stderr @@ -0,0 +1,20 @@ +error: Undefined Behavior: Data race detected between (1) creating a new allocation on thread `main` and (2) non-atomic write on thread `unnamed-ID` at ALLOC. (2) just happened here + --> $DIR/local_variable_alloc_race.rs:LL:CC + | +LL | *ptr = 127; + | ^^^^^^^^^^ Data race detected between (1) creating a new allocation on thread `main` and (2) non-atomic write on thread `unnamed-ID` at ALLOC. (2) just happened here + | +help: and (1) occurred earlier here + --> $DIR/local_variable_alloc_race.rs:LL:CC + | +LL | StorageLive(val); + | ^^^^^^^^^^^^^^^^ + = 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 (of the first span) on thread `unnamed-ID`: + = note: inside closure at $DIR/local_variable_alloc_race.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/fail/data_race/local_variable_read_race.rs b/src/tools/miri/tests/fail/data_race/local_variable_read_race.rs new file mode 100644 index 00000000000..80d2b7b7c12 --- /dev/null +++ b/src/tools/miri/tests/fail/data_race/local_variable_read_race.rs @@ -0,0 +1,38 @@ +//@compile-flags: -Zmiri-preemption-rate=0.0 -Zmiri-disable-weak-memory-emulation +use std::sync::atomic::Ordering::*; +use std::sync::atomic::*; + +static P: AtomicPtr = AtomicPtr::new(core::ptr::null_mut()); + +fn main() { + // Create the local variable, and initialize it. + let mut val: u8 = 0; + + let t1 = std::thread::spawn(|| { + while P.load(Relaxed).is_null() { + std::hint::spin_loop(); + } + unsafe { + // Initialize `*P`. + let ptr = P.load(Relaxed); + *ptr = 127; + //~^ ERROR: Data race detected between (1) non-atomic read on thread `main` and (2) non-atomic write on thread `unnamed-1` + } + }); + + // This read is not ordered with the store above, and thus should be reported as a race. + let _val = val; + + // Actually generate memory for the local variable. + // This is the time its value is actually written to memory. + // If we just "pre-date" the write to the beginning of time (since we don't know + // when it actually happened), we'd miss the UB in this test. + // Also, the UB error should point at the write above, not the addr-of here. + P.store(std::ptr::addr_of_mut!(val), Relaxed); + + // Wait for the thread to be done. + t1.join().unwrap(); + + // Read initialized value. + assert_eq!(val, 127); +} diff --git a/src/tools/miri/tests/fail/data_race/local_variable_read_race.stderr b/src/tools/miri/tests/fail/data_race/local_variable_read_race.stderr new file mode 100644 index 00000000000..d14c2fb47ff --- /dev/null +++ b/src/tools/miri/tests/fail/data_race/local_variable_read_race.stderr @@ -0,0 +1,20 @@ +error: Undefined Behavior: Data race detected between (1) non-atomic read on thread `main` and (2) non-atomic write on thread `unnamed-ID` at ALLOC. (2) just happened here + --> $DIR/local_variable_read_race.rs:LL:CC + | +LL | *ptr = 127; + | ^^^^^^^^^^ Data race detected between (1) non-atomic read on thread `main` and (2) non-atomic write on thread `unnamed-ID` at ALLOC. (2) just happened here + | +help: and (1) occurred earlier here + --> $DIR/local_variable_read_race.rs:LL:CC + | +LL | let _val = val; + | ^^^ + = 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 (of the first span) on thread `unnamed-ID`: + = note: inside closure at $DIR/local_variable_read_race.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/fail/data_race/local_variable_write_race.rs b/src/tools/miri/tests/fail/data_race/local_variable_write_race.rs new file mode 100644 index 00000000000..eabbe4403c6 --- /dev/null +++ b/src/tools/miri/tests/fail/data_race/local_variable_write_race.rs @@ -0,0 +1,37 @@ +//@compile-flags: -Zmiri-preemption-rate=0.0 -Zmiri-disable-weak-memory-emulation +use std::sync::atomic::Ordering::*; +use std::sync::atomic::*; + +static P: AtomicPtr = AtomicPtr::new(core::ptr::null_mut()); + +fn main() { + let t1 = std::thread::spawn(|| { + while P.load(Relaxed).is_null() { + std::hint::spin_loop(); + } + unsafe { + // Initialize `*P`. + let ptr = P.load(Relaxed); + *ptr = 127; + //~^ ERROR: Data race detected between (1) non-atomic write on thread `main` and (2) non-atomic write on thread `unnamed-1` + } + }); + + // Create the local variable, and initialize it. + // This is not ordered with the store above, so it's definitely UB + // for that thread to access this variable. + let mut val: u8 = 0; + + // Actually generate memory for the local variable. + // This is the time its value is actually written to memory. + // If we just "pre-date" the write to the beginning of time (since we don't know + // when it actually happened), we'd miss the UB in this test. + // Also, the UB error should point at the write above, not the addr-of here. + P.store(std::ptr::addr_of_mut!(val), Relaxed); + + // Wait for the thread to be done. + t1.join().unwrap(); + + // Read initialized value. + assert_eq!(val, 127); +} diff --git a/src/tools/miri/tests/fail/data_race/local_variable_write_race.stderr b/src/tools/miri/tests/fail/data_race/local_variable_write_race.stderr new file mode 100644 index 00000000000..d84db955a3d --- /dev/null +++ b/src/tools/miri/tests/fail/data_race/local_variable_write_race.stderr @@ -0,0 +1,20 @@ +error: Undefined Behavior: Data race detected between (1) non-atomic write on thread `main` and (2) non-atomic write on thread `unnamed-ID` at ALLOC. (2) just happened here + --> $DIR/local_variable_write_race.rs:LL:CC + | +LL | *ptr = 127; + | ^^^^^^^^^^ Data race detected between (1) non-atomic write on thread `main` and (2) non-atomic write on thread `unnamed-ID` at ALLOC. (2) just happened here + | +help: and (1) occurred earlier here + --> $DIR/local_variable_write_race.rs:LL:CC + | +LL | let mut val: u8 = 0; + | ^ + = 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 (of the first span) on thread `unnamed-ID`: + = note: inside closure at $DIR/local_variable_write_race.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/concurrency/data_race.rs b/src/tools/miri/tests/pass/concurrency/data_race.rs index d31420380a5..34380dfa504 100644 --- a/src/tools/miri/tests/pass/concurrency/data_race.rs +++ b/src/tools/miri/tests/pass/concurrency/data_race.rs @@ -1,6 +1,6 @@ //@compile-flags: -Zmiri-disable-weak-memory-emulation -Zmiri-preemption-rate=0 -use std::sync::atomic::{fence, AtomicUsize, Ordering}; +use std::sync::atomic::*; use std::thread::spawn; #[derive(Copy, Clone)] @@ -112,9 +112,41 @@ pub fn test_simple_release() { } } +fn test_local_variable_lazy_write() { + static P: AtomicPtr = AtomicPtr::new(core::ptr::null_mut()); + + // Create the local variable, and initialize it. + // This write happens before the thread is spanwed, so there is no data race. + let mut val: u8 = 0; + + let t1 = std::thread::spawn(|| { + while P.load(Ordering::Relaxed).is_null() { + std::hint::spin_loop(); + } + unsafe { + // Initialize `*P`. + let ptr = P.load(Ordering::Relaxed); + *ptr = 127; + } + }); + + // Actually generate memory for the local variable. + // This is the time its value is actually written to memory: + // that's *after* the thread above was spawned! + // This may hence look like a data race wrt the access in the thread above. + P.store(std::ptr::addr_of_mut!(val), Ordering::Relaxed); + + // Wait for the thread to be done. + t1.join().unwrap(); + + // Read initialized value. + assert_eq!(val, 127); +} + pub fn main() { test_fence_sync(); test_multiple_reads(); test_rmw_no_block(); test_simple_release(); + test_local_variable_lazy_write(); }