From 0b6c30a865231ff6763298f6eefee184f96c5011 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 28 Oct 2023 10:44:55 +0200 Subject: [PATCH 1/2] =?UTF-8?q?atomic=5Fop=20=E2=86=92=20atomic=5Frmw=5Fop?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/tools/miri/src/concurrency/data_race.rs | 4 ++-- src/tools/miri/src/shims/intrinsics/atomic.rs | 24 +++++++++---------- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/src/tools/miri/src/concurrency/data_race.rs b/src/tools/miri/src/concurrency/data_race.rs index 4cab86af8861f..294335f77dcb3 100644 --- a/src/tools/miri/src/concurrency/data_race.rs +++ b/src/tools/miri/src/concurrency/data_race.rs @@ -558,8 +558,8 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: MiriInterpCxExt<'mir, 'tcx> { this.buffered_atomic_write(val, dest, atomic, val) } - /// Perform an atomic operation on a memory location. - fn atomic_op_immediate( + /// Perform an atomic RMW operation on a memory location. + fn atomic_rmw_op_immediate( &mut self, place: &MPlaceTy<'tcx, Provenance>, rhs: &ImmTy<'tcx, Provenance>, diff --git a/src/tools/miri/src/shims/intrinsics/atomic.rs b/src/tools/miri/src/shims/intrinsics/atomic.rs index e38b677f485f3..4d7f6a6b4e0e2 100644 --- a/src/tools/miri/src/shims/intrinsics/atomic.rs +++ b/src/tools/miri/src/shims/intrinsics/atomic.rs @@ -77,40 +77,40 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { this.atomic_compare_exchange_weak(args, dest, rw_ord(ord1)?, read_ord(ord2)?)?, ["or", ord] => - this.atomic_op(args, dest, AtomicOp::MirOp(BinOp::BitOr, false), rw_ord(ord)?)?, + this.atomic_rmw_op(args, dest, AtomicOp::MirOp(BinOp::BitOr, false), rw_ord(ord)?)?, ["xor", ord] => - this.atomic_op(args, dest, AtomicOp::MirOp(BinOp::BitXor, false), rw_ord(ord)?)?, + this.atomic_rmw_op(args, dest, AtomicOp::MirOp(BinOp::BitXor, false), rw_ord(ord)?)?, ["and", ord] => - this.atomic_op(args, dest, AtomicOp::MirOp(BinOp::BitAnd, false), rw_ord(ord)?)?, + this.atomic_rmw_op(args, dest, AtomicOp::MirOp(BinOp::BitAnd, false), rw_ord(ord)?)?, ["nand", ord] => - this.atomic_op(args, dest, AtomicOp::MirOp(BinOp::BitAnd, true), rw_ord(ord)?)?, + this.atomic_rmw_op(args, dest, AtomicOp::MirOp(BinOp::BitAnd, true), rw_ord(ord)?)?, ["xadd", ord] => - this.atomic_op(args, dest, AtomicOp::MirOp(BinOp::Add, false), rw_ord(ord)?)?, + this.atomic_rmw_op(args, dest, AtomicOp::MirOp(BinOp::Add, false), rw_ord(ord)?)?, ["xsub", ord] => - this.atomic_op(args, dest, AtomicOp::MirOp(BinOp::Sub, false), rw_ord(ord)?)?, + this.atomic_rmw_op(args, dest, AtomicOp::MirOp(BinOp::Sub, false), rw_ord(ord)?)?, ["min", ord] => { // Later we will use the type to indicate signed vs unsigned, // so make sure it matches the intrinsic name. assert!(matches!(args[1].layout.ty.kind(), ty::Int(_))); - this.atomic_op(args, dest, AtomicOp::Min, rw_ord(ord)?)?; + this.atomic_rmw_op(args, dest, AtomicOp::Min, rw_ord(ord)?)?; } ["umin", ord] => { // Later we will use the type to indicate signed vs unsigned, // so make sure it matches the intrinsic name. assert!(matches!(args[1].layout.ty.kind(), ty::Uint(_))); - this.atomic_op(args, dest, AtomicOp::Min, rw_ord(ord)?)?; + this.atomic_rmw_op(args, dest, AtomicOp::Min, rw_ord(ord)?)?; } ["max", ord] => { // Later we will use the type to indicate signed vs unsigned, // so make sure it matches the intrinsic name. assert!(matches!(args[1].layout.ty.kind(), ty::Int(_))); - this.atomic_op(args, dest, AtomicOp::Max, rw_ord(ord)?)?; + this.atomic_rmw_op(args, dest, AtomicOp::Max, rw_ord(ord)?)?; } ["umax", ord] => { // Later we will use the type to indicate signed vs unsigned, // so make sure it matches the intrinsic name. assert!(matches!(args[1].layout.ty.kind(), ty::Uint(_))); - this.atomic_op(args, dest, AtomicOp::Max, rw_ord(ord)?)?; + this.atomic_rmw_op(args, dest, AtomicOp::Max, rw_ord(ord)?)?; } _ => throw_unsup_format!("unimplemented intrinsic: `atomic_{intrinsic_name}`"), @@ -178,7 +178,7 @@ trait EvalContextPrivExt<'mir, 'tcx: 'mir>: MiriInterpCxExt<'mir, 'tcx> { Ok(()) } - fn atomic_op( + fn atomic_rmw_op( &mut self, args: &[OpTy<'tcx, Provenance>], dest: &PlaceTy<'tcx, Provenance>, @@ -213,7 +213,7 @@ trait EvalContextPrivExt<'mir, 'tcx: 'mir>: MiriInterpCxExt<'mir, 'tcx> { Ok(()) } AtomicOp::MirOp(op, neg) => { - let old = this.atomic_op_immediate(&place, &rhs, op, neg, atomic)?; + let old = this.atomic_rmw_op_immediate(&place, &rhs, op, neg, atomic)?; this.write_immediate(*old, dest)?; // old value is returned Ok(()) } From bd81a5866d67ae56f85f3230a6ccd8435a8a7d26 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 28 Oct 2023 11:11:09 +0200 Subject: [PATCH 2/2] accept some atomic loads from read-only memory --- src/tools/miri/src/concurrency/data_race.rs | 64 ++++++++++++++----- .../concurrency/read_only_atomic_cmpxchg.rs | 2 +- .../read_only_atomic_cmpxchg.stderr | 12 ++-- .../concurrency/read_only_atomic_load.stderr | 21 ------ ...ad.rs => read_only_atomic_load_acquire.rs} | 2 +- .../read_only_atomic_load_acquire.stderr | 19 ++++++ .../read_only_atomic_load_large.rs | 18 ++++++ .../read_only_atomic_load_large.stderr | 19 ++++++ .../miri/tests/pass/atomic-readonly-load.rs | 12 ++++ 9 files changed, 121 insertions(+), 48 deletions(-) delete mode 100644 src/tools/miri/tests/fail/concurrency/read_only_atomic_load.stderr rename src/tools/miri/tests/fail/concurrency/{read_only_atomic_load.rs => read_only_atomic_load_acquire.rs} (79%) create mode 100644 src/tools/miri/tests/fail/concurrency/read_only_atomic_load_acquire.stderr create mode 100644 src/tools/miri/tests/fail/concurrency/read_only_atomic_load_large.rs create mode 100644 src/tools/miri/tests/fail/concurrency/read_only_atomic_load_large.stderr create mode 100644 src/tools/miri/tests/pass/atomic-readonly-load.rs diff --git a/src/tools/miri/src/concurrency/data_race.rs b/src/tools/miri/src/concurrency/data_race.rs index 294335f77dcb3..f0220d038ae5b 100644 --- a/src/tools/miri/src/concurrency/data_race.rs +++ b/src/tools/miri/src/concurrency/data_race.rs @@ -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::*; @@ -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 @@ -526,7 +533,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: MiriInterpCxExt<'mir, 'tcx> { atomic: AtomicReadOrd, ) -> InterpResult<'tcx, Scalar> { 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 @@ -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)?; @@ -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))?; @@ -592,7 +599,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: MiriInterpCxExt<'mir, 'tcx> { atomic: AtomicRwOrd, ) -> InterpResult<'tcx, Scalar> { 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))?; @@ -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()?; @@ -652,7 +659,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: MiriInterpCxExt<'mir, 'tcx> { ) -> InterpResult<'tcx, Immediate> { 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 @@ -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 @@ -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 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 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 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 for more information" + ); + } + _ => { + // Large relaxed loads are fine! + } + } } Ok(()) } diff --git a/src/tools/miri/tests/fail/concurrency/read_only_atomic_cmpxchg.rs b/src/tools/miri/tests/fail/concurrency/read_only_atomic_cmpxchg.rs index cb6aeea665d39..88c73d14ef72b 100644 --- a/src/tools/miri/tests/fail/concurrency/read_only_atomic_cmpxchg.rs +++ b/src/tools/miri/tests/fail/concurrency/read_only_atomic_cmpxchg.rs @@ -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 } diff --git a/src/tools/miri/tests/fail/concurrency/read_only_atomic_cmpxchg.stderr b/src/tools/miri/tests/fail/concurrency/read_only_atomic_cmpxchg.stderr index d51fdee0b256f..fc5982e7f94f9 100644 --- a/src/tools/miri/tests/fail/concurrency/read_only_atomic_cmpxchg.stderr +++ b/src/tools/miri/tests/fail/concurrency/read_only_atomic_cmpxchg.stderr @@ -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 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 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 if this is a problem for you + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ atomic store and read-modify-write operations cannot be performed on read-only memory +see 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 diff --git a/src/tools/miri/tests/fail/concurrency/read_only_atomic_load.stderr b/src/tools/miri/tests/fail/concurrency/read_only_atomic_load.stderr deleted file mode 100644 index 17851d6b470b4..0000000000000 --- a/src/tools/miri/tests/fail/concurrency/read_only_atomic_load.stderr +++ /dev/null @@ -1,21 +0,0 @@ -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 if this is a problem for you - --> $DIR/read_only_atomic_load.rs:LL:CC - | -LL | x.load(Ordering::Relaxed); - | ^^^^^^^^^^^^^^^^^^^^^^^^^ 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 if this is a problem for you - | - = 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.rs:LL:CC - -note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace - -error: aborting due to previous error - diff --git a/src/tools/miri/tests/fail/concurrency/read_only_atomic_load.rs b/src/tools/miri/tests/fail/concurrency/read_only_atomic_load_acquire.rs similarity index 79% rename from src/tools/miri/tests/fail/concurrency/read_only_atomic_load.rs rename to src/tools/miri/tests/fail/concurrency/read_only_atomic_load_acquire.rs index 6e92453e3c195..af0dc2d3fd64a 100644 --- a/src/tools/miri/tests/fail/concurrency/read_only_atomic_load.rs +++ b/src/tools/miri/tests/fail/concurrency/read_only_atomic_load_acquire.rs @@ -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 } diff --git a/src/tools/miri/tests/fail/concurrency/read_only_atomic_load_acquire.stderr b/src/tools/miri/tests/fail/concurrency/read_only_atomic_load_acquire.stderr new file mode 100644 index 0000000000000..2945344877a5b --- /dev/null +++ b/src/tools/miri/tests/fail/concurrency/read_only_atomic_load_acquire.stderr @@ -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 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 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 + diff --git a/src/tools/miri/tests/fail/concurrency/read_only_atomic_load_large.rs b/src/tools/miri/tests/fail/concurrency/read_only_atomic_load_large.rs new file mode 100644 index 0000000000000..a9a8f0f5dddda --- /dev/null +++ b/src/tools/miri/tests/fail/concurrency/read_only_atomic_load_large.rs @@ -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 +} diff --git a/src/tools/miri/tests/fail/concurrency/read_only_atomic_load_large.stderr b/src/tools/miri/tests/fail/concurrency/read_only_atomic_load_large.stderr new file mode 100644 index 0000000000000..5d8cb707f3fea --- /dev/null +++ b/src/tools/miri/tests/fail/concurrency/read_only_atomic_load_large.stderr @@ -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 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 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 + diff --git a/src/tools/miri/tests/pass/atomic-readonly-load.rs b/src/tools/miri/tests/pass/atomic-readonly-load.rs new file mode 100644 index 0000000000000..8f8086b3538c6 --- /dev/null +++ b/src/tools/miri/tests/pass/atomic-readonly-load.rs @@ -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); +}