From f66f720fa54cdd28ac998dcc39c859567b2455e0 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 10 Jan 2023 06:26:46 +0000 Subject: [PATCH 1/4] Move `no-std` sync implementations to a folder to clean up --- lightning/src/lib.rs | 11 ----------- lightning/src/sync/mod.rs | 11 +++++++++++ lightning/src/{sync.rs => sync/nostd_sync.rs} | 0 3 files changed, 11 insertions(+), 11 deletions(-) create mode 100644 lightning/src/sync/mod.rs rename lightning/src/{sync.rs => sync/nostd_sync.rs} (100%) diff --git a/lightning/src/lib.rs b/lightning/src/lib.rs index 1f3ab47b1ae..1ec4d6aa5a5 100644 --- a/lightning/src/lib.rs +++ b/lightning/src/lib.rs @@ -180,15 +180,4 @@ mod debug_sync; #[cfg(all(not(feature = "_bench_unstable"), feature = "backtrace", feature = "std", test))] extern crate backtrace; -#[cfg(feature = "std")] -mod sync { - #[cfg(all(not(feature = "_bench_unstable"), test))] - pub use crate::debug_sync::*; - #[cfg(any(feature = "_bench_unstable", not(test)))] - pub use ::std::sync::{Arc, Mutex, Condvar, MutexGuard, RwLock, RwLockReadGuard, RwLockWriteGuard}; - #[cfg(any(feature = "_bench_unstable", not(test)))] - pub use crate::util::fairrwlock::FairRwLock; -} - -#[cfg(not(feature = "std"))] mod sync; diff --git a/lightning/src/sync/mod.rs b/lightning/src/sync/mod.rs new file mode 100644 index 00000000000..584338031fc --- /dev/null +++ b/lightning/src/sync/mod.rs @@ -0,0 +1,11 @@ +#[cfg(all(feature = "std", not(feature = "_bench_unstable"), test))] +pub use crate::debug_sync::*; +#[cfg(all(feature = "std", any(feature = "_bench_unstable", not(test))))] +pub use ::std::sync::{Arc, Mutex, Condvar, MutexGuard, RwLock, RwLockReadGuard, RwLockWriteGuard}; +#[cfg(all(feature = "std", any(feature = "_bench_unstable", not(test))))] +pub use crate::util::fairrwlock::FairRwLock; + +#[cfg(not(feature = "std"))] +mod nostd_sync; +#[cfg(not(feature = "std"))] +pub use nostd_sync::*; diff --git a/lightning/src/sync.rs b/lightning/src/sync/nostd_sync.rs similarity index 100% rename from lightning/src/sync.rs rename to lightning/src/sync/nostd_sync.rs From 558bfa3fb3789d7d2586fef11d62367c174f370f Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 10 Jan 2023 06:29:58 +0000 Subject: [PATCH 2/4] Move `debug_sync` to the new `sync` folder --- lightning/src/lib.rs | 2 -- lightning/src/{ => sync}/debug_sync.rs | 0 lightning/src/sync/mod.rs | 5 ++++- 3 files changed, 4 insertions(+), 3 deletions(-) rename lightning/src/{ => sync}/debug_sync.rs (100%) diff --git a/lightning/src/lib.rs b/lightning/src/lib.rs index 1ec4d6aa5a5..d4289d07d2e 100644 --- a/lightning/src/lib.rs +++ b/lightning/src/lib.rs @@ -175,8 +175,6 @@ mod prelude { pub use alloc::string::ToString; } -#[cfg(all(not(feature = "_bench_unstable"), feature = "std", test))] -mod debug_sync; #[cfg(all(not(feature = "_bench_unstable"), feature = "backtrace", feature = "std", test))] extern crate backtrace; diff --git a/lightning/src/debug_sync.rs b/lightning/src/sync/debug_sync.rs similarity index 100% rename from lightning/src/debug_sync.rs rename to lightning/src/sync/debug_sync.rs diff --git a/lightning/src/sync/mod.rs b/lightning/src/sync/mod.rs index 584338031fc..f5755fc1017 100644 --- a/lightning/src/sync/mod.rs +++ b/lightning/src/sync/mod.rs @@ -1,5 +1,8 @@ #[cfg(all(feature = "std", not(feature = "_bench_unstable"), test))] -pub use crate::debug_sync::*; +mod debug_sync; +#[cfg(all(feature = "std", not(feature = "_bench_unstable"), test))] +pub use debug_sync::*; + #[cfg(all(feature = "std", any(feature = "_bench_unstable", not(test))))] pub use ::std::sync::{Arc, Mutex, Condvar, MutexGuard, RwLock, RwLockReadGuard, RwLockWriteGuard}; #[cfg(all(feature = "std", any(feature = "_bench_unstable", not(test))))] From 230331f3e8efe9cee0ad2dc1644051bd9679a01f Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 10 Jan 2023 06:34:30 +0000 Subject: [PATCH 3/4] Move tests from debug_sync to a new submodule This will allow us to change the module regex match in debug_sync to make it more robust. --- lightning/src/sync/debug_sync.rs | 97 --------------------- lightning/src/sync/mod.rs | 3 + lightning/src/sync/test_lockorder_checks.rs | 94 ++++++++++++++++++++ 3 files changed, 97 insertions(+), 97 deletions(-) create mode 100644 lightning/src/sync/test_lockorder_checks.rs diff --git a/lightning/src/sync/debug_sync.rs b/lightning/src/sync/debug_sync.rs index b61d1cb55e8..ac82475f964 100644 --- a/lightning/src/sync/debug_sync.rs +++ b/lightning/src/sync/debug_sync.rs @@ -333,100 +333,3 @@ impl RwLock { } pub type FairRwLock = RwLock; - -mod tests { - use super::{RwLock, Mutex}; - - #[test] - #[should_panic] - #[cfg(not(feature = "backtrace"))] - fn recursive_lock_fail() { - let mutex = Mutex::new(()); - let _a = mutex.lock().unwrap(); - let _b = mutex.lock().unwrap(); - } - - #[test] - fn recursive_read() { - let lock = RwLock::new(()); - let _a = lock.read().unwrap(); - let _b = lock.read().unwrap(); - } - - #[test] - #[should_panic] - fn lockorder_fail() { - let a = Mutex::new(()); - let b = Mutex::new(()); - { - let _a = a.lock().unwrap(); - let _b = b.lock().unwrap(); - } - { - let _b = b.lock().unwrap(); - let _a = a.lock().unwrap(); - } - } - - #[test] - #[should_panic] - fn write_lockorder_fail() { - let a = RwLock::new(()); - let b = RwLock::new(()); - { - let _a = a.write().unwrap(); - let _b = b.write().unwrap(); - } - { - let _b = b.write().unwrap(); - let _a = a.write().unwrap(); - } - } - - #[test] - #[should_panic] - fn read_lockorder_fail() { - let a = RwLock::new(()); - let b = RwLock::new(()); - { - let _a = a.read().unwrap(); - let _b = b.read().unwrap(); - } - { - let _b = b.read().unwrap(); - let _a = a.read().unwrap(); - } - } - - #[test] - fn read_recursive_no_lockorder() { - // Like the above, but note that no lockorder is implied when we recursively read-lock a - // RwLock, causing this to pass just fine. - let a = RwLock::new(()); - let b = RwLock::new(()); - let _outer = a.read().unwrap(); - { - let _a = a.read().unwrap(); - let _b = b.read().unwrap(); - } - { - let _b = b.read().unwrap(); - let _a = a.read().unwrap(); - } - } - - #[test] - #[should_panic] - fn read_write_lockorder_fail() { - let a = RwLock::new(()); - let b = RwLock::new(()); - { - let _a = a.write().unwrap(); - let _b = b.read().unwrap(); - } - { - let _b = b.read().unwrap(); - let _a = a.write().unwrap(); - } - } -} diff --git a/lightning/src/sync/mod.rs b/lightning/src/sync/mod.rs index f5755fc1017..f7226a5fa34 100644 --- a/lightning/src/sync/mod.rs +++ b/lightning/src/sync/mod.rs @@ -2,6 +2,9 @@ mod debug_sync; #[cfg(all(feature = "std", not(feature = "_bench_unstable"), test))] pub use debug_sync::*; +#[cfg(all(feature = "std", not(feature = "_bench_unstable"), test))] +// Note that to make debug_sync's regex work this must not contain `debug_string` in the module name +mod test_lockorder_checks; #[cfg(all(feature = "std", any(feature = "_bench_unstable", not(test))))] pub use ::std::sync::{Arc, Mutex, Condvar, MutexGuard, RwLock, RwLockReadGuard, RwLockWriteGuard}; diff --git a/lightning/src/sync/test_lockorder_checks.rs b/lightning/src/sync/test_lockorder_checks.rs new file mode 100644 index 00000000000..f9f30e2cfa2 --- /dev/null +++ b/lightning/src/sync/test_lockorder_checks.rs @@ -0,0 +1,94 @@ +use crate::sync::debug_sync::{RwLock, Mutex}; + +#[test] +#[should_panic] +#[cfg(not(feature = "backtrace"))] +fn recursive_lock_fail() { + let mutex = Mutex::new(()); + let _a = mutex.lock().unwrap(); + let _b = mutex.lock().unwrap(); +} + +#[test] +fn recursive_read() { + let lock = RwLock::new(()); + let _a = lock.read().unwrap(); + let _b = lock.read().unwrap(); +} + +#[test] +#[should_panic] +fn lockorder_fail() { + let a = Mutex::new(()); + let b = Mutex::new(()); + { + let _a = a.lock().unwrap(); + let _b = b.lock().unwrap(); + } + { + let _b = b.lock().unwrap(); + let _a = a.lock().unwrap(); + } +} + +#[test] +#[should_panic] +fn write_lockorder_fail() { + let a = RwLock::new(()); + let b = RwLock::new(()); + { + let _a = a.write().unwrap(); + let _b = b.write().unwrap(); + } + { + let _b = b.write().unwrap(); + let _a = a.write().unwrap(); + } +} + +#[test] +#[should_panic] +fn read_lockorder_fail() { + let a = RwLock::new(()); + let b = RwLock::new(()); + { + let _a = a.read().unwrap(); + let _b = b.read().unwrap(); + } + { + let _b = b.read().unwrap(); + let _a = a.read().unwrap(); + } +} + +#[test] +fn read_recursive_no_lockorder() { + // Like the above, but note that no lockorder is implied when we recursively read-lock a + // RwLock, causing this to pass just fine. + let a = RwLock::new(()); + let b = RwLock::new(()); + let _outer = a.read().unwrap(); + { + let _a = a.read().unwrap(); + let _b = b.read().unwrap(); + } + { + let _b = b.read().unwrap(); + let _a = a.read().unwrap(); + } +} + +#[test] +#[should_panic] +fn read_write_lockorder_fail() { + let a = RwLock::new(()); + let b = RwLock::new(()); + { + let _a = a.write().unwrap(); + let _b = b.read().unwrap(); + } + { + let _b = b.read().unwrap(); + let _a = a.write().unwrap(); + } +} From ab46f6b988c9c6f1e4fdc0d8f549dd64a1c07fcf Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 10 Jan 2023 06:37:39 +0000 Subject: [PATCH 4/4] Make `debug_sync` regex more robust On windows the symbol names appear to sometimes be truncated, which causes the symbol name to not include the `::new` at the end. This causes the regex to mis-match and track the wrong location for the mutex construction, leading to bogus lockorder violations. For example, in testing the following symbol name appeared on Windows, without the function name itself: `lightning::debug_sync::RwLock,std::collections::hash::map::RandomState> >::` --- lightning/src/sync/debug_sync.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lightning/src/sync/debug_sync.rs b/lightning/src/sync/debug_sync.rs index ac82475f964..9f7caa2c180 100644 --- a/lightning/src/sync/debug_sync.rs +++ b/lightning/src/sync/debug_sync.rs @@ -77,7 +77,7 @@ 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. - let sync_mutex_constr_regex = regex::Regex::new(r"lightning.*debug_sync.*new").unwrap(); + 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() {