test: Remove spurious double lock tsan suppressions by bumping to clang-12 #21669

maflcko · 2021-04-13T18:07:37Z

The double lock warnings appeared in #19041, but they didn't make any sense. Also, our sync module would detect double locks, if there were any.

Bumping to clang-12 allows us to remove the spurious suppressions needed to run the tests, so do that.

…ng-12

hebasto · 2021-04-13T18:10:10Z

Concept ACK. Any links to clang developers notes (if known)?

maflcko · 2021-04-13T18:47:24Z

No, sorry not aware of any links

I can no longer observe the need for this suppression. This reverts commit fa1fc53.

practicalswift

Very happy to see that these suppressions are no longer needed: especially the mutex:UpdateTip and race:ProcessNewBlock suppressions.

Every file or function level suppression risks hiding future issues in the same file or function, so I think it makes sense to continually try to prune the list of suppression and keep it as short as possible. Thanks for doing that @MarcoFalke!

cr ACK fadea0b assuming CI passes and more specifically that newer Clang agrees that these TSan suppressions are no longer needed.

Aside:

Great TSan field report posted in April 2021: "Eliminating Data Races in Firefox – A Technical Report". Recommended reading for all sanitizer fans! :)

hebasto · 2021-04-14T08:16:12Z

@practicalswift

Great TSan field report posted in April 2021: "Eliminating Data Races in Firefox – A Technical Report". Recommended reading for all sanitizer fans! :)

Thanks! 👍

hebasto · 2021-04-14T08:43:41Z

It makes local TSan tests a bit complicated as clang-12 is not available in the Focal repo for now.

hebasto · 2021-04-14T08:45:34Z

https://cirrus-ci.com/task/4889293647773696

hebasto · 2021-04-14T10:12:16Z

Tested locally:

