Skip to content

Commit

Permalink
allow mixed-size atomic reads
Browse files Browse the repository at this point in the history
  • Loading branch information
RalfJung committed Aug 10, 2024
1 parent 24c19b8 commit e219737
Show file tree
Hide file tree
Showing 13 changed files with 155 additions and 129 deletions.
24 changes: 15 additions & 9 deletions library/core/src/sync/atomic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,12 @@
//! atomic load (via the operations provided in this module). A "modification of an atomic object"
//! refers to an atomic store.
//!
//! The most important aspect of this model is that conflicting non-synchronized accesses are
//! Undefined Behavior unless both accesses are atomic. Here, accesses are *conflicting* if they
//! affect overlapping regions of memory and at least one of them is a write. They are
//! *non-synchronized* if neither of them *happens-before* the other, according to the
//! happens-before order of the memory model.
//!
//! The end result is *almost* equivalent to saying that creating a *shared reference* to one of the
//! Rust atomic types corresponds to creating an `atomic_ref` in C++, with the `atomic_ref` being
//! destroyed when the lifetime of the shared reference ends. The main difference is that Rust
Expand All @@ -41,9 +47,10 @@
//! objects" and "non-atomic objects" (with `atomic_ref` temporarily converting a non-atomic object
//! into an atomic object).
//!
//! That said, Rust *does* inherit the C++ limitation that non-synchronized atomic accesses may not
//! partially overlap: they must be either disjoint or access the exact same memory. This in
//! particular rules out non-synchronized differently-sized accesses to the same data.
//! That said, Rust *does* inherit the C++ limitation that non-synchronized conflicting atomic
//! accesses may not partially overlap: they must be either disjoint or access the exact same
//! memory. This in particular rules out non-synchronized differently-sized atomic accesses to the
//! same data unless all accesses are reads.
//!
//! [cpp]: https://en.cppreference.com/w/cpp/atomic
//! [cpp-intro.races]: https://timsong-cpp.github.io/cppwp/n4868/intro.multithread#intro.races
Expand All @@ -63,7 +70,7 @@
//! let atomic = AtomicU16::new(0);
//!
//! thread::scope(|s| {
//! // This is UB: conflicting concurrent accesses.
//! // This is UB: conflicting non-synchronized accesses.
//! s.spawn(|| atomic.store(1, Ordering::Relaxed)); // atomic store
//! s.spawn(|| unsafe { atomic.as_ptr().write(2) }); // non-atomic write
//! });
Expand All @@ -77,16 +84,15 @@
//! });
//!
//! thread::scope(|s| {
//! // This is fine, `join` synchronizes the code in a way such that atomic
//! // and non-atomic accesses can't happen "at the same time".
//! // This is fine, `join` synchronizes the code in a way such that the atomic
//! // store happens-before the non-atomic write.
//! let handle = s.spawn(|| atomic.store(1, Ordering::Relaxed)); // atomic store
//! handle.join().unwrap(); // synchronize
//! s.spawn(|| unsafe { atomic.as_ptr().write(2) }); // non-atomic write
//! });
//!
//! thread::scope(|s| {
//! // This is UB: using differently-sized atomic accesses to the same data.
//! // (It would be UB even if these are both loads.)
//! // This is UB: non-synchronized conflicting differently-sized atomic accesses.
//! s.spawn(|| atomic.store(1, Ordering::Relaxed));
//! s.spawn(|| unsafe {
//! let differently_sized = transmute::<&AtomicU16, &AtomicU8>(&atomic);
Expand All @@ -96,7 +102,7 @@
//!
//! thread::scope(|s| {
//! // This is fine, `join` synchronizes the code in a way such that
//! // differently-sized accesses can't happen "at the same time".
//! // the 1-byte store happens-before the 2-byte store.
//! let handle = s.spawn(|| atomic.store(1, Ordering::Relaxed));
//! handle.join().unwrap();
//! s.spawn(|| unsafe {
Expand Down
40 changes: 27 additions & 13 deletions src/tools/miri/src/concurrency/data_race.rs
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,8 @@ struct AtomicMemoryCellClocks {
/// The size of accesses to this atomic location.
/// We use this to detect non-synchronized mixed-size accesses. Since all accesses must be
/// aligned to their size, this is sufficient to detect imperfectly overlapping accesses.
size: Size,
/// `None` indicates that we saw multiple different sizes, which is okay as long as it's all about reads.
size: Option<Size>,
}

