Skip to content

Commit

Permalink
data-race: preserve structured access information longer, and don't u…
Browse files Browse the repository at this point in the history 
…pper-case access types
  • Loading branch information
@RalfJung
RalfJung committed Oct 27, 2023
1 parent f40022d commit 38bb5c8
Show file tree
Hide file tree
Showing 70 changed files with 196 additions and 154 deletions.
142 changes: 92 additions & 50 deletions src/concurrency/data_race.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@
//! on the data-race detection code.
use std::{
borrow::Cow,
cell::{Cell, Ref, RefCell, RefMut},
fmt::Debug,
mem,
Expand Down Expand Up @@ -199,7 +198,7 @@ struct AtomicMemoryCellClocks {
/// are all treated as writes for the purpose
/// of the data-race detector.
#[derive(Copy, Clone, PartialEq, Eq, Debug)]
enum WriteType {
enum NaWriteType {
/// Allocate memory.
Allocate,

Expand All @@ -212,12 +211,41 @@ enum WriteType {
/// (Same for `Allocate` above.)
Deallocate,
}
impl WriteType {
fn get_descriptor(self) -> &'static str {

impl NaWriteType {
fn description(self) -> &'static str {
match self {
NaWriteType::Allocate => "creating a new allocation",
NaWriteType::Write => "non-atomic write",
NaWriteType::Deallocate => "deallocation",
}
}
}

#[derive(Copy, Clone, PartialEq, Eq, Debug)]
enum AccessType {
NaRead,
NaWrite(NaWriteType),
AtomicLoad,
AtomicStore,
AtomicRmw,
}

impl AccessType {
fn description(self) -> &'static str {
match self {
AccessType::NaRead => "non-atomic read",
AccessType::NaWrite(w) => w.description(),
AccessType::AtomicLoad => "atomic load",
AccessType::AtomicStore => "atomic store",
AccessType::AtomicRmw => "atomic read-modify-write",
}
}

fn is_atomic(self) -> bool {
match self {
WriteType::Allocate => "Allocate",
WriteType::Write => "Write",
WriteType::Deallocate => "Deallocate",
AccessType::AtomicLoad | AccessType::AtomicStore | AccessType::AtomicRmw => true,
AccessType::NaRead | AccessType::NaWrite(_) => false,
}
}
}
Expand All @@ -234,7 +262,7 @@ struct MemoryCellClocks {
/// The type of operation that the write index represents,
/// either newly allocated memory, a non-atomic write or
/// a deallocation of memory.
write_type: WriteType,
write_type: NaWriteType,

/// The vector-clock of all non-atomic reads that happened since the last non-atomic write
/// (i.e., we join together the "singleton" clocks corresponding to each read). It is reset to
Expand Down Expand Up @@ -265,7 +293,7 @@ impl MemoryCellClocks {
MemoryCellClocks {
read: VClock::default(),
write: (alloc_index, alloc),
write_type: WriteType::Allocate,
write_type: NaWriteType::Allocate,
atomic_ops: None,
}
}
Expand Down Expand Up @@ -488,7 +516,7 @@ impl MemoryCellClocks {
&mut self,
thread_clocks: &mut ThreadClockSet,
index: VectorIdx,
write_type: WriteType,
write_type: NaWriteType,
current_span: Span,
) -> Result<(), DataRace> {
log::trace!("Unsynchronized write with vectors: {:#?} :: {:#?}", self, thread_clocks);
Expand Down Expand Up @@ -838,48 +866,47 @@ impl VClockAlloc {
global: &GlobalState,
thread_mgr: &ThreadManager<'_, '_>,
mem_clocks: &MemoryCellClocks,
action: &str,
is_atomic: bool,
access: AccessType,
access_size: Size,
ptr_dbg: Pointer<AllocId>,
) -> InterpResult<'tcx> {
let (current_index, current_clocks) = global.current_thread_state(thread_mgr);
let mut action = Cow::Borrowed(action);
let mut other_size = None; // if `Some`, this was a size-mismatch race
let mut involves_non_atomic = true;
let write_clock;
let (other_action, other_thread, other_clock) =
// First check the atomic-nonatomic cases. If it looks like multiple
// cases apply, this one should take precedence, else it might look like
// we are reporting races between two non-atomic reads.
if !is_atomic &&
if !access.is_atomic() &&
let Some(atomic) = mem_clocks.atomic() &&
let Some(idx) = Self::find_gt_index(&atomic.write_vector, &current_clocks.clock)
{
(format!("Atomic Store"), idx, &atomic.write_vector)
} else if !is_atomic &&
(AccessType::AtomicStore, idx, &atomic.write_vector)
} else if !access.is_atomic() &&
let Some(atomic) = mem_clocks.atomic() &&
let Some(idx) = Self::find_gt_index(&atomic.read_vector, &current_clocks.clock)
{
(format!("Atomic Load"), idx, &atomic.read_vector)
(AccessType::AtomicLoad, idx, &atomic.read_vector)
// Then check races with non-atomic writes/reads.
} else if mem_clocks.write.1 > current_clocks.clock[mem_clocks.write.0] {
write_clock = mem_clocks.write();
(mem_clocks.write_type.get_descriptor().to_owned(), mem_clocks.write.0, &write_clock)
(AccessType::NaWrite(mem_clocks.write_type), mem_clocks.write.0, &write_clock)
} else if let Some(idx) = Self::find_gt_index(&mem_clocks.read, &current_clocks.clock) {
(format!("Read"), idx, &mem_clocks.read)
(AccessType::NaRead, idx, &mem_clocks.read)
// Finally, mixed-size races.
} else if is_atomic && let Some(atomic) = mem_clocks.atomic() && atomic.size != access_size {
} else if access.is_atomic() && let Some(atomic) = mem_clocks.atomic() && atomic.size != access_size {
// This is only a race if we are not synchronized with all atomic accesses, so find
// the one we are not synchronized with.
involves_non_atomic = false;
action = format!("{}-byte (different-size) {action}", access_size.bytes()).into();
other_size = Some(atomic.size);
if let Some(idx) = Self::find_gt_index(&atomic.write_vector, &current_clocks.clock)
{
(format!("{}-byte Atomic Store", atomic.size.bytes()), idx, &atomic.write_vector)
(AccessType::AtomicStore, idx, &atomic.write_vector)
} else if let Some(idx) =
Self::find_gt_index(&atomic.read_vector, &current_clocks.clock)
{
(format!("{}-byte Atomic Load", atomic.size.bytes()), idx, &atomic.read_vector)
(AccessType::AtomicLoad, idx, &atomic.read_vector)
} else {
unreachable!(
"Failed to report data-race for mixed-size access: no race found"
Expand All @@ -898,12 +925,24 @@ impl VClockAlloc {
involves_non_atomic,
ptr: ptr_dbg,
op1: RacingOp {
action: other_action.to_string(),
action: if let Some(other_size) = other_size {
format!("{}-byte {}", other_size.bytes(), other_action.description())
} else {
other_action.description().to_owned()
},
thread_info: other_thread_info,
span: other_clock.as_slice()[other_thread.index()].span_data(),
},
op2: RacingOp {
action: action.to_string(),
action: if other_size.is_some() {
format!(
"{}-byte (different-size) {}",
access_size.bytes(),
access.description()
)
} else {
access.description().to_owned()
},
thread_info: current_thread_info,
span: current_clocks.clock.as_slice()[current_index.index()].span_data(),
},
Expand Down Expand Up @@ -938,8 +977,7 @@ impl VClockAlloc {
global,
&machine.threads,
mem_clocks,
"Read",
/* is_atomic */ false,
AccessType::NaRead,
access_range.size,
Pointer::new(alloc_id, Size::from_bytes(mem_clocks_range.start)),
);
Expand All @@ -956,7 +994,7 @@ impl VClockAlloc {
&mut self,
alloc_id: AllocId,
access_range: AllocRange,
write_type: WriteType,
write_type: NaWriteType,
machine: &mut MiriMachine<'_, '_>,
) -> InterpResult<'tcx> {
let current_span = machine.current_span();
Expand All @@ -978,8 +1016,7 @@ impl VClockAlloc {
global,
&machine.threads,
mem_clocks,
write_type.get_descriptor(),
/* is_atomic */ false,
AccessType::NaWrite(write_type),
access_range.size,
Pointer::new(alloc_id, Size::from_bytes(mem_clocks_range.start)),
);
Expand All @@ -1001,7 +1038,7 @@ impl VClockAlloc {
range: AllocRange,
machine: &mut MiriMachine<'_, '_>,
) -> InterpResult<'tcx> {
self.unique_access(alloc_id, range, WriteType::Write, machine)
self.unique_access(alloc_id, range, NaWriteType::Write, machine)
}

/// Detect data-races for an unsynchronized deallocate operation, will not perform
Expand All @@ -1014,7 +1051,7 @@ impl VClockAlloc {
range: AllocRange,
machine: &mut MiriMachine<'_, '_>,
) -> InterpResult<'tcx> {
self.unique_access(alloc_id, range, WriteType::Deallocate, machine)
self.unique_access(alloc_id, range, NaWriteType::Deallocate, machine)
}
}

Expand Down Expand Up @@ -1104,7 +1141,7 @@ trait EvalContextPrivExt<'mir, 'tcx: 'mir>: MiriInterpCxExt<'mir, 'tcx> {
this.validate_atomic_op(
place,
atomic,
"Atomic Load",
AccessType::AtomicLoad,
move |memory, clocks, index, atomic| {
if atomic == AtomicReadOrd::Relaxed {
memory.load_relaxed(&mut *clocks, index, place.layout.size)
Expand All @@ -1126,7 +1163,7 @@ trait EvalContextPrivExt<'mir, 'tcx: 'mir>: MiriInterpCxExt<'mir, 'tcx> {
this.validate_atomic_op(
place,
atomic,
"Atomic Store",
AccessType::AtomicStore,
move |memory, clocks, index, atomic| {
if atomic == AtomicWriteOrd::Relaxed {
memory.store_relaxed(clocks, index, place.layout.size)
Expand All @@ -1148,26 +1185,31 @@ trait EvalContextPrivExt<'mir, 'tcx: 'mir>: MiriInterpCxExt<'mir, 'tcx> {
let acquire = matches!(atomic, Acquire | AcqRel | SeqCst);
let release = matches!(atomic, Release | AcqRel | SeqCst);
let this = self.eval_context_mut();
this.validate_atomic_op(place, atomic, "Atomic RMW", move |memory, clocks, index, _| {
if acquire {
memory.load_acquire(clocks, index, place.layout.size)?;
} else {
memory.load_relaxed(clocks, index, place.layout.size)?;
}
if release {
memory.rmw_release(clocks, index, place.layout.size)
} else {
memory.rmw_relaxed(clocks, index, place.layout.size)
}
})
this.validate_atomic_op(
place,
atomic,
AccessType::AtomicRmw,
move |memory, clocks, index, _| {
if acquire {
memory.load_acquire(clocks, index, place.layout.size)?;
} else {
memory.load_relaxed(clocks, index, place.layout.size)?;
}
if release {
memory.rmw_release(clocks, index, place.layout.size)
} else {
memory.rmw_relaxed(clocks, index, place.layout.size)
}
},
)
}

/// Generic atomic operation implementation
fn validate_atomic_op<A: Debug + Copy>(
&self,
place: &MPlaceTy<'tcx, Provenance>,
atomic: A,
description: &str,
access: AccessType,
mut op: impl FnMut(
&mut MemoryCellClocks,
&mut ThreadClockSet,
Expand All @@ -1176,6 +1218,7 @@ trait EvalContextPrivExt<'mir, 'tcx: 'mir>: MiriInterpCxExt<'mir, 'tcx> {
) -> Result<(), DataRace>,
) -> InterpResult<'tcx> {
let this = self.eval_context_ref();
assert!(access.is_atomic());
if let Some(data_race) = &this.machine.data_race {
if data_race.race_detecting() {
let size = place.layout.size;
Expand All @@ -1185,7 +1228,7 @@ trait EvalContextPrivExt<'mir, 'tcx: 'mir>: MiriInterpCxExt<'mir, 'tcx> {
let alloc_meta = this.get_alloc_extra(alloc_id)?.data_race.as_ref().unwrap();
log::trace!(
"Atomic op({}) with ordering {:?} on {:?} (size={})",
description,
access.description(),
&atomic,
place.ptr(),
size.bytes()
Expand All @@ -1207,8 +1250,7 @@ trait EvalContextPrivExt<'mir, 'tcx: 'mir>: MiriInterpCxExt<'mir, 'tcx> {
data_race,
&this.machine.threads,
mem_clocks,
description,
/* is_atomic */ true,
access,
place.layout.size,
Pointer::new(
alloc_id,
Expand Down
2 changes: 1 addition & 1 deletion tests/fail/both_borrows/retag_data_race_write.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ fn thread_1(p: SendPtr) {
fn thread_2(p: SendPtr) {
let p = p.0;
unsafe {
*p = 5; //~ ERROR: /Data race detected between \(1\) (Read|Write) on thread `<unnamed>` and \(2\) Write on thread `<unnamed>`/
*p = 5; //~ ERROR: /Data race detected between \(1\) non-atomic (read|write) on thread `<unnamed>` and \(2\) non-atomic write on thread `<unnamed>`/
}
}

Expand Down
4 changes: 2 additions & 2 deletions tests/fail/both_borrows/retag_data_race_write.stack.stderr
View file
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
error: Undefined Behavior: Data race detected between (1) Write on thread `<unnamed>` and (2) Write on thread `<unnamed>` at ALLOC. (2) just happened here
error: Undefined Behavior: Data race detected between (1) non-atomic write on thread `<unnamed>` and (2) non-atomic write on thread `<unnamed>` at ALLOC. (2) just happened here
--> $DIR/retag_data_race_write.rs:LL:CC
|
LL | *p = 5;
| ^^^^^^ Data race detected between (1) Write on thread `<unnamed>` and (2) Write on thread `<unnamed>` at ALLOC. (2) just happened here
| ^^^^^^ Data race detected between (1) non-atomic write on thread `<unnamed>` and (2) non-atomic write on thread `<unnamed>` at ALLOC. (2) just happened here
|
help: and (1) occurred earlier here
--> $DIR/retag_data_race_write.rs:LL:CC
Expand Down
4 changes: 2 additions & 2 deletions tests/fail/both_borrows/retag_data_race_write.tree.stderr
View file
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
error: Undefined Behavior: Data race detected between (1) Read on thread `<unnamed>` and (2) Write on thread `<unnamed>` at ALLOC. (2) just happened here
error: Undefined Behavior: Data race detected between (1) non-atomic read on thread `<unnamed>` and (2) non-atomic write on thread `<unnamed>` at ALLOC. (2) just happened here
--> $DIR/retag_data_race_write.rs:LL:CC
|
LL | *p = 5;
| ^^^^^^ Data race detected between (1) Read on thread `<unnamed>` and (2) Write on thread `<unnamed>` at ALLOC. (2) just happened here
| ^^^^^^ Data race detected between (1) non-atomic read on thread `<unnamed>` and (2) non-atomic write on thread `<unnamed>` at ALLOC. (2) just happened here
|
help: and (1) occurred earlier here
--> $DIR/retag_data_race_write.rs:LL:CC
Expand Down
2 changes: 1 addition & 1 deletion tests/fail/data_race/alloc_read_race.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ pub fn main() {
let pointer = &*ptr.0;

// Note: could also error due to reading uninitialized memory, but the data-race detector triggers first.
*pointer.load(Ordering::Relaxed) //~ ERROR: Data race detected between (1) Allocate on thread `<unnamed>` and (2) Read on thread `<unnamed>`
*pointer.load(Ordering::Relaxed) //~ ERROR: Data race detected between (1) creating a new allocation on thread `<unnamed>` and (2) non-atomic read on thread `<unnamed>`
});

j1.join().unwrap();
Expand Down
4 changes: 2 additions & 2 deletions tests/fail/data_race/alloc_read_race.stderr
View file
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
error: Undefined Behavior: Data race detected between (1) Allocate on thread `<unnamed>` and (2) Read on thread `<unnamed>` at ALLOC. (2) just happened here
error: Undefined Behavior: Data race detected between (1) creating a new allocation on thread `<unnamed>` and (2) non-atomic read on thread `<unnamed>` at ALLOC. (2) just happened here
--> $DIR/alloc_read_race.rs:LL:CC
|
LL | *pointer.load(Ordering::Relaxed)
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Data race detected between (1) Allocate on thread `<unnamed>` and (2) Read on thread `<unnamed>` at ALLOC. (2) just happened here
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Data race detected between (1) creating a new allocation on thread `<unnamed>` and (2) non-atomic read on thread `<unnamed>` at ALLOC. (2) just happened here
|
help: and (1) occurred earlier here
--> $DIR/alloc_read_race.rs:LL:CC
Expand Down
2 changes: 1 addition & 1 deletion tests/fail/data_race/alloc_write_race.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ pub fn main() {
let j2 = spawn(move || {
let ptr = ptr; // avoid field capturing
let pointer = &*ptr.0;
*pointer.load(Ordering::Relaxed) = 2; //~ ERROR: Data race detected between (1) Allocate on thread `<unnamed>` and (2) Write on thread `<unnamed>`
*pointer.load(Ordering::Relaxed) = 2; //~ ERROR: Data race detected between (1) creating a new allocation on thread `<unnamed>` and (2) non-atomic write on thread `<unnamed>`
});

j1.join().unwrap();
Expand Down
4 changes: 2 additions & 2 deletions tests/fail/data_race/alloc_write_race.stderr
View file
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
error: Undefined Behavior: Data race detected between (1) Allocate on thread `<unnamed>` and (2) Write on thread `<unnamed>` at ALLOC. (2) just happened here
error: Undefined Behavior: Data race detected between (1) creating a new allocation on thread `<unnamed>` and (2) non-atomic write on thread `<unnamed>` at ALLOC. (2) just happened here
--> $DIR/alloc_write_race.rs:LL:CC
|
LL | *pointer.load(Ordering::Relaxed) = 2;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Data race detected between (1) Allocate on thread `<unnamed>` and (2) Write on thread `<unnamed>` at ALLOC. (2) just happened here
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Data race detected between (1) creating a new allocation on thread `<unnamed>` and (2) non-atomic write on thread `<unnamed>` at ALLOC. (2) just happened here
|
help: and (1) occurred earlier here
--> $DIR/alloc_write_race.rs:LL:CC
Expand Down
2 changes: 1 addition & 1 deletion tests/fail/data_race/atomic_read_na_write_race1.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ pub fn main() {

let j2 = spawn(move || {
let c = c; // avoid field capturing
(&*c.0).load(Ordering::SeqCst) //~ ERROR: Data race detected between (1) Write on thread `<unnamed>` and (2) Atomic Load on thread `<unnamed>`
(&*c.0).load(Ordering::SeqCst) //~ ERROR: Data race detected between (1) non-atomic write on thread `<unnamed>` and (2) atomic load on thread `<unnamed>`
});

j1.join().unwrap();
Expand Down
Loading

0 comments on commit 38bb5c8

Please sign in to comment.