$ lsb_release -c
Codename:	hirsute
$ clang --version | head -1
Ubuntu clang version 12.0.0-++rc4-1ubuntu1
$ git rev-parse HEAD
b8e5bbdf93e5b3b35557d3b0e57a426492d568c1
$ echo $TSAN_OPTIONS 
suppressions=/home/hebasto/bitcoin/test/sanitizer_suppressions/tsan:second_deadlock_stack=1
$ make -C depends CC=clang CXX='clang++ -stdlib=libc++'
$ CONFIG_SITE=$PWD/depends/x86_64-pc-linux-gnu/share/config.site ./configure --with-sanitizers=thread CC=clang CXX='clang++ -stdlib=libc++' CXXFLAGS=-g
$ make clean
$ make check
...
Running 2 test cases...
Entering test module "Bitcoin Core Test Suite"
test/sync_tests.cpp(79): Entering test suite "sync_tests"
test/sync_tests.cpp(81): Entering test case "potential_deadlock_detected"
==================
WARNING: ThreadSanitizer: lock-order-inversion (potential deadlock) (pid=16889)
  Cycle in lock order graph: M131693 (0x7ffce5bda6f8) => M131694 (0x7ffce5bda6d0) => M131693

  Mutex M131694 acquired here while holding mutex M131693 in main thread:
    #0 pthread_mutex_lock <null> (test_bitcoin+0xf5036)
    #1 std::__1::recursive_mutex::lock() <null> (libc++.so.1+0x48b05)
    #2 sync_tests::potential_deadlock_detected::test_method() /home/hebasto/bitcoin/src/test/sync_tests.cpp:89:5 (test_bitcoin+0x673d8e)
    #3 sync_tests::potential_deadlock_detected_invoker() /home/hebasto/bitcoin/src/test/sync_tests.cpp:81:1 (test_bitcoin+0x67368b)
    #4 boost::detail::function::void_function_invoker0<void (*)(), void>::invoke(boost::detail::function::function_buffer&) /home/hebasto/bitcoin/depends/x86_64-pc-linux-gnu/include/boost/function/function_template.hpp:117:11 (test_bitcoin+0x1ba82d)
    #5 boost::detail::function::function_obj_invoker0<boost::detail::forward, int>::invoke(boost::detail::function::function_buffer&) <null> (test_bitcoin+0xfe42da)

  Mutex M131693 previously acquired by the same thread here:
    #0 pthread_mutex_lock <null> (test_bitcoin+0xf5036)
    #1 std::__1::recursive_mutex::lock() <null> (libc++.so.1+0x48b05)
    #2 sync_tests::potential_deadlock_detected::test_method() /home/hebasto/bitcoin/src/test/sync_tests.cpp:89:5 (test_bitcoin+0x673d8e)
    #3 sync_tests::potential_deadlock_detected_invoker() /home/hebasto/bitcoin/src/test/sync_tests.cpp:81:1 (test_bitcoin+0x67368b)
    #4 boost::detail::function::void_function_invoker0<void (*)(), void>::invoke(boost::detail::function::function_buffer&) /home/hebasto/bitcoin/depends/x86_64-pc-linux-gnu/include/boost/function/function_template.hpp:117:11 (test_bitcoin+0x1ba82d)
    #5 boost::detail::function::function_obj_invoker0<boost::detail::forward, int>::invoke(boost::detail::function::function_buffer&) <null> (test_bitcoin+0xfe42da)

  Mutex M131693 acquired here while holding mutex M131694 in main thread:
    #0 pthread_mutex_lock <null> (test_bitcoin+0xf5036)
    #1 std::__1::recursive_mutex::lock() <null> (libc++.so.1+0x48b05)
    #2 sync_tests::potential_deadlock_detected::test_method() /home/hebasto/bitcoin/src/test/sync_tests.cpp:89:5 (test_bitcoin+0x673d8e)
    #3 sync_tests::potential_deadlock_detected_invoker() /home/hebasto/bitcoin/src/test/sync_tests.cpp:81:1 (test_bitcoin+0x67368b)
    #4 boost::detail::function::void_function_invoker0<void (*)(), void>::invoke(boost::detail::function::function_buffer&) /home/hebasto/bitcoin/depends/x86_64-pc-linux-gnu/include/boost/function/function_template.hpp:117:11 (test_bitcoin+0x1ba82d)
    #5 boost::detail::function::function_obj_invoker0<boost::detail::forward, int>::invoke(boost::detail::function::function_buffer&) <null> (test_bitcoin+0xfe42da)

  Mutex M131694 previously acquired by the same thread here:
    #0 pthread_mutex_lock <null> (test_bitcoin+0xf5036)
    #1 std::__1::recursive_mutex::lock() <null> (libc++.so.1+0x48b05)
    #2 sync_tests::potential_deadlock_detected::test_method() /home/hebasto/bitcoin/src/test/sync_tests.cpp:89:5 (test_bitcoin+0x673d8e)
    #3 sync_tests::potential_deadlock_detected_invoker() /home/hebasto/bitcoin/src/test/sync_tests.cpp:81:1 (test_bitcoin+0x67368b)
    #4 boost::detail::function::void_function_invoker0<void (*)(), void>::invoke(boost::detail::function::function_buffer&) /home/hebasto/bitcoin/depends/x86_64-pc-linux-gnu/include/boost/function/function_template.hpp:117:11 (test_bitcoin+0x1ba82d)
    #5 boost::detail::function::function_obj_invoker0<boost::detail::forward, int>::invoke(boost::detail::function::function_buffer&) <null> (test_bitcoin+0xfe42da)

