Skip to content

adjust Miri to Pointer type overhaul #1851

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

Merged
merged 3 commits into from
Jul 17, 2021
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 @@
3982eb35cabe3a99194d768d34a92347967c3fa2
c78ebb7bdcfc924a20fd069891ffe1364d6814e7
14 changes: 10 additions & 4 deletions src/bin/miri.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ extern crate rustc_session;

use std::convert::TryFrom;
use std::env;
use std::num::NonZeroU64;
use std::path::PathBuf;
use std::rc::Rc;
use std::str::FromStr;
Expand Down Expand Up @@ -412,11 +413,16 @@ fn main() {
}
}
arg if arg.starts_with("-Zmiri-track-alloc-id=") => {
let id: u64 = match arg.strip_prefix("-Zmiri-track-alloc-id=").unwrap().parse()
let id = match arg
.strip_prefix("-Zmiri-track-alloc-id=")
.unwrap()
.parse()
.ok()
.and_then(NonZeroU64::new)
{
Ok(id) => id,
Err(err) =>
panic!("-Zmiri-track-alloc-id requires a valid `u64` argument: {}", err),
Some(id) => id,
None =>
panic!("-Zmiri-track-alloc-id requires a valid non-zero `u64` argument"),
};
miri_config.tracked_alloc_id = Some(miri::AllocId(id));
}
Expand Down
105 changes: 39 additions & 66 deletions src/data_race.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,9 +73,9 @@ use rustc_middle::{mir, ty::layout::TyAndLayout};
use rustc_target::abi::Size;

use crate::{
ImmTy, Immediate, InterpResult, MPlaceTy, MemPlaceMeta, MemoryKind, MiriEvalContext,
MiriEvalContextExt, MiriMemoryKind, OpTy, Pointer, RangeMap, Scalar, ScalarMaybeUninit, Tag,
ThreadId, VClock, VTimestamp, VectorIdx,
AllocId, AllocRange, ImmTy, Immediate, InterpResult, MPlaceTy, MemPlaceMeta, MemoryKind,
MiriEvalContext, MiriEvalContextExt, MiriMemoryKind, OpTy, Pointer, RangeMap, Scalar,
ScalarMaybeUninit, Tag, ThreadId, VClock, VTimestamp, VectorIdx,
};

pub type AllocExtra = VClockAlloc;
Expand Down Expand Up @@ -561,7 +561,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: MiriEvalContextExt<'mir, 'tcx> {
if lt { &rhs } else { &old }
};

this.allow_data_races_mut(|this| this.write_immediate_to_mplace(**new_val, place))?;
this.allow_data_races_mut(|this| this.write_immediate(**new_val, &(*place).into()))?;

this.validate_atomic_rmw(&place, atomic)?;

Expand Down Expand Up @@ -713,18 +713,6 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: MiriEvalContextExt<'mir, 'tcx> {
Ok(())
}
}

fn reset_vector_clocks(&mut self, ptr: Pointer<Tag>, size: Size) -> InterpResult<'tcx> {
let this = self.eval_context_mut();
if let Some(data_race) = &mut this.memory.extra.data_race {
if data_race.multi_threaded.get() {
let alloc_meta =
this.memory.get_alloc_extra_mut(ptr.alloc_id)?.0.data_race.as_mut().unwrap();
alloc_meta.reset_clocks(ptr.offset, size);
}
}
Ok(())
}
}

/// Vector clock metadata for a logical memory allocation.
Expand Down Expand Up @@ -769,14 +757,6 @@ impl VClockAlloc {
}
}

fn reset_clocks(&mut self, offset: Size, len: Size) {
let alloc_ranges = self.alloc_ranges.get_mut();
for (_, range) in alloc_ranges.iter_mut(offset, len) {
// Reset the portion of the range
*range = MemoryCellClocks::new(0, VectorIdx::MAX_INDEX);
}
}

