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.

Sign up for GitHub

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

Jump to bottom

avoid copying thread manager state in data race detector #2302

Merged
merged 3 commits into from
Jul 2, 2022
Merged

avoid copying thread manager state in data race detector #2302

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f238513
rename some data_race types for more clarity
RalfJung Jul 1, 2022
dfdedae
avoid copying thread manager state in data race detector
RalfJung Jul 1, 2022
d09db16
fix and slightly improve data race reports
RalfJung Jul 1, 2022
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
285 changes: 129 additions & 156 deletions src/concurrency/data_race.rs
View file
Open in desktop

Large diffs are not rendered by default.

74 changes: 41 additions & 33 deletions src/concurrency/weak_memory.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
//! Note that this implementation does not take into account of C++20's memory model revision to SC accesses
//! and fences introduced by P0668 (<https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0668r5.html>).
//! This implementation is not fully correct under the revised C++20 model and may generate behaviours C++20
//! disallows.
//! disallows (<https://github.com/rust-lang/miri/issues/2301>).
//!
//! Rust follows the C++20 memory model (except for the Consume ordering and some operations not performable through C++'s
//! std::atomic<T> API). It is therefore possible for this implementation to generate behaviours never observable when the
Expand Down Expand Up @@ -82,10 +82,12 @@ use rustc_const_eval::interpret::{
};
use rustc_data_structures::fx::FxHashMap;

use crate::{AtomicReadOp, AtomicRwOp, AtomicWriteOp, Tag, VClock, VTimestamp, VectorIdx};
use crate::{
AtomicReadOrd, AtomicRwOrd, AtomicWriteOrd, Tag, ThreadManager, VClock, VTimestamp, VectorIdx,
};

use super::{
data_race::{GlobalState, ThreadClockSet},
data_race::{GlobalState as DataRaceState, ThreadClockSet},
range_object_map::{AccessType, RangeObjectMap},
};

