From 044a068c672cf8edae2cd9d5032995f37f1c3718 Mon Sep 17 00:00:00 2001 From: Vytautas Astrauskas Date: Thu, 30 Apr 2020 14:07:07 -0700 Subject: [PATCH] Improve code readability and comments. --- src/eval.rs | 4 ++-- src/shims/sync.rs | 12 +++++----- src/sync.rs | 15 +++++++----- src/thread.rs | 60 +++++++++++++++++++++++------------------------ 4 files changed, 47 insertions(+), 44 deletions(-) diff --git a/src/eval.rs b/src/eval.rs index 30901a8f127f..7a6c562e7ca0 100644 --- a/src/eval.rs +++ b/src/eval.rs @@ -210,11 +210,11 @@ pub fn eval_main<'tcx>(tcx: TyCtxt<'tcx>, main_id: DefId, config: MiriConfig) -> SchedulingAction::ExecuteStep => { assert!(ecx.step()?, "a terminated thread was scheduled for execution"); } - SchedulingAction::ExecuteCallback => { + SchedulingAction::ExecuteTimeoutCallback => { assert!(ecx.machine.communicate, "scheduler callbacks require disabled isolation, but the code \ that created the callback did not check it"); - ecx.run_scheduler_callback()?; + ecx.run_timeout_callback()?; } SchedulingAction::ExecuteDtors => { // This will either enable the thread again (so we go back diff --git a/src/shims/sync.rs b/src/shims/sync.rs index dfd7999457eb..f31efe18e1c1 100644 --- a/src/shims/sync.rs +++ b/src/shims/sync.rs @@ -128,7 +128,7 @@ fn mutex_get_or_create_id<'mir, 'tcx: 'mir>( mutex_set_id(ecx, mutex_op, id.to_u32_scalar())?; Ok(id) } else { - Ok(id.into()) + Ok(MutexId::from_u32(id)) } } @@ -168,7 +168,7 @@ fn rwlock_get_or_create_id<'mir, 'tcx: 'mir>( rwlock_set_id(ecx, rwlock_op, id.to_u32_scalar())?; Ok(id) } else { - Ok(id.into()) + Ok(RwLockId::from_u32(id)) } } @@ -232,7 +232,7 @@ fn cond_get_or_create_id<'mir, 'tcx: 'mir>( cond_set_id(ecx, cond_op, id.to_u32_scalar())?; Ok(id) } else { - Ok(id.into()) + Ok(CondvarId::from_u32(id)) } } @@ -656,7 +656,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let id = cond_get_or_create_id(this, cond_op)?; if let Some((thread, mutex)) = this.condvar_signal(id) { reacquire_cond_mutex(this, thread, mutex)?; - this.unregister_callback_if_exists(thread)?; + this.unregister_timeout_callback_if_exists(thread)?; } Ok(0) @@ -668,7 +668,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx while let Some((thread, mutex)) = this.condvar_signal(id) { reacquire_cond_mutex(this, thread, mutex)?; - this.unregister_callback_if_exists(thread)?; + this.unregister_timeout_callback_if_exists(thread)?; } Ok(0) @@ -739,7 +739,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx }; // Register the timeout callback. - this.register_callback( + this.register_timeout_callback( active_thread, timeout_time, Box::new(move |ecx| { diff --git a/src/sync.rs b/src/sync.rs index 5d181692fb2a..88b5d6c060dd 100644 --- a/src/sync.rs +++ b/src/sync.rs @@ -9,9 +9,18 @@ use crate::*; macro_rules! declare_id { ($name: ident) => { + /// 0 is used to indicate that the id was not yet assigned and, + /// therefore, is not a valid identifier. #[derive(Clone, Copy, Debug, PartialOrd, Ord, PartialEq, Eq, Hash)] pub struct $name(NonZeroU32); + impl $name { + // Panics if `id == 0`. + pub fn from_u32(id: u32) -> Self { + Self(NonZeroU32::new(id).unwrap()) + } + } + impl Idx for $name { fn new(idx: usize) -> Self { $name(NonZeroU32::new(u32::try_from(idx).unwrap() + 1).unwrap()) @@ -21,12 +30,6 @@ macro_rules! declare_id { } } - impl From for $name { - fn from(id: u32) -> Self { - Self(NonZeroU32::new(id).unwrap()) - } - } - impl $name { pub fn to_u32_scalar<'tcx>(&self) -> Scalar { Scalar::from_u32(self.0.get()) diff --git a/src/thread.rs b/src/thread.rs index 856468705d30..f67de48b710d 100644 --- a/src/thread.rs +++ b/src/thread.rs @@ -24,15 +24,17 @@ use crate::*; pub enum SchedulingAction { /// Execute step on the active thread. ExecuteStep, - /// Execute a scheduler's callback. - ExecuteCallback, + /// Execute a timeout callback. + ExecuteTimeoutCallback, /// Execute destructors of the active thread. ExecuteDtors, /// Stop the program. Stop, } -type EventCallback<'mir, 'tcx> = +/// Timeout timeout_callbacks can be created by synchronization primitives to tell the +/// scheduler that they should be called once some period of time passes. +type TimeoutCallback<'mir, 'tcx> = Box>) -> InterpResult<'tcx> + 'tcx>; /// A thread identifier. @@ -161,14 +163,14 @@ impl<'mir, 'tcx> Default for Thread<'mir, 'tcx> { /// conditional variable with a timeout creates a callback that is called after /// the specified time and unblocks the thread. If another thread signals on the /// conditional variable, the signal handler deletes the callback. -struct CallBackInfo<'mir, 'tcx> { +struct TimeoutCallbackInfo<'mir, 'tcx> { /// The callback should be called no earlier than this time. call_time: Instant, /// The called function. - callback: EventCallback<'mir, 'tcx>, + callback: TimeoutCallback<'mir, 'tcx>, } -impl<'mir, 'tcx> std::fmt::Debug for CallBackInfo<'mir, 'tcx> { +impl<'mir, 'tcx> std::fmt::Debug for TimeoutCallbackInfo<'mir, 'tcx> { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { write!(f, "CallBack({:?})", self.call_time) } @@ -183,17 +185,16 @@ pub struct ThreadManager<'mir, 'tcx> { /// /// Note that this vector also contains terminated threads. threads: IndexVec>, - /// FIXME: make private. + /// This field is pub(crate) because the synchronization primitives + /// (`crate::sync`) need a way to access it. pub(crate) sync: SynchronizationState, - /// A counter used to generate unique identifiers for blocksets. - blockset_counter: u32, /// A mapping from a thread-local static to an allocation id of a thread /// specific allocation. thread_local_alloc_ids: RefCell>, /// A flag that indicates that we should change the active thread. yield_active_thread: bool, /// Callbacks that are called once the specified time passes. - callbacks: FxHashMap>, + timeout_callbacks: FxHashMap>, } impl<'mir, 'tcx> Default for ThreadManager<'mir, 'tcx> { @@ -208,10 +209,9 @@ impl<'mir, 'tcx> Default for ThreadManager<'mir, 'tcx> { active_thread: ThreadId::new(0), threads: threads, sync: SynchronizationState::default(), - blockset_counter: 0, thread_local_alloc_ids: Default::default(), yield_active_thread: false, - callbacks: FxHashMap::default(), + timeout_callbacks: FxHashMap::default(), } } } @@ -359,28 +359,28 @@ impl<'mir, 'tcx: 'mir> ThreadManager<'mir, 'tcx> { } /// Register the given `callback` to be called once the `call_time` passes. - fn register_callback( + fn register_timeout_callback( &mut self, thread: ThreadId, call_time: Instant, - callback: EventCallback<'mir, 'tcx>, + callback: TimeoutCallback<'mir, 'tcx>, ) { - self.callbacks - .insert(thread, CallBackInfo { call_time: call_time, callback: callback }) + self.timeout_callbacks + .insert(thread, TimeoutCallbackInfo { call_time: call_time, callback: callback }) .unwrap_none(); } /// Unregister the callback for the `thread`. - fn unregister_callback_if_exists(&mut self, thread: ThreadId) { - self.callbacks.remove(&thread); + fn unregister_timeout_callback_if_exists(&mut self, thread: ThreadId) { + self.timeout_callbacks.remove(&thread); } /// Get a callback that is ready to be called. - fn get_callback(&mut self) -> Option<(ThreadId, EventCallback<'mir, 'tcx>)> { + fn get_callback(&mut self) -> Option<(ThreadId, TimeoutCallback<'mir, 'tcx>)> { let current_time = Instant::now(); // We use a for loop here to make the scheduler more deterministic. for thread in self.threads.indices() { - match self.callbacks.entry(thread) { + match self.timeout_callbacks.entry(thread) { Entry::Occupied(entry) => if current_time >= entry.get().call_time { return Some((thread, entry.remove().callback)); @@ -447,17 +447,17 @@ impl<'mir, 'tcx: 'mir> ThreadManager<'mir, 'tcx> { if self.threads.iter().all(|thread| thread.state == ThreadState::Terminated) { unreachable!(); } else if let Some(next_call_time) = - self.callbacks.values().min_by_key(|info| info.call_time) + self.timeout_callbacks.values().min_by_key(|info| info.call_time) { // All threads are currently blocked, but we have unexecuted - // callbacks, which may unblock some of the threads. Hence, + // timeout_callbacks, which may unblock some of the threads. Hence, // sleep until the first callback. if let Some(sleep_time) = next_call_time.call_time.checked_duration_since(Instant::now()) { std::thread::sleep(sleep_time); } - Ok(SchedulingAction::ExecuteCallback) + Ok(SchedulingAction::ExecuteTimeoutCallback) } else { throw_machine_stop!(TerminationInfo::Deadlock); } @@ -647,27 +647,27 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx } #[inline] - fn register_callback( + fn register_timeout_callback( &mut self, thread: ThreadId, call_time: Instant, - callback: EventCallback<'mir, 'tcx>, + callback: TimeoutCallback<'mir, 'tcx>, ) -> InterpResult<'tcx> { let this = self.eval_context_mut(); - this.machine.threads.register_callback(thread, call_time, callback); + this.machine.threads.register_timeout_callback(thread, call_time, callback); Ok(()) } #[inline] - fn unregister_callback_if_exists(&mut self, thread: ThreadId) -> InterpResult<'tcx> { + fn unregister_timeout_callback_if_exists(&mut self, thread: ThreadId) -> InterpResult<'tcx> { let this = self.eval_context_mut(); - this.machine.threads.unregister_callback_if_exists(thread); + this.machine.threads.unregister_timeout_callback_if_exists(thread); Ok(()) } - /// Execute the callback on the callback's thread. + /// Execute a timeout callback on the callback's thread. #[inline] - fn run_scheduler_callback(&mut self) -> InterpResult<'tcx> { + fn run_timeout_callback(&mut self) -> InterpResult<'tcx> { let this = self.eval_context_mut(); let (thread, callback) = this.machine.threads.get_callback().expect("no callback found"); let old_thread = this.set_active_thread(thread)?;