// Find an index, if one exists where the value
// in `l` is greater than the value in `r`.
fn find_gt_index(l: &VClock, r: &VClock) -> Option<VectorIdx> {
Expand Down Expand Up @@ -820,8 +800,7 @@ impl VClockAlloc {
range: &MemoryCellClocks,
action: &str,
is_atomic: bool,
pointer: Pointer<Tag>,
len: Size,
ptr_dbg: Pointer<AllocId>,
) -> InterpResult<'tcx> {
let (current_index, current_clocks) = global.current_thread_state();
let write_clock;
Expand Down Expand Up @@ -863,15 +842,12 @@ impl VClockAlloc {

// Throw the data-race detection.
throw_ub_format!(
"Data race detected between {} on {} and {} on {}, memory({:?},offset={},size={})\
\n(current vector clock = {:?}, conflicting timestamp = {:?})",
"Data race detected between {} on {} and {} on {} at {:?} (current vector clock = {:?}, conflicting timestamp = {:?})",
action,
current_thread_info,
other_action,
other_thread_info,
pointer.alloc_id,
pointer.offset.bytes(),
len.bytes(),
ptr_dbg,
current_clocks.clock,
other_clock
)
Expand All @@ -884,17 +860,23 @@ impl VClockAlloc {
/// atomic read operations.
pub fn read<'tcx>(
&self,
pointer: Pointer<Tag>,
len: Size,
alloc_id: AllocId,
range: AllocRange,
global: &GlobalState,
) -> InterpResult<'tcx> {
if global.multi_threaded.get() {
let (index, clocks) = global.current_thread_state();
let mut alloc_ranges = self.alloc_ranges.borrow_mut();
for (_, range) in alloc_ranges.iter_mut(pointer.offset, len) {
for (offset, range) in alloc_ranges.iter_mut(range.start, range.size) {
if let Err(DataRace) = range.read_race_detect(&*clocks, index) {
// Report data-race.
return Self::report_data_race(global, range, "Read", false, pointer, len);
return Self::report_data_race(
global,
range,
"Read",
false,
Pointer::new(alloc_id, offset),
);
}
}
Ok(())
Expand All @@ -906,23 +888,22 @@ impl VClockAlloc {
// Shared code for detecting data-races on unique access to a section of memory
fn unique_access<'tcx>(
&mut self,
pointer: Pointer<Tag>,
len: Size,
alloc_id: AllocId,
range: AllocRange,
write_type: WriteType,
global: &mut GlobalState,
) -> InterpResult<'tcx> {
if global.multi_threaded.get() {
let (index, clocks) = global.current_thread_state();
for (_, range) in self.alloc_ranges.get_mut().iter_mut(pointer.offset, len) {
for (offset, range) in self.alloc_ranges.get_mut().iter_mut(range.start, range.size) {
if let Err(DataRace) = range.write_race_detect(&*clocks, index, write_type) {
// Report data-race
return Self::report_data_race(
global,
range,
write_type.get_descriptor(),
false,
pointer,
len,
Pointer::new(alloc_id, offset),
);
}
}
Expand All @@ -938,11 +919,11 @@ impl VClockAlloc {
/// operation
pub fn write<'tcx>(
&mut self,
pointer: Pointer<Tag>,
len: Size,
alloc_id: AllocId,
range: AllocRange,
global: &mut GlobalState,
) -> InterpResult<'tcx> {
self.unique_access(pointer, len, WriteType::Write, global)
self.unique_access(alloc_id, range, WriteType::Write, global)
}

/// Detect data-races for an unsynchronized deallocate operation, will not perform
Expand All @@ -951,11 +932,11 @@ impl VClockAlloc {
/// operation
pub fn deallocate<'tcx>(
&mut self,
pointer: Pointer<Tag>,
len: Size,
alloc_id: AllocId,
range: AllocRange,
global: &mut GlobalState,
) -> InterpResult<'tcx> {
self.unique_access(pointer, len, WriteType::Deallocate, global)
self.unique_access(alloc_id, range, WriteType::Deallocate, global)
}
}

Expand Down Expand Up @@ -1002,12 +983,7 @@ trait EvalContextPrivExt<'mir, 'tcx: 'mir>: MiriEvalContextExt<'mir, 'tcx> {
result
}

/// Generic atomic operation implementation,
/// this accesses memory via get_raw instead of
/// get_raw_mut, due to issues calling get_raw_mut
/// for atomic loads from read-only memory.
/// FIXME: is this valid, or should get_raw_mut be used for
/// atomic-stores/atomic-rmw?
/// Generic atomic operation implementation
fn validate_atomic_op<A: Debug + Copy>(
&self,
place: &MPlaceTy<'tcx, Tag>,
Expand All @@ -1023,25 +999,24 @@ trait EvalContextPrivExt<'mir, 'tcx: 'mir>: MiriEvalContextExt<'mir, 'tcx> {
let this = self.eval_context_ref();
if let Some(data_race) = &this.memory.extra.data_race {
if data_race.multi_threaded.get() {
let size = place.layout.size;
let (alloc_id, base_offset, ptr) = this.memory.ptr_get_alloc(place.ptr)?;
// Load and log the atomic operation.
// Note that atomic loads are possible even from read-only allocations, so `get_alloc_extra_mut` is not an option.
let place_ptr = place.ptr.assert_ptr();
let size = place.layout.size;
let alloc_meta =
&this.memory.get_alloc_extra(place_ptr.alloc_id)?.data_race.as_ref().unwrap();
&this.memory.get_alloc_extra(alloc_id)?.data_race.as_ref().unwrap();
log::trace!(
"Atomic op({}) with ordering {:?} on memory({:?}, offset={}, size={})",
"Atomic op({}) with ordering {:?} on {:?} (size={})",
description,
&atomic,
place_ptr.alloc_id,
place_ptr.offset.bytes(),
ptr,
size.bytes()
);

// Perform the atomic operation.
data_race.maybe_perform_sync_operation(|index, mut clocks| {
for (_, range) in
alloc_meta.alloc_ranges.borrow_mut().iter_mut(place_ptr.offset, size)
for (offset, range) in
alloc_meta.alloc_ranges.borrow_mut().iter_mut(base_offset, size)
{
if let Err(DataRace) = op(range, &mut *clocks, index, atomic) {
mem::drop(clocks);
Expand All @@ -1050,8 +1025,7 @@ trait EvalContextPrivExt<'mir, 'tcx: 'mir>: MiriEvalContextExt<'mir, 'tcx> {
range,
description,
true,
place_ptr,
size,
Pointer::new(alloc_id, offset),
)
.map(|_| true);
}
Expand All @@ -1063,12 +1037,11 @@ trait EvalContextPrivExt<'mir, 'tcx: 'mir>: MiriEvalContextExt<'mir, 'tcx> {

// Log changes to atomic memory.
if log::log_enabled!(log::Level::Trace) {
for (_, range) in alloc_meta.alloc_ranges.borrow().iter(place_ptr.offset, size)
for (_offset, range) in alloc_meta.alloc_ranges.borrow().iter(base_offset, size)
{
log::trace!(
"Updated atomic memory({:?}, offset={}, size={}) to {:#?}",
place.ptr.assert_ptr().alloc_id,
place_ptr.offset.bytes(),
"Updated atomic memory({:?}, size={}) to {:#?}",
ptr,
size.bytes(),
range.atomic_ops
);
Expand Down
2 changes: 1 addition & 1 deletion src/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ pub fn report_error<'tcx, 'mir>(
let helps = match e.kind() {
Unsupported(UnsupportedOpInfo::NoMirFor(..)) =>
vec![(None, format!("make sure to use a Miri sysroot, which you can prepare with `cargo miri setup`"))],
Unsupported(UnsupportedOpInfo::ReadBytesAsPointer | UnsupportedOpInfo::ThreadLocalStatic(_) | UnsupportedOpInfo::ReadExternStatic(_)) =>
Unsupported(UnsupportedOpInfo::ThreadLocalStatic(_) | UnsupportedOpInfo::ReadExternStatic(_)) =>
panic!("Error should never be raised by Miri: {:?}", e.kind()),
Unsupported(_) =>
vec![(None, format!("this is likely not a bug in the program; it indicates that the program performed an operation that the interpreter does not support"))],
Expand Down
27 changes: 15 additions & 12 deletions src/eval.rs
Original file line number Diff line number Diff line change
Expand Up @@ -164,15 +164,16 @@ pub fn create_ecx<'mir, 'tcx: 'mir>(
// Third argument (`argv`): created from `config.args`.
let argv = {
// Put each argument in memory, collect pointers.
let mut argvs = Vec::<Scalar<Tag>>::new();
let mut argvs = Vec::<Immediate<Tag>>::new();
for arg in config.args.iter() {
// Make space for `0` terminator.
let size = u64::try_from(arg.len()).unwrap().checked_add(1).unwrap();
let arg_type = tcx.mk_array(tcx.types.u8, size);
let arg_place =
ecx.allocate(ecx.layout_of(arg_type)?, MiriMemoryKind::Machine.into())?;
ecx.write_os_str_to_c_str(OsStr::new(arg), arg_place.ptr, size)?;
argvs.push(arg_place.ptr);
ecx.mark_immutable(&*arg_place);
argvs.push(arg_place.to_ref(&ecx));
}
// Make an array with all these pointers, in the Miri memory.
let argvs_layout = ecx.layout_of(
Expand All @@ -181,24 +182,26 @@ pub fn create_ecx<'mir, 'tcx: 'mir>(
let argvs_place = ecx.allocate(argvs_layout, MiriMemoryKind::Machine.into())?;
for (idx, arg) in argvs.into_iter().enumerate() {
let place = ecx.mplace_field(&argvs_place, idx)?;
ecx.write_scalar(arg, &place.into())?;
ecx.write_immediate(arg, &place.into())?;
}
ecx.memory.mark_immutable(argvs_place.ptr.assert_ptr().alloc_id)?;
ecx.mark_immutable(&*argvs_place);
// A pointer to that place is the 3rd argument for main.
let argv = argvs_place.ptr;
let argv = argvs_place.to_ref(&ecx);
// Store `argc` and `argv` for macOS `_NSGetArg{c,v}`.
{
let argc_place =
ecx.allocate(ecx.machine.layouts.isize, MiriMemoryKind::Machine.into())?;
ecx.write_scalar(argc, &argc_place.into())?;
ecx.machine.argc = Some(argc_place.ptr);
ecx.mark_immutable(&*argc_place);
ecx.machine.argc = Some(*argc_place);

let argv_place = ecx.allocate(
ecx.layout_of(tcx.mk_imm_ptr(tcx.types.unit))?,
MiriMemoryKind::Machine.into(),
)?;
ecx.write_scalar(argv, &argv_place.into())?;
ecx.machine.argv = Some(argv_place.ptr);
ecx.write_immediate(argv, &argv_place.into())?;
ecx.mark_immutable(&*argv_place);
ecx.machine.argv = Some(*argv_place);
}
// Store command line as UTF-16 for Windows `GetCommandLineW`.
{
Expand All @@ -217,12 +220,13 @@ pub fn create_ecx<'mir, 'tcx: 'mir>(
let cmd_type = tcx.mk_array(tcx.types.u16, u64::try_from(cmd_utf16.len()).unwrap());
let cmd_place =
ecx.allocate(ecx.layout_of(cmd_type)?, MiriMemoryKind::Machine.into())?;
ecx.machine.cmd_line = Some(cmd_place.ptr);
ecx.machine.cmd_line = Some(*cmd_place);
// Store the UTF-16 string. We just allocated so we know the bounds are fine.
for (idx, &c) in cmd_utf16.iter().enumerate() {
let place = ecx.mplace_field(&cmd_place, idx)?;
ecx.write_scalar(Scalar::from_u16(c), &place.into())?;
}
ecx.mark_immutable(&*cmd_place);
}
argv
};
Expand All @@ -233,7 +237,7 @@ pub fn create_ecx<'mir, 'tcx: 'mir>(
ecx.call_function(
start_instance,
Abi::Rust,
&[main_ptr.into(), argc.into(), argv.into()],
&[Scalar::from_pointer(main_ptr, &ecx).into(), argc.into(), argv],
Some(&ret_place.into()),
StackPopCleanup::None { cleanup: true },
)?;
Expand Down Expand Up @@ -285,8 +289,7 @@ pub fn eval_main<'tcx>(tcx: TyCtxt<'tcx>, main_id: DefId, config: MiriConfig) ->
}
ecx.process_diagnostics(info);
}
let return_code =
ecx.read_scalar(&ret_place.into())?.check_init()?.to_machine_isize(&ecx)?;
let return_code = ecx.read_scalar(&ret_place.into())?.to_machine_isize(&ecx)?;
Ok(return_code)
})();

Expand Down
Loading