test: Fix TestPotentialDeadLockDetected suppression #21678

1. 
   hebasto commented at 10:29 AM on April 14, 2021: member

   This PR is a #21669 follow up, and fixes locally running make check.

2. test: Fix TSan suppression

   This change fixes locally running tests.

   f2ef5a8afd
3. fanquake added the label Tests on Apr 14, 2021
4. fanquake requested review from MarcoFalke on Apr 14, 2021
5. fanquake requested review from practicalswift on Apr 14, 2021
6. 
   MarcoFalke commented at 12:46 PM on April 14, 2021: member

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

   cr ACK f2ef5a8afd5da2fb7775dafdee15e6ac532d8fe4

   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)

7. MarcoFalke renamed this:
   test: Fix TSan suppression
   test: Fix TestPotentialDeadLockDetected suppression
   on Apr 14, 2021
8. 
   hebasto commented at 12:53 PM on April 14, 2021: member

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

9. 
   MarcoFalke commented at 1:17 PM on April 14, 2021: member

   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.

10. 
    hebasto commented at 1:23 PM on April 14, 2021: member

    @ryanofsky @vasild What do you think?

11. 
    MarcoFalke commented at 1:55 PM on April 14, 2021: member

    ( Test added by @ryanofsky in commit 41b88e93375d57db12da923f45f87b9a2db8e730 )

12. 
    ryanofsky commented at 2:47 PM on April 14, 2021: member

    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.

13. 
    ryanofsky commented at 2:53 PM on April 14, 2021: member

    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.

14. MarcoFalke merged this on Apr 14, 2021
15. MarcoFalke closed this on Apr 14, 2021

    ---

16. hebasto deleted the branch on Apr 14, 2021
17. 
    vasild commented at 4:24 PM on April 14, 2021: member

    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?

18. 
    hebasto commented at 4:25 PM on April 14, 2021: member

    Why it started happening now?

    clang-12?

19. sidhujag referenced this in commit 10eb1422e0 on Apr 14, 2021
20. PastaPastaPasta referenced this in commit 0a276d388b on Sep 17, 2021
21. PastaPastaPasta referenced this in commit b69d657855 on Sep 19, 2021
22. thelazier referenced this in commit ae05394685 on Sep 25, 2021
23. DrahtBot locked this on Aug 18, 2022

Contributors

hebastoMarcoFalkeryanofskyvasild

Review Requested
MarcoFalkepracticalswift