mikros/rust
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

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

Browse Source 
accept some atomic loads from read-only memory

matches https://github.com/rust-lang/rust/pull/115577
This commit is contained in:
bors 2023-10-28 11:04:43 +00:00
commit 2c9baab7fd
10 changed files with 135 additions and 62 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

68
src/tools/miri/src/concurrency/data_race.rs
View File

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

24
src/tools/miri/src/shims/intrinsics/atomic.rs
View File

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

2
src/tools/miri/tests/fail/concurrency/read_only_atomic_cmpxchg.rs
View File

@ -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
src/tools/miri/tests/fail/concurrency/read_only_atomic_cmpxchg.stderr
View File

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

21
src/tools/miri/tests/fail/concurrency/read_only_atomic_load.stderr
View File

@ -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 <https://github.com/rust-lang/miri/issues> 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 <https://github.com/rust-lang/miri/issues> 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

2
src/tools/miri/tests/fail/concurrency/read_only_atomic_load.rs → src/tools/miri/tests/fail/concurrency/read_only_atomic_load_acquire.rs
View File

@ -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
src/tools/miri/tests/fail/concurrency/read_only_atomic_load_acquire.stderr Normal file
View File

@ -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
src/tools/miri/tests/fail/concurrency/read_only_atomic_load_large.rs Normal file
View File

@ -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
src/tools/miri/tests/fail/concurrency/read_only_atomic_load_large.stderr Normal file
View File

@ -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
src/tools/miri/tests/pass/atomic-readonly-load.rs Normal file
View File

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