Skip to content

Commit

Permalink
accept some atomic loads from read-only memory
Browse files Browse the repository at this point in the history
  • Loading branch information
RalfJung committed Oct 28, 2023
1 parent f454249 commit fabf9e8
Show file tree
Hide file tree
Showing 9 changed files with 121 additions and 48 deletions.
64 changes: 47 additions & 17 deletions src/concurrency/data_race.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ use rustc_data_structures::fx::{FxHashMap, FxHashSet};
use rustc_index::{Idx, IndexVec};
use rustc_middle::mir;
use rustc_span::Span;
use rustc_target::abi::{Align, Size};
use rustc_target::abi::{Align, HasDataLayout, Size};

use crate::diagnostics::RacingOp;
use crate::*;
Expand Down Expand Up @@ -194,6 +194,13 @@ struct AtomicMemoryCellClocks {
size: Size,
}

#[derive(Copy, Clone, PartialEq, Eq, Debug)]
enum AtomicAccessType {
Load(AtomicReadOrd),
Store,
Rmw,
}

/// Type of write operation: allocating memory
/// non-atomic writes and deallocating memory
/// are all treated as writes for the purpose
Expand Down Expand Up @@ -526,7 +533,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: MiriInterpCxExt<'mir, 'tcx> {
atomic: AtomicReadOrd,
) -> InterpResult<'tcx, Scalar<Provenance>> {
let this = self.eval_context_ref();
this.atomic_access_check(place)?;
this.atomic_access_check(place, AtomicAccessType::Load(atomic))?;
// This will read from the last store in the modification order of this location. In case
// weak memory emulation is enabled, this may not be the store we will pick to actually read from and return.
// This is fine with StackedBorrow and race checks because they don't concern metadata on
Expand All @@ -546,7 +553,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: MiriInterpCxExt<'mir, 'tcx> {
atomic: AtomicWriteOrd,
) -> InterpResult<'tcx> {
let this = self.eval_context_mut();
this.atomic_access_check(dest)?;
this.atomic_access_check(dest, AtomicAccessType::Store)?;