SUMMARY: ThreadSanitizer: lock-order-inversion (potential deadlock) (/home/hebasto/bitcoin/src/test/test_bitcoin+0xf5036) in pthread_mutex_lock
==================
==================
WARNING: ThreadSanitizer: lock-order-inversion (potential deadlock) (pid=16889)
  Cycle in lock order graph: M131695 (0x7ffce5bda6a8) => M131696 (0x7ffce5bda680) => M131695

  Mutex M131696 acquired here while holding mutex M131695 in main thread:
    #0 pthread_mutex_lock <null> (test_bitcoin+0xf5036)
    #1 std::__1::mutex::lock() <null> (libc++.so.1+0x48a15)
    #2 sync_tests::potential_deadlock_detected::test_method() /home/hebasto/bitcoin/src/test/sync_tests.cpp:94:5 (test_bitcoin+0x673dce)
    #3 sync_tests::potential_deadlock_detected_invoker() /home/hebasto/bitcoin/src/test/sync_tests.cpp:81:1 (test_bitcoin+0x67368b)
    #4 boost::detail::function::void_function_invoker0<void (*)(), void>::invoke(boost::detail::function::function_buffer&) /home/hebasto/bitcoin/depends/x86_64-pc-linux-gnu/include/boost/function/function_template.hpp:117:11 (test_bitcoin+0x1ba82d)
    #5 boost::detail::function::function_obj_invoker0<boost::detail::forward, int>::invoke(boost::detail::function::function_buffer&) <null> (test_bitcoin+0xfe42da)

  Mutex M131695 previously acquired by the same thread here:
    #0 pthread_mutex_lock <null> (test_bitcoin+0xf5036)
    #1 std::__1::mutex::lock() <null> (libc++.so.1+0x48a15)
    #2 sync_tests::potential_deadlock_detected::test_method() /home/hebasto/bitcoin/src/test/sync_tests.cpp:94:5 (test_bitcoin+0x673dce)
    #3 sync_tests::potential_deadlock_detected_invoker() /home/hebasto/bitcoin/src/test/sync_tests.cpp:81:1 (test_bitcoin+0x67368b)
    #4 boost::detail::function::void_function_invoker0<void (*)(), void>::invoke(boost::detail::function::function_buffer&) /home/hebasto/bitcoin/depends/x86_64-pc-linux-gnu/include/boost/function/function_template.hpp:117:11 (test_bitcoin+0x1ba82d)
    #5 boost::detail::function::function_obj_invoker0<boost::detail::forward, int>::invoke(boost::detail::function::function_buffer&) <null> (test_bitcoin+0xfe42da)

  Mutex M131695 acquired here while holding mutex M131696 in main thread:
    #0 pthread_mutex_lock <null> (test_bitcoin+0xf5036)
    #1 std::__1::mutex::lock() <null> (libc++.so.1+0x48a15)
    #2 sync_tests::potential_deadlock_detected::test_method() /home/hebasto/bitcoin/src/test/sync_tests.cpp:94:5 (test_bitcoin+0x673dce)
    #3 sync_tests::potential_deadlock_detected_invoker() /home/hebasto/bitcoin/src/test/sync_tests.cpp:81:1 (test_bitcoin+0x67368b)
    #4 boost::detail::function::void_function_invoker0<void (*)(), void>::invoke(boost::detail::function::function_buffer&) /home/hebasto/bitcoin/depends/x86_64-pc-linux-gnu/include/boost/function/function_template.hpp:117:11 (test_bitcoin+0x1ba82d)
    #5 boost::detail::function::function_obj_invoker0<boost::detail::forward, int>::invoke(boost::detail::function::function_buffer&) <null> (test_bitcoin+0xfe42da)

  Mutex M131696 previously acquired by the same thread here:
    #0 pthread_mutex_lock <null> (test_bitcoin+0xf5036)
    #1 std::__1::mutex::lock() <null> (libc++.so.1+0x48a15)
    #2 sync_tests::potential_deadlock_detected::test_method() /home/hebasto/bitcoin/src/test/sync_tests.cpp:94:5 (test_bitcoin+0x673dce)
    #3 sync_tests::potential_deadlock_detected_invoker() /home/hebasto/bitcoin/src/test/sync_tests.cpp:81:1 (test_bitcoin+0x67368b)
    #4 boost::detail::function::void_function_invoker0<void (*)(), void>::invoke(boost::detail::function::function_buffer&) /home/hebasto/bitcoin/depends/x86_64-pc-linux-gnu/include/boost/function/function_template.hpp:117:11 (test_bitcoin+0x1ba82d)
    #5 boost::detail::function::function_obj_invoker0<boost::detail::forward, int>::invoke(boost::detail::function::function_buffer&) <null> (test_bitcoin+0xfe42da)

