Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

adjust for earlier init checking in the core engine #2455

Merged
merged 2 commits into from
Aug 27, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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