From 7da18cdfe3a08bca722781b1c750d72d985aa27f Mon Sep 17 00:00:00 2001
From: DrMeepster <19316085+DrMeepster@users.noreply.github.com>
Date: Sun, 30 Oct 2022 21:39:16 -0700
Subject: [PATCH 1/3] add acquire when init once is already complete
---
src/concurrency/init_once.rs | 35 +++++++++----------
src/shims/windows/sync.rs | 6 ++--
tests/pass/concurrency/windows_init_once.rs | 38 +++++++++++++++++++++
3 files changed, 59 insertions(+), 20 deletions(-)
diff --git a/src/concurrency/init_once.rs b/src/concurrency/init_once.rs
index 791931901e..b1443662e2 100644
--- a/src/concurrency/init_once.rs
+++ b/src/concurrency/init_once.rs
@@ -141,18 +141,11 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
// Wake up everyone.
// need to take the queue to avoid having `this` be borrowed multiple times
for waiter in std::mem::take(&mut init_once.waiters) {
- // End of the wait happens-before woken-up thread.
- if let Some(data_race) = &this.machine.data_race {
- data_race.validate_lock_acquire(
- &this.machine.threads.sync.init_onces[id].data_race,
- waiter.thread,
- );
- }
-
this.unblock_thread(waiter.thread);
// Call callback, with the woken-up thread as `current`.
this.set_active_thread(waiter.thread);
+ this.init_once_acquire(id);
waiter.callback.call(this)?;
this.set_active_thread(current_thread);
}
@@ -172,26 +165,17 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
);
// Each complete happens-before the end of the wait
- // FIXME: should this really induce synchronization? If we think of it as a lock, then yes,
- // but the docs don't talk about such details.
if let Some(data_race) = &this.machine.data_race {
data_race.validate_lock_release(&mut init_once.data_race, current_thread);
}
// Wake up one waiting thread, so they can go ahead and try to init this.
if let Some(waiter) = init_once.waiters.pop_front() {
- // End of the wait happens-before woken-up thread.
- if let Some(data_race) = &this.machine.data_race {
- data_race.validate_lock_acquire(
- &this.machine.threads.sync.init_onces[id].data_race,
- waiter.thread,
- );
- }
-
this.unblock_thread(waiter.thread);
// Call callback, with the woken-up thread as `current`.
this.set_active_thread(waiter.thread);
+ this.init_once_acquire(id);
waiter.callback.call(this)?;
this.set_active_thread(current_thread);
} else {
@@ -201,4 +185,19 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
Ok(())
}
+
+ /// Synchronize with the previous completion or failure of an InitOnce.
+ /// This is required to prevent data races.
+ #[inline]
+ fn init_once_acquire(&mut self, id: InitOnceId) {
+ let this = self.eval_context_mut();
+ let current_thread = this.get_active_thread();
+
+ if let Some(data_race) = &this.machine.data_race {
+ data_race.validate_lock_acquire(
+ &this.machine.threads.sync.init_onces[id].data_race,
+ current_thread,
+ );
+ }
+ }
}
diff --git a/src/shims/windows/sync.rs b/src/shims/windows/sync.rs
index 8156ae8af1..f8980e188b 100644
--- a/src/shims/windows/sync.rs
+++ b/src/shims/windows/sync.rs
@@ -177,8 +177,10 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
Box::new(Callback { init_once_id: id, pending_place }),
)
}
- InitOnceStatus::Complete =>
- this.write_scalar(this.eval_windows("c", "FALSE")?, &pending_place)?,
+ InitOnceStatus::Complete => {
+ this.init_once_acquire(id);
+ this.write_scalar(this.eval_windows("c", "FALSE")?, &pending_place)?;
+ }
}
// This always succeeds (even if the thread is blocked, we will succeed if we ever unblock).
diff --git a/tests/pass/concurrency/windows_init_once.rs b/tests/pass/concurrency/windows_init_once.rs
index d3c72c3d02..6e5129acaf 100644
--- a/tests/pass/concurrency/windows_init_once.rs
+++ b/tests/pass/concurrency/windows_init_once.rs
@@ -131,8 +131,46 @@ fn retry_on_fail() {
waiter2.join().unwrap();
}
+fn no_data_race_after_complete() {
+ let mut init_once = null_mut();
+ let mut pending = 0;
+
+ unsafe {
+ assert_eq!(InitOnceBeginInitialize(&mut init_once, 0, &mut pending, null_mut()), TRUE);
+ assert_eq!(pending, TRUE);
+ }
+
+ let init_once_ptr = SendPtr(&mut init_once);
+
+ let mut place = 0;
+ let place_ptr = SendPtr(&mut place);
+
+ let reader = thread::spawn(move || unsafe {
+ let mut pending = 0;
+
+ assert_eq!(InitOnceBeginInitialize(init_once_ptr.0, 0, &mut pending, null_mut()), TRUE);
+ assert_eq!(pending, FALSE);
+ // this should not data race
+ place_ptr.0.read()
+ });
+
+ unsafe {
+ // this should not data race
+ place_ptr.0.write(1);
+ }
+
+ unsafe {
+ assert_eq!(InitOnceComplete(init_once_ptr.0, 0, null_mut()), TRUE);
+ }
+ //println!("complete");
+
+ // run reader
+ assert_eq!(reader.join().unwrap(), 1);
+}
+
fn main() {
single_thread();
block_until_complete();
retry_on_fail();
+ no_data_race_after_complete();
}
From 2b5b4e0f78282018fd0ea888ccf390255cb061db Mon Sep 17 00:00:00 2001
From: DrMeepster <19316085+DrMeepster@users.noreply.github.com>
Date: Thu, 3 Nov 2022 18:13:53 -0700
Subject: [PATCH 2/3] refactor into private functions
---
src/concurrency/init_once.rs | 75 ++++++++++++++++++++++++------------
src/shims/windows/sync.rs | 2 +-
2 files changed, 51 insertions(+), 26 deletions(-)
diff --git a/src/concurrency/init_once.rs b/src/concurrency/init_once.rs
index b1443662e2..eb42cdf80a 100644
--- a/src/concurrency/init_once.rs
+++ b/src/concurrency/init_once.rs
@@ -3,7 +3,7 @@ use std::num::NonZeroU32;
use rustc_index::vec::Idx;
-use super::sync::EvalContextExtPriv;
+use super::sync::EvalContextExtPriv as _;
use super::thread::MachineCallback;
use super::vector_clock::VClock;
use crate::*;
@@ -52,6 +52,43 @@ impl<'mir, 'tcx> VisitTags for InitOnce<'mir, 'tcx> {
}
}
+impl<'mir, 'tcx: 'mir> EvalContextExtPriv<'mir, 'tcx> for crate::MiriInterpCx<'mir, 'tcx> {}
+trait EvalContextExtPriv<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
+ /// Synchronize with the previous initialization attempt of an InitOnce.
+ #[inline]
+ fn init_once_observe_attempt(&mut self, id: InitOnceId) {
+ let this = self.eval_context_mut();
+ let current_thread = this.get_active_thread();
+
+ if let Some(data_race) = &this.machine.data_race {
+ data_race.validate_lock_acquire(
+ &this.machine.threads.sync.init_onces[id].data_race,
+ current_thread,
+ );
+ }
+ }
+
+ #[inline]
+ fn init_once_wake_waiter(
+ &mut self,
+ id: InitOnceId,
+ waiter: InitOnceWaiter<'mir, 'tcx>,
+ ) -> InterpResult<'tcx> {
+ let this = self.eval_context_mut();
+ let current_thread = this.get_active_thread();
+
+ this.unblock_thread(waiter.thread);
+
+ // Call callback, with the woken-up thread as `current`.
+ this.set_active_thread(waiter.thread);
+ this.init_once_observe_attempt(id);
+ waiter.callback.call(this)?;
+ this.set_active_thread(current_thread);
+
+ Ok(())
+ }
+}
+
impl<'mir, 'tcx: 'mir> EvalContextExt<'mir, 'tcx> for crate::MiriInterpCx<'mir, 'tcx> {}
pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
fn init_once_get_or_create_id(
@@ -141,13 +178,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
// Wake up everyone.
// need to take the queue to avoid having `this` be borrowed multiple times
for waiter in std::mem::take(&mut init_once.waiters) {
- this.unblock_thread(waiter.thread);
-
- // Call callback, with the woken-up thread as `current`.
- this.set_active_thread(waiter.thread);
- this.init_once_acquire(id);
- waiter.callback.call(this)?;
- this.set_active_thread(current_thread);
+ this.init_once_wake_waiter(id, waiter)?;
}
Ok(())
@@ -171,13 +202,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
// Wake up one waiting thread, so they can go ahead and try to init this.
if let Some(waiter) = init_once.waiters.pop_front() {
- this.unblock_thread(waiter.thread);
-
- // Call callback, with the woken-up thread as `current`.
- this.set_active_thread(waiter.thread);
- this.init_once_acquire(id);
- waiter.callback.call(this)?;
- this.set_active_thread(current_thread);
+ this.init_once_wake_waiter(id, waiter)?;
} else {
// Nobody there to take this, so go back to 'uninit'
init_once.status = InitOnceStatus::Uninitialized;
@@ -186,18 +211,18 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
Ok(())
}
- /// Synchronize with the previous completion or failure of an InitOnce.
- /// This is required to prevent data races.
+ /// Synchronize with the previous completion of an InitOnce.
+ /// Must only be called after checking that it is complete.
#[inline]
- fn init_once_acquire(&mut self, id: InitOnceId) {
+ fn init_once_observe_completed(&mut self, id: InitOnceId) {
let this = self.eval_context_mut();
- let current_thread = this.get_active_thread();
- if let Some(data_race) = &this.machine.data_race {
- data_race.validate_lock_acquire(
- &this.machine.threads.sync.init_onces[id].data_race,
- current_thread,
- );
- }
+ assert_eq!(
+ this.init_once_status(id),
+ InitOnceStatus::Complete,
+ "observing the completion of incomplete init once"
+ );
+
+ this.init_once_observe_attempt(id);
}
}
diff --git a/src/shims/windows/sync.rs b/src/shims/windows/sync.rs
index f8980e188b..098804626f 100644
--- a/src/shims/windows/sync.rs
+++ b/src/shims/windows/sync.rs
@@ -178,7 +178,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
)
}
InitOnceStatus::Complete => {
- this.init_once_acquire(id);
+ this.init_once_observe_completed(id);
this.write_scalar(this.eval_windows("c", "FALSE")?, &pending_place)?;
}
}
From bc05e6be8cab4902d95a1ebe14d2da5187678f93 Mon Sep 17 00:00:00 2001
From: DrMeepster <19316085+DrMeepster@users.noreply.github.com>
Date: Thu, 3 Nov 2022 18:30:04 -0700
Subject: [PATCH 3/3] clarify no_data_race_after_complete test
---
tests/pass/concurrency/windows_init_once.rs | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tests/pass/concurrency/windows_init_once.rs b/tests/pass/concurrency/windows_init_once.rs
index 6e5129acaf..4eb8837962 100644
--- a/tests/pass/concurrency/windows_init_once.rs
+++ b/tests/pass/concurrency/windows_init_once.rs
@@ -148,6 +148,7 @@ fn no_data_race_after_complete() {
let reader = thread::spawn(move || unsafe {
let mut pending = 0;
+ // this doesn't block because reader only executes after `InitOnceComplete` is called
assert_eq!(InitOnceBeginInitialize(init_once_ptr.0, 0, &mut pending, null_mut()), TRUE);
assert_eq!(pending, FALSE);
// this should not data race
@@ -162,9 +163,8 @@ fn no_data_race_after_complete() {
unsafe {
assert_eq!(InitOnceComplete(init_once_ptr.0, 0, null_mut()), TRUE);
}
- //println!("complete");
- // run reader
+ // run reader (without preemption, it has not taken a step yet)
assert_eq!(reader.join().unwrap(), 1);
}