Skip to content

Commit 74305c6

Browse files
committed
miri: treat non-memory local variables properly for data race detection
1 parent f04fc4d commit 74305c6

11 files changed

+382
-7
lines changed

src/concurrency/data_race.rs

Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@ use std::{
4747
};
4848

4949
use rustc_ast::Mutability;
50+
use rustc_data_structures::fx::FxHashMap;
5051
use rustc_data_structures::fx::FxHashSet;
5152
use rustc_index::{Idx, IndexVec};
5253
use rustc_middle::{mir, ty::Ty};
@@ -1121,6 +1122,103 @@ impl VClockAlloc {
11211122
}
11221123
}
11231124

1125+
/// Vector clock state for a stack frame (tracking the local variables
1126+
/// that do not have an allocation yet).
1127+
#[derive(Debug, Default)]
1128+
pub struct FrameState {
1129+
local_clocks: RefCell<FxHashMap<mir::Local, LocalClocks>>,
1130+
}
1131+
1132+
/// Stripped-down version of [`MemoryCellClocks`] for the clocks we need to keep track
1133+
/// of in a local that does not yet have addressable memory -- and hence can only
1134+
/// be accessed from the thread its stack frame belongs to, and cannot be access atomically.
1135+
#[derive(Debug)]
1136+
struct LocalClocks {
1137+
write: VTimestamp,
1138+
write_type: NaWriteType,
1139+
read: VTimestamp,
1140+
}
1141+
1142+
impl Default for LocalClocks {
1143+
fn default() -> Self {
1144+
Self { write: VTimestamp::ZERO, write_type: NaWriteType::Allocate, read: VTimestamp::ZERO }
1145+
}
1146+
}
1147+
1148+
impl FrameState {
1149+
pub fn local_write(&self, local: mir::Local, storage_live: bool, machine: &MiriMachine<'_>) {
1150+
let current_span = machine.current_span();
1151+
let global = machine.data_race.as_ref().unwrap();
1152+
if global.race_detecting() {
1153+
let (index, mut thread_clocks) = global.active_thread_state_mut(&machine.threads);
1154+
// This should do the same things as `MemoryCellClocks::write_race_detect`.
1155+
if !current_span.is_dummy() {
1156+
thread_clocks.clock.index_mut(index).span = current_span;
1157+
}
1158+
let mut clocks = self.local_clocks.borrow_mut();
1159+
if storage_live {
1160+
let new_clocks = LocalClocks {
1161+
write: thread_clocks.clock[index],
1162+
write_type: NaWriteType::Allocate,
1163+
read: VTimestamp::ZERO,
1164+
};
1165+
// There might already be an entry in the map for this, if the local was previously
1166+
// live already.
1167+
clocks.insert(local, new_clocks);
1168+
} else {
1169+
// This can fail to exist if `race_detecting` was false when the allocation
1170+
// occurred, in which case we can backdate this to the beginning of time.
1171+
let clocks = clocks.entry(local).or_insert_with(Default::default);
1172+
clocks.write = thread_clocks.clock[index];
1173+
clocks.write_type = NaWriteType::Write;
1174+
}
1175+
}
1176+
}
1177+
1178+
pub fn local_read(&self, local: mir::Local, machine: &MiriMachine<'_>) {
1179+
let current_span = machine.current_span();
1180+
let global = machine.data_race.as_ref().unwrap();
1181+
if global.race_detecting() {
1182+
let (index, mut thread_clocks) = global.active_thread_state_mut(&machine.threads);
1183+
// This should do the same things as `MemoryCellClocks::read_race_detect`.
1184+
if !current_span.is_dummy() {
1185+
thread_clocks.clock.index_mut(index).span = current_span;
1186+
}
1187+
thread_clocks.clock.index_mut(index).set_read_type(NaReadType::Read);
1188+
// This can fail to exist if `race_detecting` was false when the allocation
1189+
// occurred, in which case we can backdate this to the beginning of time.
1190+
let mut clocks = self.local_clocks.borrow_mut();
1191+
let clocks = clocks.entry(local).or_insert_with(Default::default);
1192+
clocks.read = thread_clocks.clock[index];
1193+
}
1194+
}
1195+
1196+
pub fn local_moved_to_memory(
1197+
&self,
1198+
local: mir::Local,
1199+
alloc: &mut VClockAlloc,
1200+
machine: &MiriMachine<'_>,
1201+
) {
1202+
let global = machine.data_race.as_ref().unwrap();
1203+
if global.race_detecting() {
1204+
let (index, _thread_clocks) = global.active_thread_state_mut(&machine.threads);
1205+
// Get the time the last write actually happened. This can fail to exist if
1206+
// `race_detecting` was false when the write occurred, in that case we can backdate this
1207+
// to the beginning of time.
1208+
let local_clocks = self.local_clocks.borrow_mut().remove(&local).unwrap_or_default();
1209+
for (_mem_clocks_range, mem_clocks) in alloc.alloc_ranges.get_mut().iter_mut_all() {
1210+
// The initialization write for this already happened, just at the wrong timestamp.
1211+
// Check that the thread index matches what we expect.
1212+
assert_eq!(mem_clocks.write.0, index);
1213+
// Convert the local's clocks into memory clocks.
1214+
mem_clocks.write = (index, local_clocks.write);
1215+
mem_clocks.write_type = local_clocks.write_type;
1216+
mem_clocks.read = VClock::new_with_index(index, local_clocks.read);
1217+
}
1218+
}
1219+
}
1220+
}
1221+
11241222
impl<'tcx> EvalContextPrivExt<'tcx> for MiriInterpCx<'tcx> {}
11251223
trait EvalContextPrivExt<'tcx>: MiriInterpCxExt<'tcx> {
11261224
/// Temporarily allow data-races to occur. This should only be used in

src/concurrency/thread.rs

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -530,7 +530,9 @@ impl<'tcx> ThreadManager<'tcx> {
530530
}
531531

532532
/// Mutably borrow the stack of the active thread.
533-
fn active_thread_stack_mut(&mut self) -> &mut Vec<Frame<'tcx, Provenance, FrameExtra<'tcx>>> {
533+
pub fn active_thread_stack_mut(
534+
&mut self,
535+
) -> &mut Vec<Frame<'tcx, Provenance, FrameExtra<'tcx>>> {
534536
&mut self.threads[self.active_thread].stack
535537
}
536538
pub fn all_stacks(

src/concurrency/vector_clock.rs

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -130,13 +130,19 @@ impl Ord for VTimestamp {
130130
/// also this means that there is only one unique valid length
131131
/// for each set of vector clock values and hence the PartialEq
132132
/// and Eq derivations are correct.
133+
///
134+
/// This means we cannot represent a clock where the last entry is a timestamp-0 read that occurs
135+
/// because of a retag. That's fine, all it does is risk wrong diagnostics in a extreme corner case.
133136
#[derive(PartialEq, Eq, Default, Debug)]
134137
pub struct VClock(SmallVec<[VTimestamp; SMALL_VECTOR]>);
135138

136139
impl VClock {
137140
/// Create a new vector-clock containing all zeros except
138141
/// for a value at the given index
139142
pub(super) fn new_with_index(index: VectorIdx, timestamp: VTimestamp) -> VClock {
143+
if timestamp.time() == 0 {
144+
return VClock::default();
145+
}
140146
let len = index.index() + 1;
141147
let mut vec = smallvec::smallvec![VTimestamp::ZERO; len];
142148
vec[index.index()] = timestamp;

src/machine.rs

Lines changed: 50 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -81,24 +81,42 @@ pub struct FrameExtra<'tcx> {
8181
/// an additional bit of "salt" into the cache key. This salt is fixed per-frame
8282
/// so that within a call, a const will have a stable address.
8383
salt: usize,
84+
85+
/// Data race detector per-frame data.
86+
pub data_race: Option<data_race::FrameState>,
8487
}
8588

8689
impl<'tcx> std::fmt::Debug for FrameExtra<'tcx> {
8790
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
8891
// Omitting `timing`, it does not support `Debug`.
89-
let FrameExtra { borrow_tracker, catch_unwind, timing: _, is_user_relevant: _, salt: _ } =
90-
self;
92+
let FrameExtra {
93+
borrow_tracker,
94+
catch_unwind,
95+
timing: _,
96+
is_user_relevant,
97+
salt,
98+
data_race,
99+
} = self;
91100
f.debug_struct("FrameData")
92101
.field("borrow_tracker", borrow_tracker)
93102
.field("catch_unwind", catch_unwind)
103+
.field("is_user_relevant", is_user_relevant)
104+
.field("salt", salt)
105+
.field("data_race", data_race)
94106
.finish()
95107
}
96108
}
97109

98110
impl VisitProvenance for FrameExtra<'_> {
99111
fn visit_provenance(&self, visit: &mut VisitWith<'_>) {
100-
let FrameExtra { catch_unwind, borrow_tracker, timing: _, is_user_relevant: _, salt: _ } =
101-
self;
112+
let FrameExtra {
113+
catch_unwind,
114+
borrow_tracker,
115+
timing: _,
116+
is_user_relevant: _,
117+
salt: _,
118+
data_race: _,
119+
} = self;
102120

103121
catch_unwind.visit_provenance(visit);
104122
borrow_tracker.visit_provenance(visit);
@@ -1446,6 +1464,7 @@ impl<'tcx> Machine<'tcx> for MiriMachine<'tcx> {
14461464
timing,
14471465
is_user_relevant: ecx.machine.is_user_relevant(&frame),
14481466
salt: ecx.machine.rng.borrow_mut().gen::<usize>() % ADDRS_PER_ANON_GLOBAL,
1467+
data_race: ecx.machine.data_race.as_ref().map(|_| data_race::FrameState::default()),
14491468
};
14501469

14511470
Ok(frame.with_extra(extra))
@@ -1551,17 +1570,43 @@ impl<'tcx> Machine<'tcx> for MiriMachine<'tcx> {
15511570
res
15521571
}
15531572

1554-
fn after_local_allocated(
1573+
fn after_local_read(ecx: &InterpCx<'tcx, Self>, local: mir::Local) -> InterpResult<'tcx> {
1574+
if let Some(data_race) = &ecx.frame().extra.data_race {
1575+
data_race.local_read(local, &ecx.machine);
1576+
}
1577+
Ok(())
1578+
}
1579+
1580+
fn after_local_write(
1581+
ecx: &mut InterpCx<'tcx, Self>,
1582+
local: mir::Local,
1583+
storage_live: bool,
1584+
) -> InterpResult<'tcx> {
1585+
if let Some(data_race) = &ecx.frame().extra.data_race {
1586+
data_race.local_write(local, storage_live, &ecx.machine);
1587+
}
1588+
Ok(())
1589+
}
1590+
1591+
fn after_local_moved_to_memory(
15551592
ecx: &mut InterpCx<'tcx, Self>,
15561593
local: mir::Local,
15571594
mplace: &MPlaceTy<'tcx>,
15581595
) -> InterpResult<'tcx> {
15591596
let Some(Provenance::Concrete { alloc_id, .. }) = mplace.ptr().provenance else {
15601597
panic!("after_local_allocated should only be called on fresh allocations");
15611598
};
1599+
// Record the span where this was allocated: the declaration of the local.
15621600
let local_decl = &ecx.frame().body().local_decls[local];
15631601
let span = local_decl.source_info.span;
15641602
ecx.machine.allocation_spans.borrow_mut().insert(alloc_id, (span, None));
1603+
// The data race system has to fix the clocks used for this write.
1604+
let (alloc_info, machine) = ecx.get_alloc_extra_mut(alloc_id)?;
1605+
if let Some(data_race) =
1606+
&machine.threads.active_thread_stack().last().unwrap().extra.data_race
1607+
{
1608+
data_race.local_moved_to_memory(local, alloc_info.data_race.as_mut().unwrap(), machine);
1609+
}
15651610
Ok(())
15661611
}
15671612

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
1+
//@compile-flags: -Zmiri-preemption-rate=0.0 -Zmiri-disable-weak-memory-emulation
2+
#![feature(core_intrinsics)]
3+
#![feature(custom_mir)]
4+
5+
use std::intrinsics::mir::*;
6+
use std::sync::atomic::Ordering::*;
7+
use std::sync::atomic::*;
8+
use std::thread::JoinHandle;
9+
10+
static P: AtomicPtr<u8> = AtomicPtr::new(core::ptr::null_mut());
11+
12+
fn spawn_thread() -> JoinHandle<()> {
13+
std::thread::spawn(|| {
14+
while P.load(Relaxed).is_null() {
15+
std::hint::spin_loop();
16+
}
17+
unsafe {
18+
// Initialize `*P`.
19+
let ptr = P.load(Relaxed);
20+
*ptr = 127;
21+
//~^ ERROR: Data race detected between (1) creating a new allocation on thread `main` and (2) non-atomic write on thread `unnamed-1`
22+
}
23+
})
24+
}
25+
26+
fn finish(t: JoinHandle<()>, val_ptr: *mut u8) {
27+
P.store(val_ptr, Relaxed);
28+
29+
// Wait for the thread to be done.
30+
t.join().unwrap();
31+
32+
// Read initialized value.
33+
assert_eq!(unsafe { *val_ptr }, 127);
34+
}
35+
36+
#[custom_mir(dialect = "runtime", phase = "optimized")]
37+
fn main() {
38+
mir! {
39+
let t;
40+
let val;
41+
let val_ptr;
42+
let _ret;
43+
{
44+
Call(t = spawn_thread(), ReturnTo(after_spawn), UnwindContinue())
45+
}
46+
after_spawn = {
47+
// This races with the write in the other thread.
48+
StorageLive(val);
49+
50+
val_ptr = &raw mut val;
51+
Call(_ret = finish(t, val_ptr), ReturnTo(done), UnwindContinue())
52+
}
53+
done = {
54+
Return()
55+
}
56+
}
57+
}
Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
error: Undefined Behavior: Data race detected between (1) creating a new allocation on thread `main` and (2) non-atomic write on thread `unnamed-ID` at ALLOC. (2) just happened here
2+
--> $DIR/local_variable_alloc_race.rs:LL:CC
3+
|
4+
LL | *ptr = 127;
5+
| ^^^^^^^^^^ Data race detected between (1) creating a new allocation on thread `main` and (2) non-atomic write on thread `unnamed-ID` at ALLOC. (2) just happened here
6+
|
7+
help: and (1) occurred earlier here
8+
--> $DIR/local_variable_alloc_race.rs:LL:CC
9+
|
10+
LL | StorageLive(val);
11+
| ^^^^^^^^^^^^^^^^
12+
= help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
13+
= help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
14+
= note: BACKTRACE (of the first span) on thread `unnamed-ID`:
15+
= note: inside closure at $DIR/local_variable_alloc_race.rs:LL:CC
16+
17+
note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace
18+
19+
error: aborting due to 1 previous error
20+
Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
//@compile-flags: -Zmiri-preemption-rate=0.0 -Zmiri-disable-weak-memory-emulation
2+
use std::sync::atomic::Ordering::*;
3+
use std::sync::atomic::*;
4+
5+
static P: AtomicPtr<u8> = AtomicPtr::new(core::ptr::null_mut());
6+
7+
fn main() {
8+
// Create the local variable, and initialize it.
9+
let mut val: u8 = 0;
10+
11+
let t1 = std::thread::spawn(|| {
12+
while P.load(Relaxed).is_null() {
13+
std::hint::spin_loop();
14+
}
15+
unsafe {
16+
// Initialize `*P`.
17+
let ptr = P.load(Relaxed);
18+
*ptr = 127;
19+
//~^ ERROR: Data race detected between (1) non-atomic read on thread `main` and (2) non-atomic write on thread `unnamed-1`
20+
}
21+
});
22+
23+
// This read is not ordered with the store above, and thus should be reported as a race.
24+
let _val = val;
25+
26+
// Actually generate memory for the local variable.
27+
// This is the time its value is actually written to memory.
28+
// If we just "pre-date" the write to the beginning of time (since we don't know
29+
// when it actually happened), we'd miss the UB in this test.
30+
// Also, the UB error should point at the write above, not the addr-of here.
31+
P.store(std::ptr::addr_of_mut!(val), Relaxed);
32+
33+
// Wait for the thread to be done.
34+
t1.join().unwrap();
35+
36+
// Read initialized value.
37+
assert_eq!(val, 127);
38+
}
Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
error: Undefined Behavior: Data race detected between (1) non-atomic read on thread `main` and (2) non-atomic write on thread `unnamed-ID` at ALLOC. (2) just happened here
2+
--> $DIR/local_variable_read_race.rs:LL:CC
3+
|
4+
LL | *ptr = 127;
5+
| ^^^^^^^^^^ Data race detected between (1) non-atomic read on thread `main` and (2) non-atomic write on thread `unnamed-ID` at ALLOC. (2) just happened here
6+
|
7+
help: and (1) occurred earlier here
8+
--> $DIR/local_variable_read_race.rs:LL:CC
9+
|
10+
LL | let _val = val;
11+
| ^^^
12+
= help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
13+
= help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
14+
= note: BACKTRACE (of the first span) on thread `unnamed-ID`:
15+
= note: inside closure at $DIR/local_variable_read_race.rs:LL:CC
16+
17+
note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace
18+
19+
error: aborting due to 1 previous error
20+

0 commit comments

Comments
 (0)