Skip to content

Commit 4382807

Browse files
RalfJungRalfJung
committedAug 10, 2024·
allow mixed-size atomic reads
1 parent 24c19b8 commit 4382807

13 files changed

+150
-124
lines changed
 

‎library/core/src/sync/atomic.rs

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,12 @@
3333
//! atomic load (via the operations provided in this module). A "modification of an atomic object"
3434
//! refers to an atomic store.
3535
//!
36+
//! The most important aspect of this model is that conflicting non-synchronized accesses are
37+
//! Undefined Behavior unless both accesses are atomic. Here, accesses are *conflicting* if they
38+
//! affect overlapping regions of memory and at least one of them is a write. They are
39+
//! *non-synchronized* if neither of them *happens-before* the other, according to the
40+
//! happens-before order of the memory model.
41+
//!
3642
//! The end result is *almost* equivalent to saying that creating a *shared reference* to one of the
3743
//! Rust atomic types corresponds to creating an `atomic_ref` in C++, with the `atomic_ref` being
3844
//! destroyed when the lifetime of the shared reference ends. The main difference is that Rust
@@ -41,9 +47,10 @@
4147
//! objects" and "non-atomic objects" (with `atomic_ref` temporarily converting a non-atomic object
4248
//! into an atomic object).
4349
//!
44-
//! That said, Rust *does* inherit the C++ limitation that non-synchronized atomic accesses may not
45-
//! partially overlap: they must be either disjoint or access the exact same memory. This in
46-
//! particular rules out non-synchronized differently-sized accesses to the same data.
50+
//! That said, Rust *does* inherit the C++ limitation that non-synchronized conflicting atomic
51+
//! accesses may not partially overlap: they must be either disjoint or access the exact same
52+
//! memory. This in particular rules out non-synchronized differently-sized atomic accesses to the
53+
//! same data unless all accesses are reads.
4754
//!
4855
//! [cpp]: https://en.cppreference.com/w/cpp/atomic
4956
//! [cpp-intro.races]: https://timsong-cpp.github.io/cppwp/n4868/intro.multithread#intro.races
@@ -86,7 +93,6 @@
8693
//!
8794
//! thread::scope(|s| {
8895
//! // This is UB: using differently-sized atomic accesses to the same data.
89-
//! // (It would be UB even if these are both loads.)
9096
//! s.spawn(|| atomic.store(1, Ordering::Relaxed));
9197
//! s.spawn(|| unsafe {
9298
//! let differently_sized = transmute::<&AtomicU16, &AtomicU8>(&atomic);

‎src/tools/miri/src/concurrency/data_race.rs

Lines changed: 27 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -190,7 +190,8 @@ struct AtomicMemoryCellClocks {
190190
/// The size of accesses to this atomic location.
191191
/// We use this to detect non-synchronized mixed-size accesses. Since all accesses must be
192192
/// aligned to their size, this is sufficient to detect imperfectly overlapping accesses.
193-
size: Size,
193+
/// `None` indicates that we saw multiple different sizes, which is okay as long as it's all about reads.
194+
size: Option<Size>,
194195
}
195196

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

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

@@ -304,8 +313,7 @@ impl AccessType {
304313
}
305314
}
306315

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

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