Expand Down Expand Up @@ -149,7 +151,7 @@ impl StoreBufferAlloc {
/// before without data race, we can determine that the non-atomic access fully happens
/// after all the prior atomic accesses so the location no longer needs to exhibit
/// any weak memory behaviours until further atomic accesses.
pub fn memory_accessed(&self, range: AllocRange, global: &GlobalState) {
pub fn memory_accessed(&self, range: AllocRange, global: &DataRaceState) {
if !global.ongoing_action_data_race_free() {
let mut buffers = self.store_buffers.borrow_mut();
let access_type = buffers.access_type(range);
Expand Down Expand Up @@ -236,17 +238,18 @@ impl<'mir, 'tcx: 'mir> StoreBuffer {
}

/// Reads from the last store in modification order
fn read_from_last_store(&self, global: &GlobalState) {
fn read_from_last_store(&self, global: &DataRaceState, thread_mgr: &ThreadManager<'_, '_>) {
let store_elem = self.buffer.back();
if let Some(store_elem) = store_elem {
let (index, clocks) = global.current_thread_state();
let (index, clocks) = global.current_thread_state(thread_mgr);
store_elem.load_impl(index, &clocks);
}
}

fn buffered_read(
&self,
global: &GlobalState,
global: &DataRaceState,
thread_mgr: &ThreadManager<'_, '_>,
is_seqcst: bool,
rng: &mut (impl rand::Rng + ?Sized),
validate: impl FnOnce() -> InterpResult<'tcx>,
Expand All @@ -257,7 +260,7 @@ impl<'mir, 'tcx: 'mir> StoreBuffer {
let store_elem = {
// The `clocks` we got here must be dropped before calling validate_atomic_load
// as the race detector will update it
let (.., clocks) = global.current_thread_state();
let (.., clocks) = global.current_thread_state(thread_mgr);
// Load from a valid entry in the store buffer
self.fetch_store(is_seqcst, &clocks, &mut *rng)
};
Expand All @@ -268,18 +271,19 @@ impl<'mir, 'tcx: 'mir> StoreBuffer {
// requires access to ThreadClockSet.clock, which is updated by the race detector
validate()?;

let (index, clocks) = global.current_thread_state();
let (index, clocks) = global.current_thread_state(thread_mgr);
let loaded = store_elem.load_impl(index, &clocks);
Ok(loaded)
}

fn buffered_write(
&mut self,
val: ScalarMaybeUninit<Tag>,
global: &GlobalState,
global: &DataRaceState,
thread_mgr: &ThreadManager<'_, '_>,
is_seqcst: bool,
) -> InterpResult<'tcx> {
let (index, clocks) = global.current_thread_state();
let (index, clocks) = global.current_thread_state(thread_mgr);

self.store_impl(val, index, &clocks.clock, is_seqcst);
Ok(())
Expand Down Expand Up @@ -428,8 +432,11 @@ pub(super) trait EvalContextExt<'mir, 'tcx: 'mir>:
{
let range = alloc_range(base_offset, place.layout.size);
if alloc_buffers.is_overlapping(range)
&& !alloc_clocks
.race_free_with_atomic(range, this.machine.data_race.as_ref().unwrap())
&& !alloc_clocks.race_free_with_atomic(
range,
this.machine.data_race.as_ref().unwrap(),
&this.machine.threads,
)
{
throw_unsup_format!(
"racy imperfectly overlapping atomic access is not possible in the C++20 memory model, and not supported by Miri's weak memory emulation"
Expand All @@ -443,41 +450,41 @@ pub(super) trait EvalContextExt<'mir, 'tcx: 'mir>:
&mut self,
new_val: ScalarMaybeUninit<Tag>,
place: &MPlaceTy<'tcx, Tag>,
atomic: AtomicRwOp,
atomic: AtomicRwOrd,
init: ScalarMaybeUninit<Tag>,
) -> InterpResult<'tcx> {
let this = self.eval_context_mut();
let (alloc_id, base_offset, ..) = this.ptr_get_alloc_id(place.ptr)?;
if let (
crate::AllocExtra { weak_memory: Some(alloc_buffers), .. },
crate::Evaluator { data_race: Some(global), .. },
crate::Evaluator { data_race: Some(global), threads, .. },
) = this.get_alloc_extra_mut(alloc_id)?
{
if atomic == AtomicRwOp::SeqCst {
global.sc_read();
global.sc_write();
if atomic == AtomicRwOrd::SeqCst {
global.sc_read(threads);
global.sc_write(threads);
}
let range = alloc_range(base_offset, place.layout.size);
let buffer = alloc_buffers.get_or_create_store_buffer_mut(range, init)?;
buffer.read_from_last_store(global);
buffer.buffered_write(new_val, global, atomic == AtomicRwOp::SeqCst)?;
buffer.read_from_last_store(global, threads);
buffer.buffered_write(new_val, global, threads, atomic == AtomicRwOrd::SeqCst)?;
}
Ok(())
}

fn buffered_atomic_read(
&self,
place: &MPlaceTy<'tcx, Tag>,
atomic: AtomicReadOp,
atomic: AtomicReadOrd,
latest_in_mo: ScalarMaybeUninit<Tag>,
validate: impl FnOnce() -> InterpResult<'tcx>,
) -> InterpResult<'tcx, ScalarMaybeUninit<Tag>> {
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)?;
if let Some(alloc_buffers) = this.get_alloc_extra(alloc_id)?.weak_memory.as_ref() {
if atomic == AtomicReadOp::SeqCst {
global.sc_read();
if atomic == AtomicReadOrd::SeqCst {
global.sc_read(&this.machine.threads);
}
let mut rng = this.machine.rng.borrow_mut();
let buffer = alloc_buffers.get_or_create_store_buffer(
Expand All @@ -486,7 +493,8 @@ pub(super) trait EvalContextExt<'mir, 'tcx: 'mir>:
)?;
let loaded = buffer.buffered_read(
global,
atomic == AtomicReadOp::SeqCst,
&this.machine.threads,
atomic == AtomicReadOrd::SeqCst,
&mut *rng,
validate,
)?;
Expand All @@ -504,18 +512,18 @@ pub(super) trait EvalContextExt<'mir, 'tcx: 'mir>:
&mut self,
val: ScalarMaybeUninit<Tag>,
dest: &MPlaceTy<'tcx, Tag>,
atomic: AtomicWriteOp,
atomic: AtomicWriteOrd,
init: ScalarMaybeUninit<Tag>,
) -> InterpResult<'tcx> {
let this = self.eval_context_mut();
let (alloc_id, base_offset, ..) = this.ptr_get_alloc_id(dest.ptr)?;
if let (
crate::AllocExtra { weak_memory: Some(alloc_buffers), .. },
crate::Evaluator { data_race: Some(global), .. },
crate::Evaluator { data_race: Some(global), threads, .. },
) = this.get_alloc_extra_mut(alloc_id)?
{
if atomic == AtomicWriteOp::SeqCst {
global.sc_write();
if atomic == AtomicWriteOrd::SeqCst {
global.sc_write(threads);
}

// UGLY HACK: in write_scalar_atomic() we don't know the value before our write,
Expand All @@ -535,7 +543,7 @@ pub(super) trait EvalContextExt<'mir, 'tcx: 'mir>:
buffer.buffer.pop_front();
}

buffer.buffered_write(val, global, atomic == AtomicWriteOp::SeqCst)?;
buffer.buffered_write(val, global, threads, atomic == AtomicWriteOrd::SeqCst)?;
}

// Caller should've written to dest with the vanilla scalar write, we do nothing here
Expand All @@ -548,21 +556,21 @@ pub(super) trait EvalContextExt<'mir, 'tcx: 'mir>:
fn perform_read_on_buffered_latest(
&self,
place: &MPlaceTy<'tcx, Tag>,
atomic: AtomicReadOp,
atomic: AtomicReadOrd,
init: ScalarMaybeUninit<Tag>,
) -> InterpResult<'tcx> {
let this = self.eval_context_ref();

if let Some(global) = &this.machine.data_race {
if atomic == AtomicReadOp::SeqCst {
global.sc_read();
if atomic == AtomicReadOrd::SeqCst {
global.sc_read(&this.machine.threads);
}
let size = place.layout.size;
let (alloc_id, base_offset, ..) = this.ptr_get_alloc_id(place.ptr)?;
if let Some(alloc_buffers) = this.get_alloc_extra(alloc_id)?.weak_memory.as_ref() {
let buffer = alloc_buffers
.get_or_create_store_buffer(alloc_range(base_offset, size), init)?;
buffer.read_from_last_store(global);
buffer.read_from_last_store(global, &this.machine.threads);
}
}
Ok(())
Expand Down
2 changes: 1 addition & 1 deletion src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ pub use crate::shims::tls::{EvalContextExt as _, TlsData};
pub use crate::shims::EvalContextExt as _;

pub use crate::concurrency::data_race::{
AtomicFenceOp, AtomicReadOp, AtomicRwOp, AtomicWriteOp,
AtomicFenceOrd, AtomicReadOrd, AtomicRwOrd, AtomicWriteOrd,
EvalContextExt as DataRaceEvalContextExt,
};
pub use crate::diagnostics::{
Expand Down
28 changes: 24 additions & 4 deletions src/machine.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -647,7 +647,12 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'mir, 'tcx> {
None
};
let race_alloc = if let Some(data_race) = &ecx.machine.data_race {
Some(data_race::AllocExtra::new_allocation(data_race, alloc.size(), kind))
Some(data_race::AllocExtra::new_allocation(
data_race,
&ecx.machine.threads,
alloc.size(),
kind,
))
} else {
None
};
Expand Down Expand Up @@ -756,7 +761,12 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'mir, 'tcx> {
range: AllocRange,
) -> InterpResult<'tcx> {
if let Some(data_race) = &alloc_extra.data_race {
data_race.read(alloc_id, range, machine.data_race.as_ref().unwrap())?;
data_race.read(
alloc_id,
range,
machine.data_race.as_ref().unwrap(),
&machine.threads,
)?;
}
if let Some(stacked_borrows) = &alloc_extra.stacked_borrows {
stacked_borrows.borrow_mut().memory_read(
Expand All @@ -782,7 +792,12 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'mir, 'tcx> {
range: AllocRange,
) -> InterpResult<'tcx> {
if let Some(data_race) = &mut alloc_extra.data_race {
data_race.write(alloc_id, range, machine.data_race.as_mut().unwrap())?;
data_race.write(
alloc_id,
range,
machine.data_race.as_mut().unwrap(),
&machine.threads,
)?;
}
if let Some(stacked_borrows) = &mut alloc_extra.stacked_borrows {
stacked_borrows.get_mut().memory_written(
Expand Down Expand Up @@ -811,7 +826,12 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'mir, 'tcx> {
register_diagnostic(NonHaltingDiagnostic::FreedAlloc(alloc_id));
}
if let Some(data_race) = &mut alloc_extra.data_race {
data_race.deallocate(alloc_id, range, machine.data_race.as_mut().unwrap())?;
data_race.deallocate(
alloc_id,
range,
machine.data_race.as_mut().unwrap(),
&machine.threads,
)?;
}
if let Some(stacked_borrows) = &mut alloc_extra.stacked_borrows {
stacked_borrows.get_mut().memory_deallocated(
Expand Down
Loading