Looks like your compiler inlined the whole function and thus can not apply the suppression?

cr ACK f2ef5a8

An alternative would be to simply avoid the intentional lock-order-inversion.

diff --git a/src/test/sync_tests.cpp b/src/test/sync_tests.cpp
index 3e4d1dac9e..737cf166cd 100644
--- a/src/test/sync_tests.cpp
+++ b/src/test/sync_tests.cpp
@@ -17,6 +17,11 @@ void TestPotentialDeadLockDetected(MutexType& mutex1, MutexType& mutex2)
         LOCK2(mutex1, mutex2);
     }
     BOOST_CHECK(LockStackEmpty());
+#ifndef DEBUG_LOCKORDER
+    // Return early to avoid the lock-order-inversion.
+    // Sanitizers different from our own DEBUG_LOCKORDER sanitizer would otherwise warn about this.
+    return;
+#endif
     bool error_thrown = false;
     try {
         LOCK2(mutex2, mutex1);
@@ -25,11 +30,7 @@ void TestPotentialDeadLockDetected(MutexType& mutex1, MutexType& mutex2)
         error_thrown = true;
     }
     BOOST_CHECK(LockStackEmpty());
-    #ifdef DEBUG_LOCKORDER
     BOOST_CHECK(error_thrown);
-    #else
-    BOOST_CHECK(!error_thrown);
-    #endif
 }
 
 #ifdef DEBUG_LOCKORDER
diff --git a/test/sanitizer_suppressions/tsan b/test/sanitizer_suppressions/tsan
index e2c79d56c5..0213dc58fc 100644
--- a/test/sanitizer_suppressions/tsan
+++ b/test/sanitizer_suppressions/tsan
@@ -15,9 +15,6 @@ race:bitcoin-qt
 # deadlock (TODO fix)
 deadlock:CChainState::ConnectTip
 
-# Intentional deadlock in tests
-deadlock:TestPotentialDeadLockDetected
-
 # Wildcard for all gui tests, should be replaced with non-wildcard suppressions
 race:src/qt/test/*
 deadlock:src/qt/test/*

(diff untested and uncompiled)

The suggested diff skips a part of the test, particularly BOOST_CHECK(!error_thrown);, that seems unwanted.

What is the value of checking that two locks can be successfully taken both in the order a-b and in the order b-a. We don't use that anywhere, so it seems odd to have a test for it.

@ryanofsky @vasild What do you think?

( Test added by @ryanofsky in commit 41b88e9 )

I'm not sure what question's being asked, but I'm happy to review any change.

Intention of #else clause in the referenced commit is to check that process will not unnecessarily throw an exception and potentially crash when DEBUG_LOCKORDER is disabled. Crashes can be bad, so it is good to verify that a crash won't happen when crashes are supposed to be disabled just because locks are acquired in a theoretically-bad ordering that wouldn't cause an actual problem.

But I don't think it's a big deal to drop the check if it's too expensive because it can't be done with sanitizers enabled and would require a separate build. Or any other reason. It's not a very important check.

Oh, I understand better now. This PR is fixing a broken suppression, and Marco's saying it is easier to skip the check that requires the suppression instead of fixing the suppression.

I have no opinion on what's easier. I think the check does serve a purpose, but it's also not very important.

So, the problem this PR fixed is that TestPotentialDeadLockDetected() was inlined and thus did not match the suppression deadlock:TestPotentialDeadLockDetected? Why it started happening now?

Why it started happening now?

clang-12?