#[derive(Copy, Clone, PartialEq, Eq, Debug)]
Expand Down Expand Up @@ -264,6 +265,14 @@ impl AccessType {
let mut msg = String::new();

if let Some(size) = size {
if size == Size::ZERO {
// In this case where were multiple read accesss with different sizes and then a write.
// We will be reporting *one* of the other reads, but we don't have enough information
// to determine which one had which size.
assert!(self == AccessType::AtomicLoad);
assert!(ty.is_none());
return format!("multiple differently-sized atomic loads, including one load");
}
msg.push_str(&format!("{}-byte {}", size.bytes(), msg))
}

Expand Down Expand Up @@ -304,8 +313,7 @@ impl AccessType {
}
}

/// Memory Cell vector clock metadata
/// for data-race detection.
/// Per-byte vector clock metadata for data-race detection.
#[derive(Clone, PartialEq, Eq, Debug)]
struct MemoryCellClocks {
/// The vector-clock timestamp and the thread that did the last non-atomic write. We don't need
Expand All @@ -324,8 +332,8 @@ struct MemoryCellClocks {
read: VClock,

/// Atomic access, acquire, release sequence tracking clocks.
/// For non-atomic memory in the common case this
/// value is set to None.
/// For non-atomic memory this value is set to None.
/// For atomic memory, each byte carries this information.
atomic_ops: Option<Box<AtomicMemoryCellClocks>>,
}

Expand All @@ -335,7 +343,7 @@ impl AtomicMemoryCellClocks {
read_vector: Default::default(),
write_vector: Default::default(),
sync_vector: Default::default(),
size,
size: Some(size),
}
}
}
Expand Down Expand Up @@ -382,17 +390,23 @@ impl MemoryCellClocks {
&mut self,
thread_clocks: &ThreadClockSet,
size: Size,
write: bool,
) -> Result<&mut AtomicMemoryCellClocks, DataRace> {
match self.atomic_ops {
Some(ref mut atomic) => {
// We are good if the size is the same or all atomic accesses are before our current time.
if atomic.size == size {
if atomic.size == Some(size) {
Ok(atomic)
} else if atomic.read_vector <= thread_clocks.clock
&& atomic.write_vector <= thread_clocks.clock
{
// This is now the new size that must be used for accesses here.
atomic.size = size;
// We are fully ordered after all previous accesses, so we can change the size.
atomic.size = Some(size);
Ok(atomic)
} else if !write && atomic.write_vector <= thread_clocks.clock {
// This is a read, and it is ordered after the last write. It's okay for the
// sizes to mismatch, as long as no writes with a different size occur later.
atomic.size = None;
Ok(atomic)
} else {
Err(DataRace)
Expand Down Expand Up @@ -507,7 +521,7 @@ impl MemoryCellClocks {
access_size: Size,
) -> Result<(), DataRace> {
trace!("Atomic read with vectors: {:#?} :: {:#?}", self, thread_clocks);
let atomic = self.atomic_access(thread_clocks, access_size)?;
let atomic = self.atomic_access(thread_clocks, access_size, /*write*/ false)?;
atomic.read_vector.set_at_index(&thread_clocks.clock, index);
// Make sure the last non-atomic write was before this access.
if self.write_was_before(&thread_clocks.clock) { Ok(()) } else { Err(DataRace) }
Expand All @@ -522,7 +536,7 @@ impl MemoryCellClocks {
access_size: Size,
) -> Result<(), DataRace> {
trace!("Atomic write with vectors: {:#?} :: {:#?}", self, thread_clocks);
let atomic = self.atomic_access(thread_clocks, access_size)?;
let atomic = self.atomic_access(thread_clocks, access_size, /*write*/ true)?;
atomic.write_vector.set_at_index(&thread_clocks.clock, index);
// Make sure the last non-atomic write and all non-atomic reads were before this access.
if self.write_was_before(&thread_clocks.clock) && self.read <= thread_clocks.clock {
Expand Down Expand Up @@ -967,10 +981,10 @@ impl VClockAlloc {
} else if let Some(idx) = Self::find_gt_index(&mem_clocks.read, &active_clocks.clock) {
(AccessType::NaRead(mem_clocks.read[idx].read_type()), idx, &mem_clocks.read)
// Finally, mixed-size races.
} else if access.is_atomic() && let Some(atomic) = mem_clocks.atomic() && atomic.size != access_size {
} else if access.is_atomic() && let Some(atomic) = mem_clocks.atomic() && atomic.size != Some(access_size) {
// This is only a race if we are not synchronized with all atomic accesses, so find
// the one we are not synchronized with.
other_size = Some(atomic.size);
other_size = Some(atomic.size.unwrap_or(Size::ZERO));
if let Some(idx) = Self::find_gt_index(&atomic.write_vector, &active_clocks.clock)
{
(AccessType::AtomicStore, idx, &atomic.write_vector)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,11 @@ fn main() {
});
s.spawn(|| {
a8[0].load(Ordering::SeqCst);
//~^ ERROR: Race condition detected between (1) 2-byte atomic load on thread `unnamed-1` and (2) 1-byte atomic load on thread `unnamed-2`
});
s.spawn(|| {
thread::yield_now(); // make sure this happens last
a16.store(0, Ordering::SeqCst);
//~^ ERROR: Race condition detected between (1) multiple differently-sized atomic loads, including one load on thread `unnamed-1` and (2) 2-byte atomic store on thread `unnamed-3`
});
});
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
error: Undefined Behavior: Race condition detected between (1) multiple differently-sized atomic loads, including one load on thread `unnamed-ID` and (2) 2-byte atomic store on thread `unnamed-ID` at ALLOC. (2) just happened here
--> $DIR/mixed_size_read_read_write.rs:LL:CC
|
LL | a16.store(0, Ordering::SeqCst);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Race condition detected between (1) multiple differently-sized atomic loads, including one load on thread `unnamed-ID` and (2) 2-byte atomic store on thread `unnamed-ID` at ALLOC. (2) just happened here
|
help: and (1) occurred earlier here
--> $DIR/mixed_size_read_read_write.rs:LL:CC
|
LL | a16.load(Ordering::SeqCst);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^
= help: overlapping unsynchronized atomic accesses must use the same access size
= help: see https://doc.rust-lang.org/nightly/std/sync/atomic/index.html#memory-model-for-atomic-accesses for more information about the Rust memory model
= 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/mixed_size_read_read_write.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

Original file line number Diff line number Diff line change
@@ -1,20 +1,20 @@
error: Undefined Behavior: Race condition detected between (1) 4-byte atomic store on thread `unnamed-ID` and (2) 2-byte atomic load on thread `unnamed-ID` at ALLOC. (2) just happened here
--> $DIR/racing_mixed_size.rs:LL:CC
error: Undefined Behavior: Race condition detected between (1) 1-byte atomic load on thread `unnamed-ID` and (2) 2-byte atomic store on thread `unnamed-ID` at ALLOC. (2) just happened here
--> $DIR/mixed_size_read_write.rs:LL:CC
|
LL | std::intrinsics::atomic_load_relaxed(hi);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Race condition detected between (1) 4-byte atomic store on thread `unnamed-ID` and (2) 2-byte atomic load on thread `unnamed-ID` at ALLOC. (2) just happened here
LL | a16.store(1, Ordering::SeqCst);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Race condition detected between (1) 1-byte atomic load on thread `unnamed-ID` and (2) 2-byte atomic store on thread `unnamed-ID` at ALLOC. (2) just happened here
|
help: and (1) occurred earlier here
--> $DIR/racing_mixed_size.rs:LL:CC
--> $DIR/mixed_size_read_write.rs:LL:CC
|
LL | x.store(1, Relaxed);
| ^^^^^^^^^^^^^^^^^^^
LL | a8[0].load(Ordering::SeqCst);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
= help: overlapping unsynchronized atomic accesses must use the same access size
= help: see https://doc.rust-lang.org/nightly/std/sync/atomic/index.html#memory-model-for-atomic-accesses for more information about the Rust memory model
= 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/racing_mixed_size.rs:LL:CC
= note: inside closure at $DIR/mixed_size_read_write.rs:LL:CC

note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace

Expand Down
39 changes: 39 additions & 0 deletions src/tools/miri/tests/fail/data_race/mixed_size_read_write.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
//@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
// Two revisions, depending on which access goes first.
//@revisions: read_write write_read

use std::sync::atomic::{AtomicU16, AtomicU8, Ordering};
use std::thread;

fn convert(a: &AtomicU16) -> &[AtomicU8; 2] {
unsafe { std::mem::transmute(a) }
}

// We can't allow mixed-size accesses; they are not possible in C++ and even
// Intel says you shouldn't do it.
fn main() {
let a = AtomicU16::new(0);
let a16 = &a;
let a8 = convert(a16);

thread::scope(|s| {
s.spawn(|| {
if cfg!(read_write) {
// Let the other one go first.
thread::yield_now();
}
a16.store(1, Ordering::SeqCst);
//~[read_write]^ ERROR: Race condition detected between (1) 1-byte atomic load on thread `unnamed-2` and (2) 2-byte atomic store on thread `unnamed-1`
});
s.spawn(|| {
if cfg!(write_read) {
// Let the other one go first.
thread::yield_now();
}
a8[0].load(Ordering::SeqCst);
//~[write_read]^ ERROR: Race condition detected between (1) 2-byte atomic store on thread `unnamed-1` and (2) 1-byte atomic load on thread `unnamed-2`
});
});
}
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
error: Undefined Behavior: Race condition detected between (1) 2-byte atomic store on thread `unnamed-ID` and (2) 1-byte atomic store on thread `unnamed-ID` at ALLOC. (2) just happened here
--> $DIR/mixed_size_write.rs:LL:CC
--> $DIR/mixed_size_read_write.rs:LL:CC
|
LL | a8[0].store(1, Ordering::SeqCst);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Race condition detected between (1) 2-byte atomic store on thread `unnamed-ID` and (2) 1-byte atomic store on thread `unnamed-ID` at ALLOC. (2) just happened here
|
help: and (1) occurred earlier here
--> $DIR/mixed_size_write.rs:LL:CC
--> $DIR/mixed_size_read_write.rs:LL:CC
|
LL | a16.store(1, Ordering::SeqCst);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Expand All @@ -14,7 +14,7 @@ LL | a16.store(1, Ordering::SeqCst);
= 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/mixed_size_write.rs:LL:CC
= note: inside closure at $DIR/mixed_size_read_write.rs:LL:CC

note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace

Expand Down
Original file line number Diff line number Diff line change
@@ -1,20 +1,20 @@
error: Undefined Behavior: Race condition detected between (1) 2-byte atomic load on thread `unnamed-ID` and (2) 1-byte atomic load on thread `unnamed-ID` at ALLOC. (2) just happened here
--> $DIR/mixed_size_read.rs:LL:CC
error: Undefined Behavior: Race condition detected between (1) 2-byte atomic store on thread `unnamed-ID` and (2) 1-byte atomic load on thread `unnamed-ID` at ALLOC. (2) just happened here
--> $DIR/mixed_size_read_write.rs:LL:CC
|
LL | a8[0].load(Ordering::SeqCst);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Race condition detected between (1) 2-byte atomic load on thread `unnamed-ID` and (2) 1-byte atomic load on thread `unnamed-ID` at ALLOC. (2) just happened here
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Race condition detected between (1) 2-byte atomic store on thread `unnamed-ID` and (2) 1-byte atomic load on thread `unnamed-ID` at ALLOC. (2) just happened here
|
help: and (1) occurred earlier here
--> $DIR/mixed_size_read.rs:LL:CC
--> $DIR/mixed_size_read_write.rs:LL:CC
|
LL | a16.load(Ordering::SeqCst);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^
LL | a16.store(1, Ordering::SeqCst);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
= help: overlapping unsynchronized atomic accesses must use the same access size
= help: see https://doc.rust-lang.org/nightly/std/sync/atomic/index.html#memory-model-for-atomic-accesses for more information about the Rust memory model
= 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/mixed_size_read.rs:LL:CC
= note: inside closure at $DIR/mixed_size_read_write.rs:LL:CC

note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace

Expand Down
Loading

0 comments on commit e219737

Please sign in to comment.