this.allow_data_races_mut(move |this| this.write_scalar(val, dest))?;
this.validate_atomic_store(dest, atomic)?;
Expand All @@ -568,7 +575,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: MiriInterpCxExt<'mir, 'tcx> {
atomic: AtomicRwOrd,
) -> InterpResult<'tcx, ImmTy<'tcx, Provenance>> {
let this = self.eval_context_mut();
this.atomic_access_check(place)?;
this.atomic_access_check(place, AtomicAccessType::Rmw)?;

let old = this.allow_data_races_mut(|this| this.read_immediate(place))?;

Expand All @@ -592,7 +599,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: MiriInterpCxExt<'mir, 'tcx> {
atomic: AtomicRwOrd,
) -> InterpResult<'tcx, Scalar<Provenance>> {
let this = self.eval_context_mut();
this.atomic_access_check(place)?;
this.atomic_access_check(place, AtomicAccessType::Rmw)?;

let old = this.allow_data_races_mut(|this| this.read_scalar(place))?;
this.allow_data_races_mut(|this| this.write_scalar(new, place))?;
Expand All @@ -613,7 +620,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: MiriInterpCxExt<'mir, 'tcx> {
atomic: AtomicRwOrd,
) -> InterpResult<'tcx, ImmTy<'tcx, Provenance>> {
let this = self.eval_context_mut();
this.atomic_access_check(place)?;
this.atomic_access_check(place, AtomicAccessType::Rmw)?;

let old = this.allow_data_races_mut(|this| this.read_immediate(place))?;
let lt = this.wrapping_binary_op(mir::BinOp::Lt, &old, &rhs)?.to_scalar().to_bool()?;
Expand Down Expand Up @@ -652,7 +659,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: MiriInterpCxExt<'mir, 'tcx> {
) -> InterpResult<'tcx, Immediate<Provenance>> {
use rand::Rng as _;
let this = self.eval_context_mut();
this.atomic_access_check(place)?;
this.atomic_access_check(place, AtomicAccessType::Rmw)?;

// Failure ordering cannot be stronger than success ordering, therefore first attempt
// to read with the failure ordering and if successful then try again with the success
Expand Down Expand Up @@ -1062,7 +1069,11 @@ trait EvalContextPrivExt<'mir, 'tcx: 'mir>: MiriInterpCxExt<'mir, 'tcx> {
}

/// Checks that an atomic access is legal at the given place.
fn atomic_access_check(&self, place: &MPlaceTy<'tcx, Provenance>) -> InterpResult<'tcx> {
fn atomic_access_check(
&self,
place: &MPlaceTy<'tcx, Provenance>,
access_type: AtomicAccessType,
) -> InterpResult<'tcx> {
let this = self.eval_context_ref();
// Check alignment requirements. Atomics must always be aligned to their size,
// even if the type they wrap would be less aligned (e.g. AtomicU64 on 32bit must
Expand All @@ -1080,15 +1091,34 @@ trait EvalContextPrivExt<'mir, 'tcx: 'mir>: MiriInterpCxExt<'mir, 'tcx> {
.ptr_try_get_alloc_id(place.ptr())
.expect("there are no zero-sized atomic accesses");
if this.get_alloc_mutability(alloc_id)? == Mutability::Not {
// FIXME: make this prettier, once these messages have separate title/span/help messages.
throw_ub_format!(
"atomic operations cannot be performed on read-only memory\n\
many platforms require atomic read-modify-write instructions to be performed on writeable memory, even if the operation fails \
(and is hence nominally read-only)\n\
some platforms implement (some) atomic loads via compare-exchange, which means they do not work on read-only memory; \
it is possible that we could have an exception permitting this for specific kinds of loads\n\
please report an issue at <https://github.com/rust-lang/miri/issues> if this is a problem for you"
);
// See if this is fine.
match access_type {
AtomicAccessType::Rmw | AtomicAccessType::Store => {
throw_ub_format!(
"atomic store and read-modify-write operations cannot be performed on read-only memory\n\
see <https://doc.rust-lang.org/nightly/std/sync/atomic/index.html#atomic-accesses-to-read-only-memory> for more information"
);
}
AtomicAccessType::Load(_)
if place.layout.size > this.tcx.data_layout().pointer_size() =>
{
throw_ub_format!(
"large atomic load operations cannot be performed on read-only memory\n\
these operations often have to be implemented using read-modify-write operations, which require writeable memory\n\
see <https://doc.rust-lang.org/nightly/std/sync/atomic/index.html#atomic-accesses-to-read-only-memory> for more information"
);
}
AtomicAccessType::Load(o) if o != AtomicReadOrd::Relaxed => {
throw_ub_format!(
"non-relaxed atomic load operations cannot be performed on read-only memory\n\
these operations sometimes have to be implemented using read-modify-write operations, which require writeable memory\n\
see <https://doc.rust-lang.org/nightly/std/sync/atomic/index.html#atomic-accesses-to-read-only-memory> for more information"
);
}
_ => {
// Large relaxed loads are fine!
}
}
}
Ok(())
}
Expand Down
2 changes: 1 addition & 1 deletion tests/fail/concurrency/read_only_atomic_cmpxchg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,5 @@ fn main() {
static X: i32 = 0;
let x = &X as *const i32 as *const AtomicI32;
let x = unsafe { &*x };
x.compare_exchange(1, 2, Ordering::Relaxed, Ordering::Relaxed).unwrap_err(); //~ERROR: atomic operations cannot be performed on read-only memory
x.compare_exchange(1, 2, Ordering::Relaxed, Ordering::Relaxed).unwrap_err(); //~ERROR: cannot be performed on read-only memory
}
12 changes: 4 additions & 8 deletions tests/fail/concurrency/read_only_atomic_cmpxchg.stderr
Original file line number Diff line number Diff line change
@@ -1,14 +1,10 @@
error: Undefined Behavior: atomic operations cannot be performed on read-only memory
many platforms require atomic read-modify-write instructions to be performed on writeable memory, even if the operation fails (and is hence nominally read-only)
some platforms implement (some) atomic loads via compare-exchange, which means they do not work on read-only memory; it is possible that we could have an exception permitting this for specific kinds of loads
please report an issue at <https://github.com/rust-lang/miri/issues> if this is a problem for you
error: Undefined Behavior: atomic store and read-modify-write operations cannot be performed on read-only memory
see <https://doc.rust-lang.org/nightly/std/sync/atomic/index.html#atomic-accesses-to-read-only-memory> for more information
--> $DIR/read_only_atomic_cmpxchg.rs:LL:CC
|
LL | x.compare_exchange(1, 2, Ordering::Relaxed, Ordering::Relaxed).unwrap_err();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ atomic operations cannot be performed on read-only memory
many platforms require atomic read-modify-write instructions to be performed on writeable memory, even if the operation fails (and is hence nominally read-only)
some platforms implement (some) atomic loads via compare-exchange, which means they do not work on read-only memory; it is possible that we could have an exception permitting this for specific kinds of loads
please report an issue at <https://github.com/rust-lang/miri/issues> if this is a problem for you
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ atomic store and read-modify-write operations cannot be performed on read-only memory
see <https://doc.rust-lang.org/nightly/std/sync/atomic/index.html#atomic-accesses-to-read-only-memory> for more information
|
= 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
Expand Down
21 changes: 0 additions & 21 deletions tests/fail/concurrency/read_only_atomic_load.stderr

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -9,5 +9,5 @@ fn main() {
let x = unsafe { &*x };
// Some targets can implement atomic loads via compare_exchange, so we cannot allow them on
// read-only memory.
x.load(Ordering::Relaxed); //~ERROR: atomic operations cannot be performed on read-only memory
x.load(Ordering::Acquire); //~ERROR: cannot be performed on read-only memory
}
19 changes: 19 additions & 0 deletions tests/fail/concurrency/read_only_atomic_load_acquire.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
error: Undefined Behavior: non-relaxed atomic load operations cannot be performed on read-only memory
these operations sometimes have to be implemented using read-modify-write operations, which require writeable memory
see <https://doc.rust-lang.org/nightly/std/sync/atomic/index.html#atomic-accesses-to-read-only-memory> for more information
--> $DIR/read_only_atomic_load_acquire.rs:LL:CC
|
LL | x.load(Ordering::Acquire);
| ^^^^^^^^^^^^^^^^^^^^^^^^^ non-relaxed atomic load operations cannot be performed on read-only memory
these operations sometimes have to be implemented using read-modify-write operations, which require writeable memory
see <https://doc.rust-lang.org/nightly/std/sync/atomic/index.html#atomic-accesses-to-read-only-memory> for more information
|
= 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/read_only_atomic_load_acquire.rs:LL:CC

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

error: aborting due to previous error

18 changes: 18 additions & 0 deletions tests/fail/concurrency/read_only_atomic_load_large.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// Should not rely on the aliasing model for its failure.
//@compile-flags: -Zmiri-disable-stacked-borrows
// Needs atomic accesses larger than the pointer size
//@ignore-64bit

use std::sync::atomic::{AtomicI64, Ordering};

#[repr(align(8))]
struct AlignedI64(i64);

fn main() {
static X: AlignedI64 = AlignedI64(0);
let x = &X as *const AlignedI64 as *const AtomicI64;
let x = unsafe { &*x };
// Some targets can implement atomic loads via compare_exchange, so we cannot allow them on
// read-only memory.
x.load(Ordering::Relaxed); //~ERROR: cannot be performed on read-only memory
}
19 changes: 19 additions & 0 deletions tests/fail/concurrency/read_only_atomic_load_large.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
error: Undefined Behavior: large atomic load operations cannot be performed on read-only memory
these operations often have to be implemented using read-modify-write operations, which require writeable memory
see <https://doc.rust-lang.org/nightly/std/sync/atomic/index.html#atomic-accesses-to-read-only-memory> for more information
--> $DIR/read_only_atomic_load_large.rs:LL:CC
|
LL | x.load(Ordering::Relaxed);
| ^^^^^^^^^^^^^^^^^^^^^^^^^ large atomic load operations cannot be performed on read-only memory
these operations often have to be implemented using read-modify-write operations, which require writeable memory
see <https://doc.rust-lang.org/nightly/std/sync/atomic/index.html#atomic-accesses-to-read-only-memory> for more information
|
= 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/read_only_atomic_load_large.rs:LL:CC

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

error: aborting due to previous error

12 changes: 12 additions & 0 deletions tests/pass/atomic-readonly-load.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// Stacked Borrows doesn't like this.
//@compile-flags: -Zmiri-tree-borrows

use std::sync::atomic::*;

fn main() {
// Atomic loads from read-only memory are fine if they are relaxed and small.
static X: i32 = 0;
let x = &X as *const i32 as *const AtomicI32;
let x = unsafe { &*x };
x.load(Ordering::Relaxed);
}

0 comments on commit fabf9e8

Please sign in to comment.