Skip to content

Commit 4e9554d

Browse files
RalfJungRalfJung
committed
fix behavior of release_clock()
1 parent ee491b3 commit 4e9554d

File tree

9 files changed

+90
-30
lines changed

9 files changed

+90
-30
lines changed

‎src/tools/miri/src/alloc_addresses/mod.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -453,7 +453,7 @@ impl<'tcx> MiriMachine<'tcx> {
453453
let thread = self.threads.active_thread();
454454
global_state.reuse.add_addr(rng, addr, size, align, kind, thread, || {
455455
if let Some(data_race) = &self.data_race {
456-
data_race.release_clock(&self.threads).clone()
456+
data_race.release_clock(&self.threads, |clock| clock.clone())
457457
} else {
458458
VClock::default()
459459
}

‎src/tools/miri/src/concurrency/data_race.rs‎

Lines changed: 17 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -828,15 +828,14 @@ pub trait EvalContextExt<'tcx>: MiriInterpCxExt<'tcx> {
828828
}
829829
}
830830

831-
/// Returns the `release` clock of the current thread.
831+
/// Calls the callback with the "release" clock of the current thread.
832832
/// Other threads can acquire this clock in the future to establish synchronization
833833
/// with this program point.
834-
fn release_clock<'a>(&'a self) -> Option<Ref<'a, VClock>>
835-
where
836-
'tcx: 'a,
837-
{
834+
///
835+
/// The closure will only be invoked if data race handling is on.
836+
fn release_clock<R>(&self, callback: impl FnOnce(&VClock) -> R) -> Option<R> {
838837
let this = self.eval_context_ref();
839-
Some(this.machine.data_race.as_ref()?.release_clock(&this.machine.threads))
838+
Some(this.machine.data_race.as_ref()?.release_clock(&this.machine.threads, callback))
840839
}
841840

842841
/// Acquire the given clock into the current thread, establishing synchronization with
@@ -1728,7 +1727,7 @@ impl GlobalState {
17281727
let current_index = self.active_thread_index(thread_mgr);
17291728

17301729
// Store the terminaion clock.
1731-
let terminaion_clock = self.release_clock(thread_mgr).clone();
1730+
let terminaion_clock = self.release_clock(thread_mgr, |clock| clock.clone());
17321731
self.thread_info.get_mut()[current_thread].termination_vector_clock =
17331732
Some(terminaion_clock);
17341733

@@ -1778,21 +1777,23 @@ impl GlobalState {
17781777
clocks.clock.join(clock);
17791778
}
17801779

1781-
/// Returns the `release` clock of the current thread.
1780+
/// Calls the given closure with the "release" clock of the current thread.
17821781
/// Other threads can acquire this clock in the future to establish synchronization
17831782
/// with this program point.
1784-
pub fn release_clock<'tcx>(&self, threads: &ThreadManager<'tcx>) -> Ref<'_, VClock> {
1783+
pub fn release_clock<'tcx, R>(
1784+
&self,
1785+
threads: &ThreadManager<'tcx>,
1786+
callback: impl FnOnce(&VClock) -> R,
1787+
) -> R {
17851788
let thread = threads.active_thread();
17861789
let span = threads.active_thread_ref().current_span();
1787-
// We increment the clock each time this happens, to ensure no two releases
1788-
// can be confused with each other.
17891790
let (index, mut clocks) = self.thread_state_mut(thread);
1791+
let r = callback(&clocks.clock);
1792+
// Increment the clock, so that all following events cannot be confused with anything that
1793+
// occurred before the release. Crucially, the callback is invoked on the *old* clock!
17901794
clocks.increment_clock(index, span);
1791-
drop(clocks);
1792-
// To return a read-only view, we need to release the RefCell
1793-
// and borrow it again.
1794-
let (_index, clocks) = self.thread_state(thread);
1795-
Ref::map(clocks, |c| &c.clock)
1795+
1796+
r
17961797
}
17971798

17981799
fn thread_index(&self, thread: ThreadId) -> VectorIdx {

‎src/tools/miri/src/concurrency/init_once.rs‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,8 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
9393

9494
// Each complete happens-before the end of the wait
9595
if let Some(data_race) = &this.machine.data_race {
96-
init_once.clock.clone_from(&data_race.release_clock(&this.machine.threads));
96+
data_race
97+
.release_clock(&this.machine.threads, |clock| init_once.clock.clone_from(clock));
9798
}
9899

99100
// Wake up everyone.
@@ -119,7 +120,8 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
119120

120121
// Each complete happens-before the end of the wait
121122
if let Some(data_race) = &this.machine.data_race {
122-
init_once.clock.clone_from(&data_race.release_clock(&this.machine.threads));
123+
data_race
124+
.release_clock(&this.machine.threads, |clock| init_once.clock.clone_from(clock));
123125
}
124126

125127
// Wake up one waiting thread, so they can go ahead and try to init this.

‎src/tools/miri/src/concurrency/sync.rs‎

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -444,7 +444,9 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
444444
// The mutex is completely unlocked. Try transferring ownership
445445
// to another thread.
446446
if let Some(data_race) = &this.machine.data_race {
447-
mutex.clock.clone_from(&data_race.release_clock(&this.machine.threads));
447+
data_race.release_clock(&this.machine.threads, |clock| {
448+
mutex.clock.clone_from(clock)
449+
});
448450
}
449451
if let Some(thread) = this.machine.sync.mutexes[id].queue.pop_front() {
450452
this.unblock_thread(thread, BlockReason::Mutex(id))?;
@@ -553,7 +555,9 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
553555
}
554556
if let Some(data_race) = &this.machine.data_race {
555557
// Add this to the shared-release clock of all concurrent readers.
556-
rwlock.clock_current_readers.join(&data_race.release_clock(&this.machine.threads));
558+
data_race.release_clock(&this.machine.threads, |clock| {
559+
rwlock.clock_current_readers.join(clock)
560+
});
557561
}
558562

559563
// The thread was a reader. If the lock is not held any more, give it to a writer.
@@ -632,7 +636,9 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
632636
trace!("rwlock_writer_unlock: {:?} unlocked by {:?}", id, thread);
633637
// Record release clock for next lock holder.
634638
if let Some(data_race) = &this.machine.data_race {
635-
rwlock.clock_unlocked.clone_from(&*data_race.release_clock(&this.machine.threads));
639+
data_race.release_clock(&this.machine.threads, |clock| {
640+
rwlock.clock_unlocked.clone_from(clock)
641+
});
636642
}
637643
// The thread was a writer.
638644
//
@@ -764,7 +770,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
764770

765771
// Each condvar signal happens-before the end of the condvar wake
766772
if let Some(data_race) = data_race {
767-
condvar.clock.clone_from(&*data_race.release_clock(&this.machine.threads));
773+
data_race.release_clock(&this.machine.threads, |clock| condvar.clock.clone_from(clock));
768774
}
769775
let Some(waiter) = condvar.waiters.pop_front() else {
770776
return interp_ok(false);
@@ -837,7 +843,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
837843

838844
// Each futex-wake happens-before the end of the futex wait
839845
if let Some(data_race) = data_race {
840-
futex.clock.clone_from(&*data_race.release_clock(&this.machine.threads));
846+
data_race.release_clock(&this.machine.threads, |clock| futex.clock.clone_from(clock));
841847
}
842848

843849
// Wake up the first thread in the queue that matches any of the bits in the bitset.

‎src/tools/miri/src/shims/unix/linux/epoll.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -568,9 +568,9 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
568568
let epoll = epfd.downcast::<Epoll>().unwrap();
569569

570570
// Synchronize running thread to the epoll ready list.
571-
if let Some(clock) = &this.release_clock() {
571+
this.release_clock(|clock| {
572572
epoll.ready_list.clock.borrow_mut().join(clock);
573-
}
573+
});
574574

575575
if let Some(thread_id) = epoll.thread_id.borrow_mut().pop() {
576576
waiter.push(thread_id);

‎src/tools/miri/src/shims/unix/linux/eventfd.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -140,9 +140,9 @@ impl FileDescription for Event {
140140
match self.counter.get().checked_add(num) {
141141
Some(new_count @ 0..=MAX_COUNTER) => {
142142
// Future `read` calls will synchronize with this write, so update the FD clock.
143-
if let Some(clock) = &ecx.release_clock() {
143+
ecx.release_clock(|clock| {
144144
self.clock.borrow_mut().join(clock);
145-
}
145+
});
146146
self.counter.set(new_count);
147147
}
148148
None | Some(u64::MAX) =>

‎src/tools/miri/src/shims/unix/unnamed_socket.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -234,9 +234,9 @@ impl FileDescription for AnonSocket {
234234
}
235235
}
236236
// Remember this clock so `read` can synchronize with us.
237-
if let Some(clock) = &ecx.release_clock() {
237+
ecx.release_clock(|clock| {
238238
writebuf.clock.join(clock);
239-
}
239+
});
240240
// Do full write / partial write based on the space available.
241241
let actual_write_size = len.min(available_space);
242242
let bytes = ecx.read_bytes_ptr_strip_provenance(ptr, Size::from_bytes(len))?;

‎src/tools/miri/tests/fail-dep/libc/socketpair-data-race.rs‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
//! This is a regression test for <https://github.com/rust-lang/miri/issues/3947>: we had some
2+
//! faulty logic around `release_clock` that led to this code not reporting a data race.
3+
//@ignore-target: windows # no libc socketpair on Windows
4+
//@compile-flags: -Zmiri-preemption-rate=0
5+
use std::thread;
6+
7+
fn main() {
8+
static mut VAL: u8 = 0;
9+
let mut fds = [-1, -1];
10+
let res = unsafe { libc::socketpair(libc::AF_UNIX, libc::SOCK_STREAM, 0, fds.as_mut_ptr()) };
11+
assert_eq!(res, 0);
12+
let thread1 = thread::spawn(move || {
13+
let data = "a".as_bytes().as_ptr();
14+
let res = unsafe { libc::write(fds[0], data as *const libc::c_void, 1) };
15+
assert_eq!(res, 1);
16+
// The write to VAL is *after* the write to the socket, so there's no proper synchronization.
17+
unsafe { VAL = 1 };
18+
});
19+
thread::yield_now();
20+
21+
let mut buf: [u8; 1] = [0; 1];
22+
let res: i32 = unsafe {
23+
libc::read(fds[1], buf.as_mut_ptr().cast(), buf.len() as libc::size_t).try_into().unwrap()
24+
};
25+
assert_eq!(res, 1);
26+
assert_eq!(buf, "a".as_bytes());
27+
28+
unsafe { assert_eq!({ VAL }, 1) }; //~ERROR: Data race
29+
30+
thread1.join().unwrap();
31+
}

‎src/tools/miri/tests/fail-dep/libc/socketpair-data-race.stderr‎

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 write on thread `unnamed-ID` and (2) non-atomic read on thread `main` at ALLOC. (2) just happened here
2+
--> tests/fail-dep/libc/socketpair-data-race.rs:LL:CC
3+
|
4+
LL | unsafe { assert_eq!({ VAL }, 1) };
5+
| ^^^ Data race detected between (1) non-atomic write on thread `unnamed-ID` and (2) non-atomic read on thread `main` at ALLOC. (2) just happened here
6+
|
7+
help: and (1) occurred earlier here
8+
--> tests/fail-dep/libc/socketpair-data-race.rs:LL:CC
9+
|
10+
LL | unsafe { VAL = 1 };
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):
15+
= note: inside `main` at tests/fail-dep/libc/socketpair-data-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)