From b933e0d1a9e3b01133a5090eca3b0db868f22715 Mon Sep 17 00:00:00 2001 From: Jon Gjengset Date: Thu, 9 Apr 2020 16:48:25 -0400 Subject: [PATCH 1/2] Add lazy_static macro This adds the `lazy_static!` macro from the `lazy_static` crate. We need to re-implement it both since it establishes causality (through its use of `Once`), and because the state needs to be cleared on every execution. This implementation does that. One unfortunate downside of this implementation is that it is inherently unsafe. Specifically, in the context of loom, statics are, well, not static. And yet the API of `lazy_static!` is such that it gives out `'static` references. We _cannot_ safely give out `'static` refs into statics that we clear on every execution. One option here is to leak every static we allocate. The code would then be safe (since the references would indeed be `'static`), but it would mean we leak _all_ statics for _every_ execution, no matter whether the user needs them to be `'static` or not. Fixes #124. --- src/lazy_static.rs | 82 +++++++++++++++++++++++++++++++++++++++++++ src/lib.rs | 54 ++++++++++++++++++++++++++++ src/rt/execution.rs | 8 ++++- src/rt/lazy_static.rs | 64 +++++++++++++++++++++++++++++++++ src/rt/mod.rs | 1 + tests/atomic.rs | 22 ++++++++++++ 6 files changed, 230 insertions(+), 1 deletion(-) create mode 100644 src/lazy_static.rs create mode 100644 src/rt/lazy_static.rs diff --git a/src/lazy_static.rs b/src/lazy_static.rs new file mode 100644 index 00000000..f70b3dac --- /dev/null +++ b/src/lazy_static.rs @@ -0,0 +1,82 @@ +//! Mock implementation of `std::thread`. + +use crate::rt; +pub use crate::rt::thread::AccessError; +pub use crate::rt::yield_now; +use crate::sync::atomic::Ordering; + +pub use std::thread::panicking; + +use std::fmt; +use std::marker::PhantomData; + +/// Mock implementation of `lazy_static::Lazy`. +pub struct Lazy { + // Sadly, these fields have to be public, since function pointers in const + // fns are unstable. When fn pointer arguments to const fns stabilize, these + // should be made private and replaced with a `const fn new`. + // + // User code should not rely on the existence of these fields. + #[doc(hidden)] + pub init: fn() -> T, + #[doc(hidden)] + pub _p: PhantomData, +} + +impl Lazy { + /// Mock implementation of `lazy_static::Lazy::get`. + pub fn get(&'static self) -> &'static T { + // This is not great. Specifically, we're returning a 'static reference to a value that + // only lives for the duration of the execution. Unfortunately, the semantics of lazy + // static is that, well, you get a value that is in fact 'static. If we did not provide + // that, then this replacement wouldn't actually work. + // + // The "upside" here is that _if_ the code compiled with `lazy_static::lazy_static!`, + // _then_ this is safe. That's not super satisfying, but I'm not sure how we do better + // without changing the API pretty drastically. We could perhaps here provide a + // `with(closure)` like we do for `UnsafeCell`, and require people to wrap the "real" + // `lazy_static` the same way, but that seems like its own kind of unfortunate as I'm sure + // users sometimes _rely_ on the returned reference being 'static. If we provided something + // that used a closure to give the user a non-`'static` reference, we wouldn't be all that + // much further along. + match unsafe { self.try_get() } { + Some(v) => v, + None => { + // Init the value out of the `rt::execution` + let mut sv = crate::rt::lazy_static::StaticValue::new((self.init)()); + + rt::execution(|execution| { + // lazy_static uses std::sync::Once, which does a swap(AcqRel) to set + sv.sync.sync_store(&mut execution.threads, Ordering::AcqRel); + + execution.lazy_statics.init_static(self, sv); + }); + + unsafe { self.try_get() }.expect("bug") + } + } + } + + unsafe fn try_get(&'static self) -> Option<&'static T> { + unsafe fn transmute_lt<'a, 'b, T>(t: &'a T) -> &'b T { + std::mem::transmute::<&'a T, &'b T>(t) + } + + let sv = rt::execution(|execution| { + let sv = execution.lazy_statics.get_static(self)?; + + // lazy_static uses std::sync::Once, which does a load(Acquire) to get + sv.sync.sync_load(&mut execution.threads, Ordering::Acquire); + + Some(transmute_lt(sv)) + })?; + + Some(sv.get::()) + } +} + +impl fmt::Debug for Lazy { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.pad("Lazy { .. }") + } +} diff --git a/src/lib.rs b/src/lib.rs index 0cc8f3d8..02750587 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -150,6 +150,7 @@ mod rt; pub mod alloc; pub mod cell; +pub mod lazy_static; pub mod model; pub mod sync; pub mod thread; @@ -189,6 +190,22 @@ macro_rules! thread_local { ); } +/// Mock version of `lazy_static::lazy_static!`. +#[macro_export] +macro_rules! lazy_static { + ($(#[$attr:meta])* static ref $N:ident : $T:ty = $e:expr; $($t:tt)*) => { + // use `()` to explicitly forward the information about private items + $crate::__lazy_static_internal!($(#[$attr])* () static ref $N : $T = $e; $($t)*); + }; + ($(#[$attr:meta])* pub static ref $N:ident : $T:ty = $e:expr; $($t:tt)*) => { + $crate::__lazy_static_internal!($(#[$attr])* (pub) static ref $N : $T = $e; $($t)*); + }; + ($(#[$attr:meta])* pub ($($vis:tt)+) static ref $N:ident : $T:ty = $e:expr; $($t:tt)*) => { + $crate::__lazy_static_internal!($(#[$attr])* (pub ($($vis)+)) static ref $N : $T = $e; $($t)*); + }; + () => () +} + #[macro_export] #[doc(hidden)] macro_rules! __thread_local_inner { @@ -200,3 +217,40 @@ macro_rules! __thread_local_inner { }; }; } + +#[macro_export] +#[doc(hidden)] +macro_rules! __lazy_static_internal { + // optional visibility restrictions are wrapped in `()` to allow for + // explicitly passing otherwise implicit information about private items + ($(#[$attr:meta])* ($($vis:tt)*) static ref $N:ident : $T:ty = $init:expr; $($t:tt)*) => { + #[allow(missing_copy_implementations)] + #[allow(non_camel_case_types)] + #[allow(dead_code)] + $(#[$attr])* + $($vis)* struct $N {__private_field: ()} + #[doc(hidden)] + $($vis)* static $N: $N = $N {__private_field: ()}; + impl ::core::ops::Deref for $N { + type Target = $T; + // this and the two __ functions below should really also be #[track_caller] + fn deref(&self) -> &$T { + #[inline(always)] + fn __static_ref_initialize() -> $T { $init } + + #[inline(always)] + fn __stability() -> &'static $T { + static LAZY: $crate::lazy_static::Lazy<$T> = + $crate::lazy_static::Lazy { + init: __static_ref_initialize, + _p: std::marker::PhantomData, + }; + LAZY.get() + } + __stability() + } + } + $crate::lazy_static!($($t)*); + }; + () => () +} diff --git a/src/rt/execution.rs b/src/rt/execution.rs index 2e23ff42..e970e653 100644 --- a/src/rt/execution.rs +++ b/src/rt/execution.rs @@ -1,5 +1,5 @@ use crate::rt::alloc::Allocation; -use crate::rt::{object, thread, Path}; +use crate::rt::{lazy_static, object, thread, Path}; use std::collections::HashMap; use std::convert::TryInto; @@ -14,6 +14,8 @@ pub(crate) struct Execution { pub(crate) threads: thread::Set, + pub(crate) lazy_statics: lazy_static::Set, + /// All loom aware objects part of this execution run. pub(super) objects: object::Store, @@ -55,6 +57,7 @@ impl Execution { id, path: Path::new(max_branches, preemption_bound), threads, + lazy_statics: lazy_static::Set::new(), objects: object::Store::with_capacity(max_branches), raw_allocations: HashMap::new(), max_threads, @@ -91,6 +94,7 @@ impl Execution { let log = self.log; let mut path = self.path; let mut objects = self.objects; + let mut lazy_statics = self.lazy_statics; let mut raw_allocations = self.raw_allocations; let mut threads = self.threads; @@ -100,6 +104,7 @@ impl Execution { } objects.clear(); + lazy_statics.clear(); raw_allocations.clear(); threads.clear(id); @@ -109,6 +114,7 @@ impl Execution { path, threads, objects, + lazy_statics, raw_allocations, max_threads, max_history, diff --git a/src/rt/lazy_static.rs b/src/rt/lazy_static.rs new file mode 100644 index 00000000..891f9c98 --- /dev/null +++ b/src/rt/lazy_static.rs @@ -0,0 +1,64 @@ +use crate::rt::synchronize::Synchronize; +use std::{any::Any, collections::HashMap}; + +pub(crate) struct Set { + /// Registered statics. + statics: HashMap, +} + +#[derive(Eq, PartialEq, Hash, Copy, Clone)] +struct StaticKeyId(usize); + +pub(crate) struct StaticValue { + pub(crate) sync: Synchronize, + v: Box, +} + +impl Set { + /// Create an empty statics set. + pub(crate) fn new() -> Set { + Set { + statics: HashMap::new(), + } + } + + pub(crate) fn clear(&mut self) { + self.statics.clear(); + } + + pub(crate) fn get_static( + &mut self, + key: &'static crate::lazy_static::Lazy, + ) -> Option<&mut StaticValue> { + self.statics.get_mut(&StaticKeyId::new(key)) + } + + pub(crate) fn init_static( + &mut self, + key: &'static crate::lazy_static::Lazy, + value: StaticValue, + ) { + assert!(self.statics.insert(StaticKeyId::new(key), value).is_none()) + } +} + +impl StaticKeyId { + fn new(key: &'static crate::lazy_static::Lazy) -> Self { + Self(key as *const _ as usize) + } +} + +impl StaticValue { + pub(crate) fn new(value: T) -> Self { + Self { + sync: Synchronize::new(), + v: Box::new(value), + } + } + + pub(crate) fn get(&self) -> &T { + self.v + .downcast_ref::() + .expect("lazy value must downcast to expected type") + } +} diff --git a/src/rt/mod.rs b/src/rt/mod.rs index 52fd55bf..dbd42bee 100644 --- a/src/rt/mod.rs +++ b/src/rt/mod.rs @@ -47,6 +47,7 @@ pub(crate) use self::scheduler::Scheduler; mod synchronize; pub(crate) use self::synchronize::Synchronize; +pub(crate) mod lazy_static; pub(crate) mod thread; mod vv; diff --git a/tests/atomic.rs b/tests/atomic.rs index 9150e0b0..4ce6b0f0 100644 --- a/tests/atomic.rs +++ b/tests/atomic.rs @@ -6,6 +6,28 @@ use loom::thread; use std::sync::atomic::Ordering::{AcqRel, Acquire, Relaxed, Release}; use std::sync::Arc; +loom::lazy_static! { + static ref A: AtomicUsize = AtomicUsize::new(0); +} + +loom::thread_local! { + static B: usize = A.load(Relaxed); +} + +#[test] +fn legal_load_after_lazy_static() { + loom::model(|| { + let t1 = thread::spawn(|| { + B.try_with(|h| *h).unwrap_or_else(|_| A.load(Relaxed)); + }); + let t2 = thread::spawn(|| { + B.try_with(|h| *h).unwrap_or_else(|_| A.load(Relaxed)); + }); + t1.join().unwrap(); + t2.join().unwrap(); + }); +} + #[test] #[should_panic] fn invalid_unsync_load_relaxed() { From 787e3498f6d24fae1de2c568b6aee13c13c66059 Mon Sep 17 00:00:00 2001 From: Jon Gjengset Date: Fri, 10 Apr 2020 16:42:28 -0400 Subject: [PATCH 2/2] Correctly model clean-up of lazy statics --- src/model.rs | 6 ++++++ src/rt/execution.rs | 2 +- src/rt/lazy_static.rs | 32 +++++++++++++++++++++++++------- tests/atomic.rs | 8 ++++++++ 4 files changed, 40 insertions(+), 8 deletions(-) diff --git a/src/model.rs b/src/model.rs index 08f863bc..0329c2f5 100644 --- a/src/model.rs +++ b/src/model.rs @@ -196,6 +196,12 @@ impl Builder { scheduler.run(&mut execution, move || { f(); + + let lazy_statics = rt::execution(|execution| execution.lazy_statics.drop()); + + // drop outside of execution + drop(lazy_statics); + rt::thread_done(); }); diff --git a/src/rt/execution.rs b/src/rt/execution.rs index e970e653..87e07bc3 100644 --- a/src/rt/execution.rs +++ b/src/rt/execution.rs @@ -104,7 +104,7 @@ impl Execution { } objects.clear(); - lazy_statics.clear(); + lazy_statics.reset(); raw_allocations.clear(); threads.clear(id); diff --git a/src/rt/lazy_static.rs b/src/rt/lazy_static.rs index 891f9c98..ba2a9887 100644 --- a/src/rt/lazy_static.rs +++ b/src/rt/lazy_static.rs @@ -3,11 +3,11 @@ use std::{any::Any, collections::HashMap}; pub(crate) struct Set { /// Registered statics. - statics: HashMap, + statics: Option>, } #[derive(Eq, PartialEq, Hash, Copy, Clone)] -struct StaticKeyId(usize); +pub(crate) struct StaticKeyId(usize); pub(crate) struct StaticValue { pub(crate) sync: Synchronize, @@ -18,19 +18,32 @@ impl Set { /// Create an empty statics set. pub(crate) fn new() -> Set { Set { - statics: HashMap::new(), + statics: Some(HashMap::new()), } } - pub(crate) fn clear(&mut self) { - self.statics.clear(); + pub(crate) fn reset(&mut self) { + assert!( + self.statics.is_none(), + "lazy_static was not dropped during execution" + ); + self.statics = Some(HashMap::new()); + } + + pub(crate) fn drop(&mut self) -> HashMap { + self.statics + .take() + .expect("lazy_statics were dropped twice in one execution") } pub(crate) fn get_static( &mut self, key: &'static crate::lazy_static::Lazy, ) -> Option<&mut StaticValue> { - self.statics.get_mut(&StaticKeyId::new(key)) + self.statics + .as_mut() + .expect("attempted to access lazy_static during shutdown") + .get_mut(&StaticKeyId::new(key)) } pub(crate) fn init_static( @@ -38,7 +51,12 @@ impl Set { key: &'static crate::lazy_static::Lazy, value: StaticValue, ) { - assert!(self.statics.insert(StaticKeyId::new(key), value).is_none()) + assert!(self + .statics + .as_mut() + .expect("attempted to access lazy_static during shutdown") + .insert(StaticKeyId::new(key), value) + .is_none()) } } diff --git a/tests/atomic.rs b/tests/atomic.rs index 4ce6b0f0..c9b6c6f6 100644 --- a/tests/atomic.rs +++ b/tests/atomic.rs @@ -8,12 +8,20 @@ use std::sync::Arc; loom::lazy_static! { static ref A: AtomicUsize = AtomicUsize::new(0); + static ref NO_LEAK: loom::sync::Arc = Default::default(); } loom::thread_local! { static B: usize = A.load(Relaxed); } +#[test] +fn lazy_static_arc_doesnt_leak() { + loom::model(|| { + assert_eq!(**NO_LEAK, 0); + }); +} + #[test] fn legal_load_after_lazy_static() { loom::model(|| {