Skip to content

Make debug_sync regex more robust #1944

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jan 10, 2023
Merged

Make debug_sync regex more robust #1944

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f66f720
Move `no-std` sync implementations to a folder to clean up
TheBlueMatt Jan 10, 2023
558bfa3
Move `debug_sync` to the new `sync` folder
TheBlueMatt Jan 10, 2023
230331f
Move tests from debug_sync to a new submodule
TheBlueMatt Jan 10, 2023
ab46f6b
Make `debug_sync` regex more robust
TheBlueMatt Jan 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 0 additions & 13 deletions lightning/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,20 +175,7 @@ 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;

#[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;
99 changes: 1 addition & 98 deletions lightning/src/debug_sync.rs → lightning/src/sync/debug_sync.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need a trailing .* to match the locks type (and new when it is not truncated)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so, is_match "Returns true if and only if there is a match for the regex in the string given." ie not if the match is the string but if there is a match in the string.

let mut found_debug_sync = false;
for frame in backtrace.frames() {
for symbol in frame.symbols() {
Expand Down Expand Up @@ -333,100 +333,3 @@ impl<T> RwLock<T> {
}

pub type FairRwLock<T> = RwLock<T>;

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();
}
}
}
17 changes: 17 additions & 0 deletions lightning/src/sync/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
#[cfg(all(feature = "std", not(feature = "_bench_unstable"), test))]
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};
#[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::*;
0 lightning/src/sync.rs → lightning/src/sync/nostd_sync.rs
View file
Open in desktop
File renamed without changes.
94 changes: 94 additions & 0 deletions lightning/src/sync/test_lockorder_checks.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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();
}
}