SUMMARY: ThreadSanitizer: lock-order-inversion (potential deadlock) (/home/hebasto/bitcoin/src/test/test_bitcoin+0xf5036) in pthread_mutex_lock
==================
2021-04-14T10:04:13Z Seed: Setting random seed for current tests to RANDOM_CTX_SEED=7acdb16613e39bc474d4c35d8c1ffd9b76f45604614957bf721d9ba4c98f544a
2021-04-14T10:04:13.840447Z [test] [init.cpp:817] [InitLogging] Bitcoin Core version v21.99.0-b8e5bbdf93e5 (release build)
2021-04-14T10:04:13.840753Z [test] [init.cpp:1011] [AppInitParameterInteraction] Assuming ancestors of block 0000000000000000000b9d2ec5a352ecba0592946514a92f14319dc2b367fc72 have valid signatures.
2021-04-14T10:04:13.840810Z [test] [init.cpp:1024] [AppInitParameterInteraction] Setting nMinimumChainWork=00000000000000000000000000000000000000001533efd8d716a517fe2c5008
2021-04-14T10:04:13.914128Z [test] [script/sigcache.cpp:102] [InitSignatureCache] Using 16 MiB out of 32/2 requested for signature cache, able to store 524288 elements
2021-04-14T10:04:13.996929Z [test] [validation.cpp:1355] [InitScriptExecutionCache] Using 16 MiB out of 32/2 requested for script execution cache, able to store 524288 elements
test/sync_tests.cpp(81): Leaving test case "potential_deadlock_detected"; testing time: 1185317us
test/sync_tests.cpp(118): Entering test case "inconsistent_lock_order_detected"
2021-04-14T10:04:15.027918Z [test] [test/util/setup_common.cpp:63] [Seed] Seed: Setting random seed for current tests to RANDOM_CTX_SEED=7acdb16613e39bc474d4c35d8c1ffd9b76f45604614957bf721d9ba4c98f544a
2021-04-14T10:04:15.028075Z [test] [init.cpp:817] [InitLogging] Bitcoin Core version v21.99.0-b8e5bbdf93e5 (release build)
2021-04-14T10:04:15.028348Z [test] [init.cpp:1011] [AppInitParameterInteraction] Assuming ancestors of block 0000000000000000000b9d2ec5a352ecba0592946514a92f14319dc2b367fc72 have valid signatures.
2021-04-14T10:04:15.028413Z [test] [init.cpp:1024] [AppInitParameterInteraction] Setting nMinimumChainWork=00000000000000000000000000000000000000001533efd8d716a517fe2c5008
2021-04-14T10:04:15.059007Z [test] [script/sigcache.cpp:102] [InitSignatureCache] Using 16 MiB out of 32/2 requested for signature cache, able to store 524288 elements
2021-04-14T10:04:15.087074Z [test] [validation.cpp:1355] [InitScriptExecutionCache] Using 16 MiB out of 32/2 requested for script execution cache, able to store 524288 elements
test/sync_tests.cpp(118): Leaving test case "inconsistent_lock_order_detected"; testing time: 66114us
test/sync_tests.cpp(79): Leaving test suite "sync_tests"; testing time: 1251550us
Leaving test module "Bitcoin Core Test Suite"; testing time: 1251692us

*** No errors detected
ThreadSanitizer: reported 2 warnings
...

maflcko · 2021-04-14T13:45:52Z

It makes local TSan tests a bit complicated as clang-12 is not available in the Focal repo for now.

What is the point of running a sanitizer with known bugs in the first place?

If you really want to run a thread sanitizer in Focal, you can use the gcc one. Apparently it is not recommended, but it doesn't need the spurious suppressions either.

hebasto · 2021-04-14T13:48:33Z

If you really want to run a thread sanitizer in Focal...

I did not mean it. Just stating a fact that I need a hirsute distro now.

maflcko · 2021-04-14T13:53:46Z

It is also possible to install another clang version on Focal, see https://apt.llvm.org/. Though, personally I use vms or docker/podman to run the version of Ubuntu I want.

hebasto · 2021-04-14T13:58:55Z

