Auto merge of #3149 - RalfJung:atomic-readonly-loads, r=RalfJung

accept some atomic loads from read-only memory

matches rust-lang/rust#115577

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<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
@@ -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)?;
@@ -558,8 +565,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>,
@@ -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<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))?;
@@ -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<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
@@ -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 <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(())
     }

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(())
             }

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
 }

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 <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

tests/fail/concurrency/read_only_atomic_load.rs → 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
 }

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 <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
+

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
+}

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 <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
+

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);
+}