Skip to content

Commit

Permalink
Further clarificarion for atomic and UnsafeCell docs:
Browse files Browse the repository at this point in the history
- UnsafeCell: mention the term "data race", and reference the data race definition
- atomic: failing RMWs are just reads, reorder and reword docs
  • Loading branch information
RalfJung committed Sep 28, 2024
1 parent 6ca5e29 commit 96be76b
Show file tree
Hide file tree
Showing 3 changed files with 41 additions and 23 deletions.
10 changes: 6 additions & 4 deletions library/core/src/cell.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1895,11 +1895,17 @@ impl<T: ?Sized + fmt::Display> fmt::Display for RefMut<'_, T> {
/// uniqueness guarantee for mutable references is unaffected. There is *no* legal way to obtain
/// aliasing `&mut`, not even with `UnsafeCell<T>`.
///
/// `UnsafeCell` does nothing to avoid data races; they are still undefined behavior. If multiple
/// threads have access to the same `UnsafeCell`, they must follow the usual rules of the
/// [concurrent memory model]: conflicting non-synchronized accesses must be done via the APIs in
/// [`core::sync::atomic`].
///
/// The `UnsafeCell` API itself is technically very simple: [`.get()`] gives you a raw pointer
/// `*mut T` to its contents. It is up to _you_ as the abstraction designer to use that raw pointer
/// correctly.
///
/// [`.get()`]: `UnsafeCell::get`
/// [concurrent memory model]: ../sync/atomic/index.html#memory-model-for-atomic-accesses
///
/// The precise Rust aliasing rules are somewhat in flux, but the main points are not contentious:
///
Expand All @@ -1922,10 +1928,6 @@ impl<T: ?Sized + fmt::Display> fmt::Display for RefMut<'_, T> {
/// live memory and the compiler is allowed to insert spurious reads if it can prove that this
/// memory has not yet been deallocated.
///
/// - At all times, you must avoid data races. If multiple threads have access to
/// the same `UnsafeCell`, then any writes must have a proper happens-before relation to all other
/// accesses (or use atomics).
///
/// To assist with proper design, the following scenarios are explicitly declared legal
/// for single-threaded code:
///
Expand Down
37 changes: 18 additions & 19 deletions library/core/src/sync/atomic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,12 +33,6 @@
//! 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 @@ -47,20 +41,25 @@
//! 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 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.
//! The most important aspect of this model is that *data races* are undefined behavior. A data race
//! is defined as conflicting non-synchronized accesses where at least one of the accesses is
//! non-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.
//!
//! [cpp]: https://en.cppreference.com/w/cpp/atomic
//! [cpp-intro.races]: https://timsong-cpp.github.io/cppwp/n4868/intro.multithread#intro.races
//! The other possible cause of undefined behavior in the memory model are mixed-size accesses: Rust
//! inherits the C++ limitation that non-synchronized conflicting atomic accesses may not partially
//! overlap. In other words, every pair of non-synchronized atomic accesses must be either disjoint,
//! access the exact same memory (including using the same access size), or both be reads.
//!
//! Each method takes an [`Ordering`] which represents the strength of
//! the memory barrier for that operation. These orderings behave the
//! same as the corresponding [C++20 atomic orderings][1]. For more information see the [nomicon][2].
//! Each atomic access takes an [`Ordering`] which defines how the operation interacts with the
//! happens-before order. These orderings behave the same as the corresponding [C++20 atomic
//! orderings][cpp_memory_order]. For more information, see the [nomicon].
//!
//! [1]: https://en.cppreference.com/w/cpp/atomic/memory_order
//! [2]: ../../../nomicon/atomics.html
//! [cpp]: https://en.cppreference.com/w/cpp/atomic
//! [cpp-intro.races]: https://timsong-cpp.github.io/cppwp/n4868/intro.multithread#intro.races
//! [cpp_memory_order]: https://en.cppreference.com/w/cpp/atomic/memory_order
//! [nomicon]: ../../../nomicon/atomics.html
//!
//! ```rust,no_run undefined_behavior
//! use std::sync::atomic::{AtomicU16, AtomicU8, Ordering};
Expand Down Expand Up @@ -157,7 +156,7 @@
//!
//! # Atomic accesses to read-only memory
//!
//! In general, *all* atomic accesses on read-only memory are Undefined Behavior. For instance, attempting
//! In general, *all* atomic accesses on read-only memory are undefined behavior. For instance, attempting
//! to do a `compare_exchange` that will definitely fail (making it conceptually a read-only
//! operation) can still cause a segmentation fault if the underlying memory page is mapped read-only. Since
//! atomic `load`s might be implemented using compare-exchange operations, even a `load` can fault
Expand All @@ -173,7 +172,7 @@
//!
//! As an exception from the general rule stated above, "sufficiently small" atomic loads with
//! `Ordering::Relaxed` are implemented in a way that works on read-only memory, and are hence not
//! Undefined Behavior. The exact size limit for what makes a load "sufficiently small" varies
//! undefined behavior. The exact size limit for what makes a load "sufficiently small" varies
//! depending on the target:
//!
//! | `target_arch` | Size limit |
Expand Down
17 changes: 17 additions & 0 deletions src/tools/miri/tests/pass/concurrency/data_race.rs
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,22 @@ fn mixed_size_read_read() {
});
}

fn failing_rmw_is_read() {
let a = AtomicUsize::new(0);
thread::scope(|s| {
s.spawn(|| unsafe {
// Non-atomic read.
let _val = *(&a as *const AtomicUsize).cast::<usize>();
});

s.spawn(|| {
// RMW that will fail.
// This is not considered a write, so there is no data race here.
a.compare_exchange(1, 2, Ordering::SeqCst, Ordering::SeqCst).unwrap_err();
});
});
}

pub fn main() {
test_fence_sync();
test_multiple_reads();
Expand All @@ -206,4 +222,5 @@ pub fn main() {
test_read_read_race1();
test_read_read_race2();
mixed_size_read_read();
failing_rmw_is_read();
}

0 comments on commit 96be76b

Please sign in to comment.