Skip to content

Commit

Permalink
Auto merge of #2455 - RalfJung:scalar-always-init, r=RalfJung
Browse files Browse the repository at this point in the history
adjust for earlier init checking in the core engine

Miri side of rust-lang/rust#100043
  • Loading branch information
bors committed Aug 27, 2022
2 parents 101c4f2 + df19b85 commit bb82124
Show file tree
Hide file tree
Showing 39 changed files with 178 additions and 247 deletions.
2 changes: 1 addition & 1 deletion rust-version
Original file line number Diff line number Diff line change
@@ -1 +1 @@
e1b28cd2f16bd5b832183d7968cae3bb9213e78d
4065b89b1e7287047d7d6c65e7abd7b8ee70bcf0
41 changes: 14 additions & 27 deletions src/concurrency/data_race.rs
Original file line number Diff line number Diff line change
Expand Up @@ -447,7 +447,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: MiriEvalContextExt<'mir, 'tcx> {
offset: u64,
layout: TyAndLayout<'tcx>,
atomic: AtomicReadOrd,
) -> InterpResult<'tcx, ScalarMaybeUninit<Provenance>> {
) -> InterpResult<'tcx, Scalar<Provenance>> {
let this = self.eval_context_ref();
let value_place = this.deref_operand_and_offset(op, offset, layout)?;
this.read_scalar_atomic(&value_place, atomic)
Expand All @@ -458,7 +458,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: MiriEvalContextExt<'mir, 'tcx> {
&mut self,
op: &OpTy<'tcx, Provenance>,
offset: u64,
value: impl Into<ScalarMaybeUninit<Provenance>>,
value: impl Into<Scalar<Provenance>>,
layout: TyAndLayout<'tcx>,
atomic: AtomicWriteOrd,
) -> InterpResult<'tcx> {
Expand All @@ -472,7 +472,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: MiriEvalContextExt<'mir, 'tcx> {
&self,
place: &MPlaceTy<'tcx, Provenance>,
atomic: AtomicReadOrd,
) -> InterpResult<'tcx, ScalarMaybeUninit<Provenance>> {
) -> InterpResult<'tcx, Scalar<Provenance>> {
let this = self.eval_context_ref();
this.atomic_access_check(place)?;
// This will read from the last store in the modification order of this location. In case
Expand All @@ -490,7 +490,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: MiriEvalContextExt<'mir, 'tcx> {
/// Perform an atomic write operation at the memory location.
fn write_scalar_atomic(
&mut self,
val: ScalarMaybeUninit<Provenance>,
val: Scalar<Provenance>,
dest: &MPlaceTy<'tcx, Provenance>,
atomic: AtomicWriteOrd,
) -> InterpResult<'tcx> {
Expand Down Expand Up @@ -530,12 +530,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: MiriEvalContextExt<'mir, 'tcx> {

this.validate_atomic_rmw(place, atomic)?;

this.buffered_atomic_rmw(
val.to_scalar_or_uninit(),
place,
atomic,
old.to_scalar_or_uninit(),
)?;
this.buffered_atomic_rmw(val.to_scalar(), place, atomic, old.to_scalar())?;
Ok(old)
}

Expand All @@ -544,9 +539,9 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: MiriEvalContextExt<'mir, 'tcx> {
fn atomic_exchange_scalar(
&mut self,
place: &MPlaceTy<'tcx, Provenance>,
new: ScalarMaybeUninit<Provenance>,
new: Scalar<Provenance>,
atomic: AtomicRwOrd,
) -> InterpResult<'tcx, ScalarMaybeUninit<Provenance>> {
) -> InterpResult<'tcx, Scalar<Provenance>> {
let this = self.eval_context_mut();
this.atomic_access_check(place)?;

Expand Down Expand Up @@ -574,7 +569,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: MiriEvalContextExt<'mir, 'tcx> {

this.validate_overlapping_atomic(place)?;
let old = this.allow_data_races_mut(|this| this.read_immediate(&place.into()))?;
let lt = this.binary_op(mir::BinOp::Lt, &old, &rhs)?.to_scalar()?.to_bool()?;
let lt = this.binary_op(mir::BinOp::Lt, &old, &rhs)?.to_scalar().to_bool()?;

let new_val = if min {
if lt { &old } else { &rhs }
Expand All @@ -586,12 +581,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: MiriEvalContextExt<'mir, 'tcx> {

this.validate_atomic_rmw(place, atomic)?;

this.buffered_atomic_rmw(
new_val.to_scalar_or_uninit(),
place,
atomic,
old.to_scalar_or_uninit(),
)?;
this.buffered_atomic_rmw(new_val.to_scalar(), place, atomic, old.to_scalar())?;

// Return the old value.
Ok(old)
Expand All @@ -607,7 +597,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: MiriEvalContextExt<'mir, 'tcx> {
&mut self,
place: &MPlaceTy<'tcx, Provenance>,
expect_old: &ImmTy<'tcx, Provenance>,
new: ScalarMaybeUninit<Provenance>,
new: Scalar<Provenance>,
success: AtomicRwOrd,
fail: AtomicReadOrd,
can_fail_spuriously: bool,
Expand All @@ -627,31 +617,28 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: MiriEvalContextExt<'mir, 'tcx> {
// If the operation would succeed, but is "weak", fail some portion
// of the time, based on `success_rate`.
let success_rate = 1.0 - this.machine.cmpxchg_weak_failure_rate;
let cmpxchg_success = eq.to_scalar()?.to_bool()?
let cmpxchg_success = eq.to_scalar().to_bool()?
&& if can_fail_spuriously {
this.machine.rng.get_mut().gen_bool(success_rate)
} else {
true
};
let res = Immediate::ScalarPair(
old.to_scalar_or_uninit(),
Scalar::from_bool(cmpxchg_success).into(),
);
let res = Immediate::ScalarPair(old.to_scalar(), Scalar::from_bool(cmpxchg_success));

// Update ptr depending on comparison.
// if successful, perform a full rw-atomic validation
// otherwise treat this as an atomic load with the fail ordering.
if cmpxchg_success {
this.allow_data_races_mut(|this| this.write_scalar(new, &place.into()))?;
this.validate_atomic_rmw(place, success)?;
this.buffered_atomic_rmw(new, place, success, old.to_scalar_or_uninit())?;
this.buffered_atomic_rmw(new, place, success, old.to_scalar())?;
} else {
this.validate_atomic_load(place, fail)?;
// A failed compare exchange is equivalent to a load, reading from the latest store
// in the modification order.
// Since `old` is only a value and not the store element, we need to separately
// find it in our store buffer and perform load_impl on it.
this.perform_read_on_buffered_latest(place, fail, old.to_scalar_or_uninit())?;
this.perform_read_on_buffered_latest(place, fail, old.to_scalar())?;
}

// Return the old value.
Expand Down
42 changes: 18 additions & 24 deletions src/concurrency/weak_memory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -77,9 +77,7 @@ use std::{
collections::VecDeque,
};

use rustc_const_eval::interpret::{
alloc_range, AllocRange, InterpResult, MPlaceTy, ScalarMaybeUninit,
};
use rustc_const_eval::interpret::{alloc_range, AllocRange, InterpResult, MPlaceTy, Scalar};
use rustc_data_structures::fx::FxHashMap;

use crate::*;
Expand Down Expand Up @@ -130,10 +128,10 @@ struct StoreElement {
/// The timestamp of the storing thread when it performed the store
timestamp: VTimestamp,
/// The value of this store
// FIXME: this means the store is either fully initialized or fully uninitialized;
// FIXME: this means the store must be fully initialized;
// we will have to change this if we want to support atomics on
// partially initialized data.
val: ScalarMaybeUninit<Provenance>,
// (partially) uninitialized data.
val: Scalar<Provenance>,

/// Timestamp of first loads from this store element by each thread
/// Behind a RefCell to keep load op take &self
Expand Down Expand Up @@ -180,7 +178,7 @@ impl StoreBufferAlloc {
fn get_or_create_store_buffer<'tcx>(
&self,
range: AllocRange,
init: ScalarMaybeUninit<Provenance>,
init: Scalar<Provenance>,
) -> InterpResult<'tcx, Ref<'_, StoreBuffer>> {
let access_type = self.store_buffers.borrow().access_type(range);
let pos = match access_type {
Expand All @@ -205,7 +203,7 @@ impl StoreBufferAlloc {
fn get_or_create_store_buffer_mut<'tcx>(
&mut self,
range: AllocRange,
init: ScalarMaybeUninit<Provenance>,
init: Scalar<Provenance>,
) -> InterpResult<'tcx, &mut StoreBuffer> {
let buffers = self.store_buffers.get_mut();
let access_type = buffers.access_type(range);
Expand All @@ -226,7 +224,7 @@ impl StoreBufferAlloc {
}

impl<'mir, 'tcx: 'mir> StoreBuffer {
fn new(init: ScalarMaybeUninit<Provenance>) -> Self {
fn new(init: Scalar<Provenance>) -> Self {
let mut buffer = VecDeque::new();
buffer.reserve(STORE_BUFFER_LIMIT);
let mut ret = Self { buffer };
Expand Down Expand Up @@ -259,7 +257,7 @@ impl<'mir, 'tcx: 'mir> StoreBuffer {
is_seqcst: bool,
rng: &mut (impl rand::Rng + ?Sized),
validate: impl FnOnce() -> InterpResult<'tcx>,
) -> InterpResult<'tcx, (ScalarMaybeUninit<Provenance>, LoadRecency)> {
) -> InterpResult<'tcx, (Scalar<Provenance>, LoadRecency)> {
// Having a live borrow to store_buffer while calling validate_atomic_load is fine
// because the race detector doesn't touch store_buffer

Expand All @@ -284,7 +282,7 @@ impl<'mir, 'tcx: 'mir> StoreBuffer {

fn buffered_write(
&mut self,
val: ScalarMaybeUninit<Provenance>,
val: Scalar<Provenance>,
global: &DataRaceState,
thread_mgr: &ThreadManager<'_, '_>,
is_seqcst: bool,
Expand Down Expand Up @@ -375,7 +373,7 @@ impl<'mir, 'tcx: 'mir> StoreBuffer {
/// ATOMIC STORE IMPL in the paper (except we don't need the location's vector clock)
fn store_impl(
&mut self,
val: ScalarMaybeUninit<Provenance>,
val: Scalar<Provenance>,
index: VectorIdx,
thread_clock: &VClock,
is_seqcst: bool,
Expand Down Expand Up @@ -417,11 +415,7 @@ impl StoreElement {
/// buffer regardless of subsequent loads by the same thread; if the earliest load of another
/// thread doesn't happen before the current one, then no subsequent load by the other thread
/// can happen before the current one.
fn load_impl(
&self,
index: VectorIdx,
clocks: &ThreadClockSet,
) -> ScalarMaybeUninit<Provenance> {
fn load_impl(&self, index: VectorIdx, clocks: &ThreadClockSet) -> Scalar<Provenance> {
let _ = self.loads.borrow_mut().try_insert(index, clocks.clock[index]);
self.val
}
Expand Down Expand Up @@ -464,10 +458,10 @@ pub(super) trait EvalContextExt<'mir, 'tcx: 'mir>:

fn buffered_atomic_rmw(
&mut self,
new_val: ScalarMaybeUninit<Provenance>,
new_val: Scalar<Provenance>,
place: &MPlaceTy<'tcx, Provenance>,
atomic: AtomicRwOrd,
init: ScalarMaybeUninit<Provenance>,
init: Scalar<Provenance>,
) -> InterpResult<'tcx> {
let this = self.eval_context_mut();
let (alloc_id, base_offset, ..) = this.ptr_get_alloc_id(place.ptr)?;
Expand All @@ -492,9 +486,9 @@ pub(super) trait EvalContextExt<'mir, 'tcx: 'mir>:
&self,
place: &MPlaceTy<'tcx, Provenance>,
atomic: AtomicReadOrd,
latest_in_mo: ScalarMaybeUninit<Provenance>,
latest_in_mo: Scalar<Provenance>,
validate: impl FnOnce() -> InterpResult<'tcx>,
) -> InterpResult<'tcx, ScalarMaybeUninit<Provenance>> {
) -> InterpResult<'tcx, Scalar<Provenance>> {
let this = self.eval_context_ref();
if let Some(global) = &this.machine.data_race {
let (alloc_id, base_offset, ..) = this.ptr_get_alloc_id(place.ptr)?;
Expand Down Expand Up @@ -529,10 +523,10 @@ pub(super) trait EvalContextExt<'mir, 'tcx: 'mir>:

fn buffered_atomic_write(
&mut self,
val: ScalarMaybeUninit<Provenance>,
val: Scalar<Provenance>,
dest: &MPlaceTy<'tcx, Provenance>,
atomic: AtomicWriteOrd,
init: ScalarMaybeUninit<Provenance>,
init: Scalar<Provenance>,
) -> InterpResult<'tcx> {
let this = self.eval_context_mut();
let (alloc_id, base_offset, ..) = this.ptr_get_alloc_id(dest.ptr)?;
Expand Down Expand Up @@ -576,7 +570,7 @@ pub(super) trait EvalContextExt<'mir, 'tcx: 'mir>:
&self,
place: &MPlaceTy<'tcx, Provenance>,
atomic: AtomicReadOrd,
init: ScalarMaybeUninit<Provenance>,
init: Scalar<Provenance>,
) -> InterpResult<'tcx> {
let this = self.eval_context_ref();

Expand Down
24 changes: 16 additions & 8 deletions src/helpers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -116,8 +116,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
let instance = this.resolve_path(path);
let cid = GlobalId { instance, promoted: None };
let const_val = this.eval_to_allocation(cid)?;
let const_val = this.read_scalar(&const_val.into())?;
const_val.check_init()
this.read_scalar(&const_val.into())
}

/// Helper function to get a `libc` constant as a `Scalar`.
Expand Down Expand Up @@ -567,7 +566,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
fn get_last_error(&mut self) -> InterpResult<'tcx, Scalar<Provenance>> {
let this = self.eval_context_mut();
let errno_place = this.last_error_place()?;
this.read_scalar(&errno_place.into())?.check_init()
this.read_scalar(&errno_place.into())
}

/// This function tries to produce the most similar OS error from the `std::io::ErrorKind`
Expand Down Expand Up @@ -680,22 +679,31 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
op: &OpTy<'tcx, Provenance>,
offset: u64,
layout: TyAndLayout<'tcx>,
) -> InterpResult<'tcx, ScalarMaybeUninit<Provenance>> {
) -> InterpResult<'tcx, Scalar<Provenance>> {
let this = self.eval_context_ref();
let value_place = this.deref_operand_and_offset(op, offset, layout)?;
this.read_scalar(&value_place.into())
}

fn write_immediate_at_offset(
&mut self,
op: &OpTy<'tcx, Provenance>,
offset: u64,
value: &ImmTy<'tcx, Provenance>,
) -> InterpResult<'tcx, ()> {
let this = self.eval_context_mut();
let value_place = this.deref_operand_and_offset(op, offset, value.layout)?;
this.write_immediate(**value, &value_place.into())
}

fn write_scalar_at_offset(
&mut self,
op: &OpTy<'tcx, Provenance>,
offset: u64,
value: impl Into<ScalarMaybeUninit<Provenance>>,
value: impl Into<Scalar<Provenance>>,
layout: TyAndLayout<'tcx>,
) -> InterpResult<'tcx, ()> {
let this = self.eval_context_mut();
let value_place = this.deref_operand_and_offset(op, offset, layout)?;
this.write_scalar(value, &value_place.into())
self.write_immediate_at_offset(op, offset, &ImmTy::from_scalar(value.into(), layout))
}

/// Parse a `timespec` struct and return it as a `std::time::Duration`. It returns `None`
Expand Down
9 changes: 2 additions & 7 deletions src/machine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ static_assert_size!(Pointer<Provenance>, 24);
// #[cfg(all(target_arch = "x86_64", target_pointer_width = "64"))]
//static_assert_size!(Pointer<Option<Provenance>>, 24);
#[cfg(all(target_arch = "x86_64", target_pointer_width = "64"))]
static_assert_size!(ScalarMaybeUninit<Provenance>, 32);
static_assert_size!(Scalar<Provenance>, 32);

impl interpret::Provenance for Provenance {
/// We use absolute addresses in the `offset` of a `Pointer<Provenance>`.
Expand Down Expand Up @@ -581,7 +581,7 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'mir, 'tcx> {
}

#[inline(always)]
fn force_int_for_alignment_check(ecx: &MiriEvalContext<'mir, 'tcx>) -> bool {
fn use_addr_for_alignment_check(ecx: &MiriEvalContext<'mir, 'tcx>) -> bool {
ecx.machine.check_alignment == AlignmentCheck::Int
}

Expand All @@ -590,11 +590,6 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'mir, 'tcx> {
ecx.machine.validate
}

#[inline(always)]
fn enforce_number_init(_ecx: &MiriEvalContext<'mir, 'tcx>) -> bool {
true
}

#[inline(always)]
fn enforce_abi(ecx: &MiriEvalContext<'mir, 'tcx>) -> bool {
ecx.machine.enforce_abi
Expand Down
Loading

0 comments on commit bb82124

Please sign in to comment.