Skip to content

Improve yield handling and add yield-based livelock detection. #1651

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

Closed
10 changes: 10 additions & 0 deletions src/bin/miri.rs
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,16 @@ fn main() {
};
miri_config.tracked_alloc_id = Some(miri::AllocId(id));
}
arg if arg.starts_with("-Zmiri-max-yield-iterations=") => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New options should come with a documentation in the README. That's also a good opportunity to try and give a short but precise description of what this does. ;)

let count: u32 = match arg.strip_prefix("-Zmiri-max-yield-iterations=").unwrap().parse() {
Ok(id) => id,
Err(err) => panic!(
"-Zmiri-max-yield-iterations= requires a valid `u32` argument: {}",
err
),
};
miri_config.max_yield_count = count;
}
_ => {
// Forward to rustc.
rustc_args.push(arg);
Expand Down
43 changes: 32 additions & 11 deletions src/data_race.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ use rustc_target::abi::Size;
use crate::{
ImmTy, Immediate, InterpResult, MPlaceTy, MemPlaceMeta, MiriEvalContext, MiriEvalContextExt,
OpTy, Pointer, RangeMap, ScalarMaybeUninit, Tag, ThreadId, VClock, VTimestamp,
VectorIdx, MemoryKind, MiriMemoryKind
VectorIdx, MemoryKind, MiriMemoryKind, ThreadsEvalContextExt
};

pub type AllocExtra = VClockAlloc;
Expand Down Expand Up @@ -445,13 +445,13 @@ impl<'mir, 'tcx: 'mir> EvalContextExt<'mir, 'tcx> for MiriEvalContext<'mir, 'tcx
pub trait EvalContextExt<'mir, 'tcx: 'mir>: MiriEvalContextExt<'mir, 'tcx> {
/// Atomic variant of read_scalar_at_offset.
fn read_scalar_at_offset_atomic(
&self,
&mut self,
op: OpTy<'tcx, Tag>,
offset: u64,
layout: TyAndLayout<'tcx>,
atomic: AtomicReadOp,
) -> InterpResult<'tcx, ScalarMaybeUninit<Tag>> {
let this = self.eval_context_ref();
let this = self.eval_context_mut();
let op_place = this.deref_operand(op)?;
let offset = Size::from_bytes(offset);

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

/// Perform an atomic read operation at the memory location.
fn read_scalar_atomic(
&self,
&mut self,
place: MPlaceTy<'tcx, Tag>,
atomic: AtomicReadOp,
) -> InterpResult<'tcx, ScalarMaybeUninit<Tag>> {
Expand Down Expand Up @@ -582,15 +582,16 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: MiriEvalContextExt<'mir, 'tcx> {
/// Update the data-race detector for an atomic read occurring at the
/// associated memory-place and on the current thread.
fn validate_atomic_load(
&self,
&mut self,
place: MPlaceTy<'tcx, Tag>,
atomic: AtomicReadOp,
) -> InterpResult<'tcx> {
let this = self.eval_context_ref();
let this = self.eval_context_mut();
this.validate_atomic_op(
place,
atomic,
"Atomic Load",
false,
move |memory, clocks, index, atomic| {
if atomic == AtomicReadOp::Relaxed {
memory.load_relaxed(&mut *clocks, index)
Expand All @@ -608,11 +609,12 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: MiriEvalContextExt<'mir, 'tcx> {
place: MPlaceTy<'tcx, Tag>,
atomic: AtomicWriteOp,
) -> InterpResult<'tcx> {
let this = self.eval_context_ref();
let this = self.eval_context_mut();
this.validate_atomic_op(
place,
atomic,
"Atomic Store",
true,
move |memory, clocks, index, atomic| {
if atomic == AtomicWriteOp::Relaxed {
memory.store_relaxed(clocks, index)
Expand All @@ -633,8 +635,15 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: MiriEvalContextExt<'mir, 'tcx> {
use AtomicRwOp::*;
let acquire = matches!(atomic, Acquire | AcqRel | SeqCst);
let release = matches!(atomic, Release | AcqRel | SeqCst);
let this = self.eval_context_ref();
this.validate_atomic_op(place, atomic, "Atomic RMW", move |memory, clocks, index, _| {
let this = self.eval_context_mut();
this.validate_atomic_op(
place,
atomic,
"Atomic RMW",
// For yields the atomic write overrides all effects of the atomic read
// so it is treated as an atomic write.
true,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using bool here makes it hard to figure out what true and false even mean... would it make sense to introduce a 2-variant enum for this purpose?

move |memory, clocks, index, _| {
if acquire {
memory.load_acquire(clocks, index)?;
} else {
Expand Down Expand Up @@ -961,18 +970,30 @@ trait EvalContextPrivExt<'mir, 'tcx: 'mir>: MiriEvalContextExt<'mir, 'tcx> {
/// FIXME: is this valid, or should get_raw_mut be used for
/// atomic-stores/atomic-rmw?
fn validate_atomic_op<A: Debug + Copy>(
&self,
&mut self,
place: MPlaceTy<'tcx, Tag>,
atomic: A,
description: &str,
write: bool,
mut op: impl FnMut(
&mut MemoryCellClocks,
&mut ThreadClockSet,
VectorIdx,
A,
) -> Result<(), DataRace>,
) -> InterpResult<'tcx> {
let this = self.eval_context_ref();
let this = self.eval_context_mut();

// Update yield metadata for yield based forward progress and live-lock detection.
let place_ptr = place.ptr.assert_ptr();
let size = place.layout.size;
if write {
this.thread_yield_atomic_wake(place_ptr.alloc_id, place_ptr.offset,size);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of passing alloc_id and offset separately, what about passing the entire Pointer?

} else {
let alloc_size = this.memory.get_raw(place_ptr.alloc_id)?.size;
this.thread_yield_atomic_watch(place_ptr.alloc_id, alloc_size, place_ptr.offset,size);
}

if let Some(data_race) = &this.memory.extra.data_race {
if data_race.multi_threaded.get() {
// Load and log the atomic operation.
Expand Down
10 changes: 10 additions & 0 deletions src/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ pub enum TerminationInfo {
UnsupportedInIsolation(String),
ExperimentalUb { msg: String, url: String },
Deadlock,
Livelock,
}

impl fmt::Display for TerminationInfo {
Expand All @@ -32,6 +33,8 @@ impl fmt::Display for TerminationInfo {
write!(f, "{}", msg),
Deadlock =>
write!(f, "the evaluated program deadlocked"),
Livelock =>
write!(f, "the evaluated program livelocked"),
}
}
}
Expand Down Expand Up @@ -67,6 +70,7 @@ pub fn report_error<'tcx, 'mir>(
ExperimentalUb { .. } =>
"Undefined Behavior",
Deadlock => "deadlock",
Livelock => "livelock",
};
let helps = match info {
UnsupportedInIsolation(_) =>
Expand All @@ -76,6 +80,12 @@ pub fn report_error<'tcx, 'mir>(
format!("this indicates a potential bug in the program: it performed an invalid operation, but the rules it violated are still experimental"),
format!("see {} for further information", url),
],
Livelock => vec![
format!("All thread yields or spin loop hints are used to dynamically generated watch sets to detect livelock and avoid unnecessary spins in spin-loops."),
format!("If this should not have reported a livelock then it may help to change the maximum number of spurious wakes from yields with no progress."),
format!("Pass the argument `-Zmiri-max-yield-iterations=<count>` to change this value."),
format!("The current value is {}, a value of 0 will allow an unlimited number of spurious wakes.", ecx.thread_yield_get_max_spurious_wake())
],
_ => vec![],
};
(title, helps)
Expand Down
6 changes: 5 additions & 1 deletion src/eval.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,9 @@ pub struct MiriConfig {
pub track_raw: bool,
/// Determine if data race detection should be enabled
pub data_race_detector: bool,
/// Maximum number of consecutive yield operations with no progress
/// to consider a thread live-locked. A value of zero never live-locks.
pub max_yield_count: u32,
}

impl Default for MiriConfig {
Expand All @@ -68,6 +71,7 @@ impl Default for MiriConfig {
tracked_alloc_id: None,
track_raw: false,
data_race_detector: true,
max_yield_count: 1
}
}
}
Expand All @@ -87,7 +91,7 @@ pub fn create_ecx<'mir, 'tcx: 'mir>(
tcx,
rustc_span::source_map::DUMMY_SP,
param_env,
Evaluator::new(config.communicate, config.validate, layout_cx),
Evaluator::new(config.communicate, config.validate, config.max_yield_count, layout_cx),
MemoryExtra::new(&config),
);
// Complete initialization.
Expand Down
1 change: 1 addition & 0 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#![feature(never_type)]
#![feature(or_patterns)]
#![feature(try_blocks)]
#![feature(hash_drain_filter)]

#![warn(rust_2018_idioms)]
#![allow(clippy::cast_lossless)]
Expand Down
3 changes: 2 additions & 1 deletion src/machine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -275,6 +275,7 @@ impl<'mir, 'tcx> Evaluator<'mir, 'tcx> {
pub(crate) fn new(
communicate: bool,
validate: bool,
max_yield_count: u32,
layout_cx: LayoutCx<'tcx, TyCtxt<'tcx>>,
) -> Self {
let layouts = PrimitiveLayouts::new(layout_cx)
Expand All @@ -293,7 +294,7 @@ impl<'mir, 'tcx> Evaluator<'mir, 'tcx> {
dir_handler: Default::default(),
time_anchor: Instant::now(),
layouts,
threads: ThreadManager::default(),
threads: ThreadManager::new(max_yield_count),
static_roots: Vec::new(),
}
}
Expand Down
6 changes: 6 additions & 0 deletions src/shims/foreign_items.rs
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,12 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
"miri_resolve_frame" => {
this.handle_miri_resolve_frame(args, dest)?;
}
// Performs a thread yield.
// Exists since a thread yield operation may not be available on a given platform.
"miri_yield_thread" => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As noted before, std::thread::yield_now is available on all platforms, so I do not understand why this new operation is needed.

let &[] = check_arg_count(args)?;
this.yield_active_thread();
}


// Standard C allocation
Expand Down
6 changes: 3 additions & 3 deletions src/shims/posix/sync.rs
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ fn mutex_set_kind<'mir, 'tcx: 'mir>(
}

fn mutex_get_id<'mir, 'tcx: 'mir>(
ecx: &MiriEvalContext<'mir, 'tcx>,
ecx: &mut MiriEvalContext<'mir, 'tcx>,
mutex_op: OpTy<'tcx, Tag>,
) -> InterpResult<'tcx, ScalarMaybeUninit<Tag>> {
ecx.read_scalar_at_offset_atomic(
Expand Down Expand Up @@ -125,7 +125,7 @@ fn mutex_get_or_create_id<'mir, 'tcx: 'mir>(
// bytes 4-7: rwlock id as u32 or 0 if id is not assigned yet.

fn rwlock_get_id<'mir, 'tcx: 'mir>(
ecx: &MiriEvalContext<'mir, 'tcx>,
ecx: &mut MiriEvalContext<'mir, 'tcx>,
rwlock_op: OpTy<'tcx, Tag>,
) -> InterpResult<'tcx, ScalarMaybeUninit<Tag>> {
ecx.read_scalar_at_offset_atomic(
Expand Down Expand Up @@ -192,7 +192,7 @@ fn condattr_set_clock_id<'mir, 'tcx: 'mir>(
// bytes 8-11: the clock id constant as i32

fn cond_get_id<'mir, 'tcx: 'mir>(
ecx: &MiriEvalContext<'mir, 'tcx>,
ecx: &mut MiriEvalContext<'mir, 'tcx>,
cond_op: OpTy<'tcx, Tag>,
) -> InterpResult<'tcx, ScalarMaybeUninit<Tag>> {
ecx.read_scalar_at_offset_atomic(
Expand Down
Loading