@@ -335,7 +343,7 @@ impl AtomicMemoryCellClocks {
335343
read_vector: Default::default(),
336344
write_vector: Default::default(),
337345
sync_vector: Default::default(),
338-
size,
346+
size: Some(size),
339347
}
340348
}
341349
}
@@ -382,17 +390,23 @@ impl MemoryCellClocks {
382390
&mut self,
383391
thread_clocks: &ThreadClockSet,
384392
size: Size,
393+
write: bool,
385394
) -> Result<&mut AtomicMemoryCellClocks, DataRace> {
386395
match self.atomic_ops {
387396
Some(ref mut atomic) => {
388397
// We are good if the size is the same or all atomic accesses are before our current time.
389-
if atomic.size == size {
398+
if atomic.size == Some(size) {
390399
Ok(atomic)
391400
} else if atomic.read_vector <= thread_clocks.clock
392401
&& atomic.write_vector <= thread_clocks.clock
393402
{
394-
// This is now the new size that must be used for accesses here.
395-
atomic.size = size;
403+
// We are fully ordered after all previous accesses, so we can change the size.
404+
atomic.size = Some(size);
405+
Ok(atomic)
406+
} else if !write && atomic.write_vector <= thread_clocks.clock {
407+
// This is a read, and it is ordered after the last write. It's okay for the
408+
// sizes to mismatch, as long as no writes with a different size occur later.
409+
atomic.size = None;
396410
Ok(atomic)
397411
} else {
398412
Err(DataRace)
@@ -507,7 +521,7 @@ impl MemoryCellClocks {
507521
access_size: Size,
508522
) -> Result<(), DataRace> {
509523
trace!("Atomic read with vectors: {:#?} :: {:#?}", self, thread_clocks);
510-
let atomic = self.atomic_access(thread_clocks, access_size)?;
524+
let atomic = self.atomic_access(thread_clocks, access_size, /*write*/ false)?;
511525
atomic.read_vector.set_at_index(&thread_clocks.clock, index);
512526
// Make sure the last non-atomic write was before this access.
513527
if self.write_was_before(&thread_clocks.clock) { Ok(()) } else { Err(DataRace) }
@@ -522,7 +536,7 @@ impl MemoryCellClocks {
522536
access_size: Size,
523537
) -> Result<(), DataRace> {
524538
trace!("Atomic write with vectors: {:#?} :: {:#?}", self, thread_clocks);
525-
let atomic = self.atomic_access(thread_clocks, access_size)?;
539+
let atomic = self.atomic_access(thread_clocks, access_size, /*write*/ true)?;
526540
atomic.write_vector.set_at_index(&thread_clocks.clock, index);
527541
// Make sure the last non-atomic write and all non-atomic reads were before this access.
528542
if self.write_was_before(&thread_clocks.clock) && self.read <= thread_clocks.clock {
@@ -967,10 +981,10 @@ impl VClockAlloc {
967981
} else if let Some(idx) = Self::find_gt_index(&mem_clocks.read, &active_clocks.clock) {
968982
(AccessType::NaRead(mem_clocks.read[idx].read_type()), idx, &mem_clocks.read)
969983
// Finally, mixed-size races.
970-
} else if access.is_atomic() && let Some(atomic) = mem_clocks.atomic() && atomic.size != access_size {
984+
} else if access.is_atomic() && let Some(atomic) = mem_clocks.atomic() && atomic.size != Some(access_size) {
971985
// This is only a race if we are not synchronized with all atomic accesses, so find
972986
// the one we are not synchronized with.
973-
other_size = Some(atomic.size);
987+
other_size = Some(atomic.size.unwrap_or(Size::ZERO));
974988
if let Some(idx) = Self::find_gt_index(&atomic.write_vector, &active_clocks.clock)
975989
{
976990
(AccessType::AtomicStore, idx, &atomic.write_vector)

‎src/tools/miri/tests/fail/data_race/mixed_size_read.rs renamed to ‎src/tools/miri/tests/fail/data_race/mixed_size_read_read_write.rs

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,11 @@ fn main() {
2222
});
2323
s.spawn(|| {
2424
a8[0].load(Ordering::SeqCst);
25-
//~^ ERROR: Race condition detected between (1) 2-byte atomic load on thread `unnamed-1` and (2) 1-byte atomic load on thread `unnamed-2`
25+
});
26+
s.spawn(|| {
27+
thread::yield_now(); // make sure this happens last
28+
a16.store(0, Ordering::SeqCst);
29+
//~^ 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`
2630
});
2731
});
2832
}

‎src/tools/miri/tests/fail/data_race/mixed_size_read_read_write.stderr

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
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
2+
--> $DIR/mixed_size_read_read_write.rs:LL:CC
3+
|
4+
LL | a16.store(0, Ordering::SeqCst);
5+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 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
6+
|
7+
help: and (1) occurred earlier here
8+
--> $DIR/mixed_size_read_read_write.rs:LL:CC
9+
|
10+
LL | a16.load(Ordering::SeqCst);
11+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^
12+
= help: overlapping unsynchronized atomic accesses must use the same access size
13+
= 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
14+
= help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
15+
= help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
16+
= note: BACKTRACE (of the first span) on thread `unnamed-ID`:
17+
= note: inside closure at $DIR/mixed_size_read_read_write.rs:LL:CC
18+
19+
note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace
20+
21+
error: aborting due to 1 previous error
22+

‎src/tools/miri/tests/fail/weak_memory/racing_mixed_size.stderr renamed to ‎src/tools/miri/tests/fail/data_race/mixed_size_read_write.read_write.stderr

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,20 +1,20 @@
1-
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
2-
--> $DIR/racing_mixed_size.rs:LL:CC
1+
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
2+
--> $DIR/mixed_size_read_write.rs:LL:CC
33
|
4-
LL | std::intrinsics::atomic_load_relaxed(hi);
5-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 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
4+
LL | a16.store(1, Ordering::SeqCst);
5+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 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
66
|
77
help: and (1) occurred earlier here
8-
--> $DIR/racing_mixed_size.rs:LL:CC
8+
--> $DIR/mixed_size_read_write.rs:LL:CC
99
|
10-
LL | x.store(1, Relaxed);
11-
| ^^^^^^^^^^^^^^^^^^^
10+
LL | a8[0].load(Ordering::SeqCst);
11+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
1212
= help: overlapping unsynchronized atomic accesses must use the same access size
1313
= 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
1414
= help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
1515
= help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
1616
= note: BACKTRACE (of the first span) on thread `unnamed-ID`:
17-
= note: inside closure at $DIR/racing_mixed_size.rs:LL:CC
17+
= note: inside closure at $DIR/mixed_size_read_write.rs:LL:CC
1818

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

‎src/tools/miri/tests/fail/data_race/mixed_size_read_write.rs

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
//@compile-flags: -Zmiri-preemption-rate=0.0 -Zmiri-disable-weak-memory-emulation
2+
// Avoid accidental synchronization via address reuse inside `thread::spawn`.
3+
//@compile-flags: -Zmiri-address-reuse-cross-thread-rate=0
4+
// Two revisions, depending on which access goes first.
5+
//@revisions: read_write write_read
6+
7+
use std::sync::atomic::{AtomicU16, AtomicU8, Ordering};
8+
use std::thread;
9+
10+
fn convert(a: &AtomicU16) -> &[AtomicU8; 2] {
11+
unsafe { std::mem::transmute(a) }
12+
}
13+
14+
// We can't allow mixed-size accesses; they are not possible in C++ and even
15+
// Intel says you shouldn't do it.
16+
fn main() {
17+
let a = AtomicU16::new(0);
18+
let a16 = &a;
19+
let a8 = convert(a16);
20+
21+
thread::scope(|s| {
22+
s.spawn(|| {
23+
if cfg!(read_write) {
24+
// Let the other one go first.
25+
thread::yield_now();
26+
}
27+
a16.store(1, Ordering::SeqCst);
28+
//~[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`
29+
});
30+
s.spawn(|| {
31+
if cfg!(write_read) {
32+
// Let the other one go first.
33+
thread::yield_now();
34+
}
35+
a8[0].load(Ordering::SeqCst);
36+
//~[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`
37+
});
38+
});
39+
}

‎src/tools/miri/tests/fail/data_race/mixed_size_write.stderr renamed to ‎src/tools/miri/tests/fail/data_race/mixed_size_read_write.stderr

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,11 @@
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
2-
--> $DIR/mixed_size_write.rs:LL:CC
2+
--> $DIR/mixed_size_read_write.rs:LL:CC
33
|
44
LL | a8[0].store(1, Ordering::SeqCst);
55
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 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
66
|
77
help: and (1) occurred earlier here
8-
--> $DIR/mixed_size_write.rs:LL:CC
8+
--> $DIR/mixed_size_read_write.rs:LL:CC
99
|
1010
LL | a16.store(1, Ordering::SeqCst);
1111
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -14,7 +14,7 @@ LL | a16.store(1, Ordering::SeqCst);
1414
= help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
1515
= help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
1616
= note: BACKTRACE (of the first span) on thread `unnamed-ID`:
17-
= note: inside closure at $DIR/mixed_size_write.rs:LL:CC
17+
= note: inside closure at $DIR/mixed_size_read_write.rs:LL:CC
1818

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

‎src/tools/miri/tests/fail/data_race/mixed_size_read.stderr renamed to ‎src/tools/miri/tests/fail/data_race/mixed_size_read_write.write_read.stderr

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,20 +1,20 @@
1-
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
2-
--> $DIR/mixed_size_read.rs:LL:CC
1+
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
2+
--> $DIR/mixed_size_read_write.rs:LL:CC
33
|
44
LL | a8[0].load(Ordering::SeqCst);
5-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 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
5+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 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
66
|
77
help: and (1) occurred earlier here
8-
--> $DIR/mixed_size_read.rs:LL:CC
8+
--> $DIR/mixed_size_read_write.rs:LL:CC
99
|
10-
LL | a16.load(Ordering::SeqCst);
11-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^
10+
LL | a16.store(1, Ordering::SeqCst);
11+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
1212
= help: overlapping unsynchronized atomic accesses must use the same access size
1313
= 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
1414
= help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
1515
= help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
1616
= note: BACKTRACE (of the first span) on thread `unnamed-ID`:
17-
= note: inside closure at $DIR/mixed_size_read.rs:LL:CC
17+
= note: inside closure at $DIR/mixed_size_read_write.rs:LL:CC
1818

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

‎src/tools/miri/tests/fail/weak_memory/racing_mixed_size_read.stderr renamed to ‎src/tools/miri/tests/fail/data_race/mixed_size_write_write.stderr

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,20 +1,20 @@
1-
error: Undefined Behavior: Race condition detected between (1) 4-byte atomic load on thread `unnamed-ID` and (2) 2-byte atomic load on thread `unnamed-ID` at ALLOC. (2) just happened here
2-
--> $DIR/racing_mixed_size_read.rs:LL:CC
1+
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
2+
--> $DIR/mixed_size_write_write.rs:LL:CC
33
|
4-
LL | (*hi).load(Relaxed);
5-
| ^^^^^^^^^^^^^^^^^^^ Race condition detected between (1) 4-byte atomic load on thread `unnamed-ID` and (2) 2-byte atomic load on thread `unnamed-ID` at ALLOC. (2) just happened here
4+
LL | a8[0].store(1, Ordering::SeqCst);
5+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 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
66
|
77
help: and (1) occurred earlier here
8-
--> $DIR/racing_mixed_size_read.rs:LL:CC
8+
--> $DIR/mixed_size_write_write.rs:LL:CC
99
|
10-
LL | x.load(Relaxed);
11-
| ^^^^^^^^^^^^^^^
10+
LL | a16.store(1, Ordering::SeqCst);
11+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
1212
= help: overlapping unsynchronized atomic accesses must use the same access size
1313
= 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
1414
= help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
1515
= help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
1616
= note: BACKTRACE (of the first span) on thread `unnamed-ID`:
17-
= note: inside closure at $DIR/racing_mixed_size_read.rs:LL:CC
17+
= note: inside closure at $DIR/mixed_size_write_write.rs:LL:CC
1818

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

0 commit comments

Comments
 (0)