X-Git-Url: http://git.bitcoin.ninja/index.cgi?a=blobdiff_plain;f=lightning%2Fsrc%2Fsync%2Fdebug_sync.rs;h=2b75e095380ec09f22d07d4d1bd636dd93e53c61;hb=f3db18487613b24ee8bdde25a0b753aebcdd3be6;hp=5631093723733f16f4d7447c0865f58219d1cf40;hpb=d0b8f455fe86d3e55231d35d35dd96693abe2049;p=rust-lightning diff --git a/lightning/src/sync/debug_sync.rs b/lightning/src/sync/debug_sync.rs index 56310937..2b75e095 100644 --- a/lightning/src/sync/debug_sync.rs +++ b/lightning/src/sync/debug_sync.rs @@ -12,7 +12,9 @@ use std::sync::RwLockReadGuard as StdRwLockReadGuard; use std::sync::RwLockWriteGuard as StdRwLockWriteGuard; use std::sync::Condvar as StdCondvar; -use crate::prelude::HashMap; +pub use std::sync::WaitTimeoutResult; + +use crate::prelude::*; use super::{LockTestExt, LockHeldState}; @@ -35,15 +37,19 @@ impl Condvar { Condvar { inner: StdCondvar::new() } } - pub fn wait<'a, T>(&'a self, guard: MutexGuard<'a, T>) -> LockResult> { + pub fn wait_while<'a, T, F: FnMut(&mut T) -> bool>(&'a self, guard: MutexGuard<'a, T>, condition: F) + -> LockResult> { let mutex: &'a Mutex = guard.mutex; - self.inner.wait(guard.into_inner()).map(|lock| MutexGuard { mutex, lock }).map_err(|_| ()) + self.inner.wait_while(guard.into_inner(), condition).map(|lock| MutexGuard { mutex, lock }) + .map_err(|_| ()) } #[allow(unused)] - pub fn wait_timeout<'a, T>(&'a self, guard: MutexGuard<'a, T>, dur: Duration) -> LockResult<(MutexGuard<'a, T>, ())> { + pub fn wait_timeout_while<'a, T, F: FnMut(&mut T) -> bool>(&'a self, guard: MutexGuard<'a, T>, dur: Duration, condition: F) + -> LockResult<(MutexGuard<'a, T>, WaitTimeoutResult)> { let mutex = guard.mutex; - self.inner.wait_timeout(guard.into_inner(), dur).map(|(lock, _)| (MutexGuard { mutex, lock }, ())).map_err(|_| ()) + self.inner.wait_timeout_while(guard.into_inner(), dur, condition).map_err(|_| ()) + .map(|(lock, e)| (MutexGuard { mutex, lock }, e)) } pub fn notify_all(&self) { self.inner.notify_all(); } @@ -51,7 +57,7 @@ impl Condvar { thread_local! { /// We track the set of locks currently held by a reference to their `LockMetadata` - static LOCKS_HELD: RefCell>> = RefCell::new(HashMap::new()); + static LOCKS_HELD: RefCell>> = RefCell::new(new_hash_map()); } static LOCK_IDX: AtomicUsize = AtomicUsize::new(0); @@ -74,30 +80,31 @@ struct LockDep { _lockdep_trace: Backtrace, } +// Locates the frame preceding the earliest `debug_sync` frame in the call stack. This ensures we +// can properly detect a lock's construction and acquiral callsites, since the latter may contain +// multiple `debug_sync` frames. #[cfg(feature = "backtrace")] -fn get_construction_location(backtrace: &Backtrace) -> String { - // Find the first frame that is after `debug_sync` (or that is in our tests) and use - // that as the mutex construction site. Note that the first few frames may be in - // the `backtrace` crate, so we have to ignore those. +fn locate_call_symbol(backtrace: &Backtrace) -> (String, Option) { + // Find the earliest `debug_sync` frame (or that is in our tests) and use the frame preceding it + // as the callsite. Note that the first few frames may be in the `backtrace` crate, so we have + // to ignore those. let sync_mutex_constr_regex = regex::Regex::new(r"lightning.*debug_sync").unwrap(); let mut found_debug_sync = false; - for frame in backtrace.frames() { - for symbol in frame.symbols() { - let symbol_name = symbol.name().unwrap().as_str().unwrap(); - if !sync_mutex_constr_regex.is_match(symbol_name) { - if found_debug_sync { - if let Some(col) = symbol.colno() { - return format!("{}:{}:{}", symbol.filename().unwrap().display(), symbol.lineno().unwrap(), col); - } else { - // Windows debug symbols don't support column numbers, so fall back to - // line numbers only if no `colno` is available - return format!("{}:{}", symbol.filename().unwrap().display(), symbol.lineno().unwrap()); + let mut symbol_after_latest_debug_sync = None; + for frame in backtrace.frames().iter() { + for symbol in frame.symbols().iter() { + if let Some(symbol_name) = symbol.name().map(|name| name.as_str()).flatten() { + if !sync_mutex_constr_regex.is_match(symbol_name) { + if found_debug_sync { + symbol_after_latest_debug_sync = Some(symbol); + found_debug_sync = false; } - } - } else { found_debug_sync = true; } + } else { found_debug_sync = true; } + } } } - panic!("Couldn't find mutex construction callsite"); + let symbol = symbol_after_latest_debug_sync.expect("Couldn't find lock call symbol"); + (format!("{}:{}", symbol.filename().unwrap().display(), symbol.lineno().unwrap()), symbol.colno()) } impl LockMetadata { @@ -106,52 +113,71 @@ impl LockMetadata { let lock_idx = LOCK_IDX.fetch_add(1, Ordering::Relaxed) as u64; let res = Arc::new(LockMetadata { - locked_before: StdMutex::new(HashMap::new()), + locked_before: StdMutex::new(new_hash_map()), lock_idx, _lock_construction_bt: backtrace, }); #[cfg(feature = "backtrace")] { - let lock_constr_location = get_construction_location(&res._lock_construction_bt); - LOCKS_INIT.call_once(|| { unsafe { LOCKS = Some(StdMutex::new(HashMap::new())); } }); + let (lock_constr_location, lock_constr_colno) = + locate_call_symbol(&res._lock_construction_bt); + LOCKS_INIT.call_once(|| { unsafe { LOCKS = Some(StdMutex::new(new_hash_map())); } }); let mut locks = unsafe { LOCKS.as_ref() }.unwrap().lock().unwrap(); match locks.entry(lock_constr_location) { - hash_map::Entry::Occupied(e) => return Arc::clone(e.get()), + hash_map::Entry::Occupied(e) => { + assert_eq!(lock_constr_colno, + locate_call_symbol(&e.get()._lock_construction_bt).1, + "Because Windows doesn't support column number results in backtraces, we cannot construct two mutexes on the same line or we risk lockorder detection false positives."); + return Arc::clone(e.get()) + }, hash_map::Entry::Vacant(e) => { e.insert(Arc::clone(&res)); }, } } res } - // Returns whether we were a recursive lock (only relevant for read) - fn _pre_lock(this: &Arc, read: bool) -> bool { - let mut inserted = false; + fn pre_lock(this: &Arc, _double_lock_self_allowed: bool) { LOCKS_HELD.with(|held| { - // For each lock which is currently locked, check that no lock's locked-before - // set includes the lock we're about to lock, which would imply a lockorder - // inversion. + // For each lock that is currently held, check that no lock's `locked_before` set + // includes the lock we're about to hold, which would imply a lockorder inversion. for (locked_idx, _locked) in held.borrow().iter() { - if read && *locked_idx == this.lock_idx { - // Recursive read locks are explicitly allowed - return; + if *locked_idx == this.lock_idx { + // Note that with `feature = "backtrace"` set, we may be looking at different + // instances of the same lock. Still, doing so is quite risky, a total order + // must be maintained, and doing so across a set of otherwise-identical mutexes + // is fraught with issues. + #[cfg(feature = "backtrace")] + debug_assert!(_double_lock_self_allowed, + "Tried to acquire a lock while it was held!\nLock constructed at {}", + locate_call_symbol(&this._lock_construction_bt).0); + #[cfg(not(feature = "backtrace"))] + panic!("Tried to acquire a lock while it was held!"); } } - for (locked_idx, locked) in held.borrow().iter() { - if !read && *locked_idx == this.lock_idx { - // With `feature = "backtrace"` set, we may be looking at different instances - // of the same lock. - debug_assert!(cfg!(feature = "backtrace"), "Tried to acquire a lock while it was held!"); - } + for (_locked_idx, locked) in held.borrow().iter() { for (locked_dep_idx, _locked_dep) in locked.locked_before.lock().unwrap().iter() { - if *locked_dep_idx == this.lock_idx && *locked_dep_idx != locked.lock_idx { - #[cfg(feature = "backtrace")] - panic!("Tried to violate existing lockorder.\nMutex that should be locked after the current lock was created at the following backtrace.\nNote that to get a backtrace for the lockorder violation, you should set RUST_BACKTRACE=1\nLock being taken constructed at: {} ({}):\n{:?}\nLock constructed at: {} ({})\n{:?}\n\nLock dep created at:\n{:?}\n\n", - get_construction_location(&this._lock_construction_bt), this.lock_idx, this._lock_construction_bt, - get_construction_location(&locked._lock_construction_bt), locked.lock_idx, locked._lock_construction_bt, - _locked_dep._lockdep_trace); - #[cfg(not(feature = "backtrace"))] - panic!("Tried to violate existing lockorder. Build with the backtrace feature for more info."); + let is_dep_this_lock = *locked_dep_idx == this.lock_idx; + let has_same_construction = *locked_dep_idx == locked.lock_idx; + if is_dep_this_lock && !has_same_construction { + #[allow(unused_mut, unused_assignments)] + let mut has_same_callsite = false; + #[cfg(feature = "backtrace")] { + has_same_callsite = _double_lock_self_allowed && + locate_call_symbol(&_locked_dep._lockdep_trace) == + locate_call_symbol(&Backtrace::new()); + } + if !has_same_callsite { + #[cfg(feature = "backtrace")] + panic!("Tried to violate existing lockorder.\nMutex that should be locked after the current lock was created at the following backtrace.\nNote that to get a backtrace for the lockorder violation, you should set RUST_BACKTRACE=1\nLock being taken constructed at: {} ({}):\n{:?}\nLock constructed at: {} ({})\n{:?}\n\nLock dep created at:\n{:?}\n\n", + locate_call_symbol(&this._lock_construction_bt).0, + this.lock_idx, this._lock_construction_bt, + locate_call_symbol(&locked._lock_construction_bt).0, + locked.lock_idx, locked._lock_construction_bt, + _locked_dep._lockdep_trace); + #[cfg(not(feature = "backtrace"))] + panic!("Tried to violate existing lockorder. Build with the backtrace feature for more info."); + } } } // Insert any already-held locks in our locked-before set. @@ -162,14 +188,9 @@ impl LockMetadata { } } held.borrow_mut().insert(this.lock_idx, Arc::clone(this)); - inserted = true; }); - inserted } - fn pre_lock(this: &Arc) { Self::_pre_lock(this, false); } - fn pre_read_lock(this: &Arc) -> bool { Self::_pre_lock(this, true) } - fn held_by_thread(this: &Arc) -> LockHeldState { let mut res = LockHeldState::NotHeldByThread; LOCKS_HELD.with(|held| { @@ -203,6 +224,11 @@ pub struct Mutex { inner: StdMutex, deps: Arc, } +impl Mutex { + pub(crate) fn into_inner(self) -> LockResult { + self.inner.into_inner().map_err(|_| ()) + } +} #[must_use = "if unused the Mutex will immediately unlock"] pub struct MutexGuard<'a, T: Sized + 'a> { @@ -249,7 +275,7 @@ impl Mutex { } pub fn lock<'a>(&'a self) -> LockResult> { - LockMetadata::pre_lock(&self.deps); + LockMetadata::pre_lock(&self.deps, false); self.inner.lock().map(|lock| MutexGuard { mutex: self, lock }).map_err(|_| ()) } @@ -262,11 +288,17 @@ impl Mutex { } } -impl LockTestExt for Mutex { +impl<'a, T: 'a> LockTestExt<'a> for Mutex { #[inline] fn held_by_thread(&self) -> LockHeldState { LockMetadata::held_by_thread(&self.deps) } + type ExclLock = MutexGuard<'a, T>; + #[inline] + fn unsafe_well_ordered_double_lock_self(&'a self) -> MutexGuard { + LockMetadata::pre_lock(&self.deps, true); + self.inner.lock().map(|lock| MutexGuard { mutex: self, lock }).unwrap() + } } pub struct RwLock { @@ -276,7 +308,6 @@ pub struct RwLock { pub struct RwLockReadGuard<'a, T: Sized + 'a> { lock: &'a RwLock, - first_lock: bool, guard: StdRwLockReadGuard<'a, T>, } @@ -295,12 +326,6 @@ impl Deref for RwLockReadGuard<'_, T> { impl Drop for RwLockReadGuard<'_, T> { fn drop(&mut self) { - if !self.first_lock { - // Note that its not strictly true that the first taken read lock will get unlocked - // last, but in practice our locks are always taken as RAII, so it should basically - // always be true. - return; - } LOCKS_HELD.with(|held| { held.borrow_mut().remove(&self.lock.deps.lock_idx); }); @@ -335,12 +360,16 @@ impl RwLock { } pub fn read<'a>(&'a self) -> LockResult> { - let first_lock = LockMetadata::pre_read_lock(&self.deps); - self.inner.read().map(|guard| RwLockReadGuard { lock: self, guard, first_lock }).map_err(|_| ()) + // Note that while we could be taking a recursive read lock here, Rust's `RwLock` may + // deadlock trying to take a second read lock if another thread is waiting on the write + // lock. This behavior is platform dependent, but our in-tree `FairRwLock` guarantees + // such a deadlock. + LockMetadata::pre_lock(&self.deps, false); + self.inner.read().map(|guard| RwLockReadGuard { lock: self, guard }).map_err(|_| ()) } pub fn write<'a>(&'a self) -> LockResult> { - LockMetadata::pre_lock(&self.deps); + LockMetadata::pre_lock(&self.deps, false); self.inner.write().map(|guard| RwLockWriteGuard { lock: self, guard }).map_err(|_| ()) } @@ -353,11 +382,17 @@ impl RwLock { } } -impl LockTestExt for RwLock { +impl<'a, T: 'a> LockTestExt<'a> for RwLock { #[inline] fn held_by_thread(&self) -> LockHeldState { LockMetadata::held_by_thread(&self.deps) } + type ExclLock = RwLockWriteGuard<'a, T>; + #[inline] + fn unsafe_well_ordered_double_lock_self(&'a self) -> RwLockWriteGuard<'a, T> { + LockMetadata::pre_lock(&self.deps, true); + self.inner.write().map(|guard| RwLockWriteGuard { lock: self, guard }).unwrap() + } } pub type FairRwLock = RwLock;