What does this change? Why?

@sipa Looks like it extends tsan checks to include checks on std::thread handling etc.

Adding libc++-dev seems to be a way to fix the build: https://packages.ubuntu.com/xenial/amd64/libc++-dev/filelist

How about running both the clang-9 wallet+qt version and the clang-10 libc++? Or is there not meaningful threading to justify it?

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• wallet: Add sqlite as an alternative wallet database and use it for new descriptor wallets #19077 (wallet: Add sqlite as an alternative wallet database and use it for new descriptor wallets by achow101)
• net: Add NAT-PMP port forwarding support #18077 (net: Add NAT-PMP port forwarding support by hebasto)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

This fails because the suppressions can not be applied:

==8563==WARNING: invalid path to external symbolizer!

==8563==WARNING: Failed to use and restart external symbolizer!

==================

WARNING: ThreadSanitizer: double lock of a mutex (pid=8563)

    #0 <null> <null> (bitcoind+0x7e8d6)

    #1 <null> <null> (libc++.so.1+0x83505)

    #2 <null> <null> (bitcoind+0x13458a)

    #3 <null> <null> (bitcoind+0x360bdc)

    #4 <null> <null> (bitcoind+0x36b491)

    #5 <null> <null> (bitcoind+0x36af3e)

    #6 <null> <null> (bitcoind+0x3334e0)

    #7 <null> <null> (bitcoind+0x38568a)

    #8 <null> <null> (bitcoind+0x11a65a)

    #9 <null> <null> (bitcoind+0x600b6e)

  Location is global '<null>' at 0x000000000000 (bitcoind+0x000001347f30)

  Mutex M131381 (0x55c3485def30) created at:

    #0 <null> <null> (bitcoind+0x7e8d6)

    #1 <null> <null> (libc++.so.1+0x83505)

    #2 <null> <null> (bitcoind+0xf136c)

    #3 <null> <null> (libc.so.6+0x270b2)

SUMMARY: ThreadSanitizer: double lock of a mutex (/home/travis/build/bitcoin/bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/bitcoind+0x7e8d6) 

==================

Anyone knows how to fix this? @practicalswift maybe?

I already tried setting PATH to $PATH:/usr/lib/llvm-10/bin/ to add llvm-symbolizer directory to PATH, but that didn't help

Fixed all issues. Travis green on the tsan run. This is ready for review. The changes are split out into separate micro-commits, so should be easier to review.

ACK faf62e6

Given the number of new suppressions I suggest also opening an issue for tackling them :)

Concept ACK.

Could removing llvm path in faf62e6 affect other builds with sanitizers?

I still not fully understand what is (potentially) wrong with code which emits "double locks" warnings about recursive mutexes. I can easy reproduce such warning but it does not give answers though.

Could someone point me to resources to get knowledge?

@hebasto Recursive mutexes are generally considered to be a bad idea. They work, and do what they're designed for - reentering a mutex you already hold is perfectly legal with them. However, they're generally a sign of badly structured code. In well-designed systems, you have code running inside the mutex, and code outside, and there is a clean barrier between the two. The fact that you need a recursive mutex means you have code that's trying to be useful in both, only making the boundary unclear.

Could removing llvm path in faf62e6 affect other builds with sanitizers?

No, because the PATH is never passed into the ci container (docker) anyway and is therefore unset either way.

I still not fully understand what is (potentially) wrong with code which emits "double locks" warnings about recursive mutexes. I can easy reproduce such warning but it does not give answers though.

I checked the first warning and it is about a plain mutex, not recursive one:

src/init.cpp:static Mutex g_genesis_wait_mutex;

I suggest we simply accept the suppressions file for now and then work our way out of this by

• removing one line in the suppressions file
• observing the warning again
• fixing the bug
• observing that the warning is gone and create a pull request

maybe re-organize commits to modify suppressions in a single one?

I have a slight preference to keep it as is:

• The commits don't mention that, but the first one is a result of running tsan in Fedora, the second commit is the result of running tsan in Focal
• Not invalidate reviews :)

@MarcoFalke

The commits don't mention that, but the first one is a result of running tsan in Fedora, the second commit is the result of running tsan in Focal

Does this mean we should run CI test on Fedora as well?