$ VBoxManage list vms
"macOS Big Sur 11" {62a53114-1691-4f71-93e2-5dbaada8734c}
"CentOS 8" {cda31622-2a2d-41e6-b466-866f0026f748}
"Windows 10" {ccc55f7a-f1bd-4827-b637-e504b3d16f3f}
"Ubuntu Focal 20.04" {f578346f-1e2b-4a0c-944b-aa21e52f3b52}
"Windows 10 (32-bit)" {75c8de41-2512-4ee9-a2bf-0bdc17106a49}
"macOS Sierra 10.12" {3b573a9e-b6f7-4902-afab-7ee627495203}
"macOS Mojave 10.14" {99b5315c-f984-4e82-b9b7-3ce3666231d4}
"Ubuntu Hirsute 21.04" {e49d3b0f-fdcc-4872-b824-d4443a537f0b}
"Fedora" {18b824ec-b9b0-4c61-b765-b3678fe2e468}
"Ubuntu Bionic 18.04" {48450886-47f1-410d-b3e2-fa2c8df5c94a}
"macOS Catalina 10.15" {147c760d-c665-4108-aae3-a6a1d50a5725}
"Debian 9 Stretch" {7c1e869a-d875-4b15-b819-df2cea423524}

:)

…ons by …

f2ef5a8 test: Fix TSan suppression (Hennadii Stepanov) Pull request description: This PR is a #21669 follow up, and fixes [locally running `make check`](bitcoin/bitcoin#21669 (comment)). ACKs for top commit: MarcoFalke: cr ACK f2ef5a8 Tree-SHA512: bb0c4d1707c6194358d2e9abfed5aa8dd487e014199025fb89f6e5a66d774af041b46a03358a9a5412e1683675c05c42a3b719217d940412ee3fe1ed18a5274c

f2ef5a8 test: Fix TSan suppression (Hennadii Stepanov) Pull request description: This PR is a bitcoin#21669 follow up, and fixes [locally running `make check`](bitcoin#21669 (comment)). ACKs for top commit: MarcoFalke: cr ACK f2ef5a8 Tree-SHA512: bb0c4d1707c6194358d2e9abfed5aa8dd487e014199025fb89f6e5a66d774af041b46a03358a9a5412e1683675c05c42a3b719217d940412ee3fe1ed18a5274c

test: Remove spurious double lock tsan suppressions by bumping to cla…

fadbd99

…ng-12

DrahtBot added the Tests label Apr 13, 2021

maflcko force-pushed the 2104-tsanSupp branch from 8b3ce81 to fadbd99 Compare April 13, 2021 18:47

fanquake requested a review from practicalswift April 14, 2021 02:15

Revert "test: Add tsan supp for leveldb::DBImpl::DeleteObsoleteFiles"

fadea0b

I can no longer observe the need for this suppression. This reverts commit fa1fc53.

practicalswift approved these changes Apr 14, 2021

View reviewed changes

maflcko merged commit b8e5bbd into bitcoin:master Apr 14, 2021

maflcko deleted the 2104-tsanSupp branch April 14, 2021 07:26

hebasto mentioned this pull request Apr 14, 2021

test: Fix TestPotentialDeadLockDetected suppression #21678

Merged

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 14, 2021

Merge bitcoin#21669: test: Remove spurious double lock tsan suppressi…

2216a22

…ons by …

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: Remove spurious double lock tsan suppressions by bumping to clang-12 #21669

test: Remove spurious double lock tsan suppressions by bumping to clang-12 #21669

maflcko commented Apr 13, 2021

hebasto commented Apr 13, 2021

maflcko commented Apr 13, 2021

practicalswift left a comment •

edited

Loading

hebasto commented Apr 14, 2021

hebasto commented Apr 14, 2021

hebasto commented Apr 14, 2021

hebasto commented Apr 14, 2021

maflcko commented Apr 14, 2021

hebasto commented Apr 14, 2021

maflcko commented Apr 14, 2021 •

edited

Loading

hebasto commented Apr 14, 2021

test: Remove spurious double lock tsan suppressions by bumping to clang-12 #21669

test: Remove spurious double lock tsan suppressions by bumping to clang-12 #21669

Conversation

maflcko commented Apr 13, 2021

hebasto commented Apr 13, 2021

maflcko commented Apr 13, 2021

practicalswift left a comment • edited Loading

Choose a reason for hiding this comment

hebasto commented Apr 14, 2021

hebasto commented Apr 14, 2021

hebasto commented Apr 14, 2021

hebasto commented Apr 14, 2021

maflcko commented Apr 14, 2021

hebasto commented Apr 14, 2021

maflcko commented Apr 14, 2021 • edited Loading

hebasto commented Apr 14, 2021

practicalswift left a comment •

edited

Loading

maflcko commented Apr 14, 2021 •

edited

Loading