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); }