ci: tsan with -stdlib=libc++-10 #19041

maflcko · 2020-05-21T16:33:45Z

According to the ThreadSanitizer docs:

C++11 threading is supported with llvm libc++.

For example, the thread sanitizer build is currently not checking for double lock of mutexes.

Fixes (partially) #19038 (comment)

sipa · 2020-05-21T16:52:19Z

What does this change? Why?

Empact · 2020-05-26T21:04:50Z

@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?

DrahtBot · 2020-05-27T20:14:53Z

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.

maflcko · 2020-05-30T13:19:31Z

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

maflcko · 2020-05-31T23:05:40Z

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.

practicalswift · 2020-06-01T07:39:47Z

ACK faf62e6

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

hebasto · 2020-06-01T08:48:17Z

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?

sipa · 2020-06-01T09:00:14Z

@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.

maflcko · 2020-06-01T10:41:41Z

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.

maflcko · 2020-06-01T10:44:41Z

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

hebasto

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

maflcko · 2020-06-02T22:51:07Z

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 :)

fanquake

Haven't tested anything locally but commits all look ok. Can ACK once my comment has been addressed.

fanquake · 2020-06-03T08:58:53Z

ci/test/05_before_script.sh

@@ -33,7 +33,9 @@ if [ -z "$NO_DEPENDS" ]; then
  else
    SHELL_OPTS="CONFIG_SHELL="
  fi
-  DOCKER_EXEC $SHELL_OPTS make $MAKEJOBS -C depends HOST=$HOST $DEP_OPTS
+  # Temporary workaround for https://github.com/bitcoin/bitcoin/issues/16368


Is this being muted because the depends build in the tsan job is now noisier when building with Clang and libstdc++? Or is this an unrelated commit similar to fac2eee?

It is a combination of all of the qt build issues I've been running into lately.

Not sure how to pass in compiler flags to the qt build from outside to disable the wdeprecated warnings in depends. gui: Building with gcc 9 prints a trillion warnings (-Wdeprecated-copy) #15822

Not sure if we can bump qt to a more recent version to get rid of the warnings QT abandoning open source LTS releases #18580

Not sure how to solve depends: File-based logging for individual packages #16368

I am just hoping that one of you build people will solve this one day 🙏 kthxbye

To limit the changes here to purely the ci folder, I will leave any build changes for the future.

To clarify, yes this commit is required. Otherwise the build log would exceed 40000 lines and be killed.

to the qt build from outside to disable the wdeprecated warnings in depends.

Can you add NO_QT=1 NO_WALLET=1 to DEP_OPTS? Given qt is no-longer used here, and bdb wasn't used beforehand. If anything that should reduce the build output.

Not sure if we can bump qt to a more recent version

That's blocked until c++17, unless someone wants to fix c++11 support for a newer version (5.12) of Qt for macOS.

Regardless of above, I'll try follow up here and improve this for you.

Can you add NO_QT=1 NO_WALLET=1 to DEP_OPTS? Given qt is no-longer used here, and bdb wasn't used beforehand. If anything that should reduce the build output.

Well the goal is to eventually enable tsan for the wallet and gui. I might wait for sqlite to avoid the bdb horror, but at least qt will be tested against hopefully soon.

Also, it is nice to have a test for the "can I compile qt depends with clang", even though the build result is thrown away.

That's blocked until c++17, unless someone wants to fix c++11 support for a newer version (5.12) of Qt for macOS.

Thanks, didn't know that they switched away from cxx11

Thanks, didn't know that they switched away from cxx11

heh. They didn't "officially". They just started using c++14 features in portions of the macOS code, while claiming to still support c++11. Some details in #17768.

fanquake

ACK faf62e6

faf62e6 ci: Remove unused workaround (MarcoFalke) fa7c850 ci: Install llvm to get llvm symbolizer (MarcoFalke) fa563ce test: Add more tsan suppressions (MarcoFalke) fa0cc02 ci: Mute depends logs completely (MarcoFalke) fa906bf test: Extend tsan suppressions for clang stdlib (MarcoFalke) fa10d85 ci: Use libc++ instead of libstdc++ for tsan (MarcoFalke) fa0d5ee ci: Set halt_on_error=1 for tsan (MarcoFalke) fa2ffe8 ci: Deduplicate DOCKER_EXEC (MarcoFalke) fac2eee cirrus: Remove no longer needed install step (MarcoFalke) Pull request description: According to the [ThreadSanitizer docs](https://clang.llvm.org/docs/ThreadSanitizer.html#current-status): > C++11 threading is supported with **llvm libc++**. For example, the thread sanitizer build is currently not checking for double lock of mutexes. Fixes (partially) bitcoin#19038 (comment) ACKs for top commit: practicalswift: ACK faf62e6 fanquake: ACK faf62e6 hebasto: ACK faf62e6, maybe re-organize commits to modify suppressions in a single one? Tree-SHA512: 98ce5154b4736dfb811ffdb6e6f63a7bc25fe50d3b73134404a8f3715ad53626c31f9c8132dbacf85de47b9409f1e17a4399e35f78b1da30b1577167ea2982ad

hebasto · 2020-06-04T06:48:00Z

@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?

Summary: ~~ci: Remove unused workaround (MarcoFalke)~~ ~~ci: Install llvm to get llvm symbolizer (MarcoFalke)~~ test: Add more tsan suppressions (MarcoFalke) ~~ci: Mute depends logs completely (MarcoFalke)~~ test: Extend tsan suppressions for clang stdlib (MarcoFalke) ~~ci: Use libc++ instead of libstdc++ for tsan (MarcoFalke)~~ ~~ci: Set halt_on_error=1 for tsan (MarcoFalke)~~ ~~ci: Deduplicate DOCKER_EXEC (MarcoFalke)~~ ~~cirrus: Remove no longer needed install step (MarcoFalke)~~ Pull request description: According to the [ThreadSanitizer docs](https://clang.llvm.org/docs/ThreadSanitizer.html#current-status): > C++11 threading is supported with **llvm libc++**. For example, the thread sanitizer build is currently not checking for double lock of mutexes. Fixes (partially) bitcoin/bitcoin#19038 (comment) --- EDIT: included our own code's suppressions so the test is green Backport of Core [[bitcoin/bitcoin#19041 | PR19041]] Test Plan: with clang-10 on archlinux: cmake -GNinja -DENABLE_SANITIZERS=thread -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ ninja check check-functional threadsanitizer is green ~~only gives out warnings about code that is exclusive to us (specifically, rcu_tests.cpp and doublelock of mutex in paymentserver qt code)~~ Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Subscribers: deadalnix Differential Revision: https://reviews.bitcoinabc.org/D7547

…ons by bumping to clang-12 fadea0b Revert "test: Add tsan supp for leveldb::DBImpl::DeleteObsoleteFiles" (MarcoFalke) fadbd99 test: Remove spurious double lock tsan suppressions by bumping to clang-12 (MarcoFalke) Pull request description: The double lock warnings appeared in bitcoin#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. ACKs for top commit: practicalswift: cr ACK fadea0b assuming CI passes and more specifically that newer Clang agrees that these TSan suppressions are no longer needed. Tree-SHA512: c411221a4b74d0af6ca8d686639b4f40b41c15906ccbb6647e8d569d6ab088264faafe075e1ac9523d5c0024b85f15a597bb3eedc7f07d4f5816245f75cfc08b

maflcko force-pushed the 2005-ciTsanFix branch from fa29dfb to 279da39 Compare May 21, 2020 16:49

maflcko force-pushed the 2005-ciTsanFix branch from 279da39 to 312851d Compare May 21, 2020 17:02

maflcko marked this pull request as draft May 21, 2020 17:12

maflcko force-pushed the 2005-ciTsanFix branch from 312851d to ea5677d Compare May 21, 2020 17:20

DrahtBot added the Tests label May 21, 2020

DrahtBot mentioned this pull request May 27, 2020

wallet: Add sqlite as an alternative wallet database and use it for new descriptor wallets #19077

Merged

DrahtBot mentioned this pull request May 27, 2020

net: Add NAT-PMP port forwarding support #18077

Merged

maflcko force-pushed the 2005-ciTsanFix branch 10 times, most recently from e241ce7 to cab0245 Compare May 30, 2020 11:56

maflcko changed the title ~~ci: tsan with -stdlib=libc++~~ ci: tsan with -stdlib=libc++-10 May 30, 2020

maflcko marked this pull request as ready for review May 30, 2020 11:59

MarcoFalke added 6 commits May 30, 2020 08:32

cirrus: Remove no longer needed install step

fac2eee

ci: Deduplicate DOCKER_EXEC

fa2ffe8

ci: Set halt_on_error=1 for tsan

fa0d5ee

ci: Use libc++ instead of libstdc++ for tsan

fa10d85

test: Extend tsan suppressions for clang stdlib

fa906bf

ci: Mute depends logs completely

fa0cc02

maflcko force-pushed the 2005-ciTsanFix branch from cab0245 to 8565e2e Compare May 30, 2020 16:55

maflcko mentioned this pull request May 31, 2020

UB: double lock of mutex in init.cpp #19024

Closed

test: Add more tsan suppressions

fa563ce

maflcko force-pushed the 2005-ciTsanFix branch 2 times, most recently from 08052c2 to fa1a893 Compare May 31, 2020 19:47

MarcoFalke added 2 commits May 31, 2020 18:37

ci: Install llvm to get llvm symbolizer

fa7c850

ci: Remove unused workaround

faf62e6

maflcko force-pushed the 2005-ciTsanFix branch from 2222ba3 to faf62e6 Compare May 31, 2020 22:38

maflcko mentioned this pull request Jun 1, 2020

build: Add MemorySanitizer (MSan) in Travis to detect use of uninitialized memory #18288

Merged

hebasto reviewed Jun 1, 2020

View reviewed changes

fanquake reviewed Jun 3, 2020

View reviewed changes

fanquake approved these changes Jun 3, 2020

View reviewed changes

fanquake merged commit 575589a into bitcoin:master Jun 3, 2020

maflcko deleted the 2005-ciTsanFix branch June 3, 2020 14:03

hebasto mentioned this pull request Jul 5, 2020

doc: Mention libc++ for ThreadSanitizer #19447

Closed

maflcko mentioned this pull request Aug 27, 2020

test: Data race detected when running rpc_blockchain.py #19712

Closed

maflcko mentioned this pull request Apr 13, 2021

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

Merged

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ci: tsan with -stdlib=libc++-10 #19041

ci: tsan with -stdlib=libc++-10 #19041

maflcko commented May 21, 2020 •

edited

Loading

sipa commented May 21, 2020

Empact commented May 26, 2020

DrahtBot commented May 27, 2020 •

edited

Loading

maflcko commented May 30, 2020

maflcko commented May 31, 2020

practicalswift commented Jun 1, 2020

hebasto commented Jun 1, 2020

sipa commented Jun 1, 2020

maflcko commented Jun 1, 2020

maflcko commented Jun 1, 2020

hebasto left a comment

maflcko commented Jun 2, 2020

fanquake left a comment

fanquake Jun 3, 2020

maflcko Jun 3, 2020

maflcko Jun 3, 2020

maflcko Jun 3, 2020

fanquake Jun 3, 2020

maflcko Jun 3, 2020

maflcko Jun 3, 2020

fanquake Jun 3, 2020

fanquake left a comment

hebasto commented Jun 4, 2020

ci: tsan with -stdlib=libc++-10 #19041

ci: tsan with -stdlib=libc++-10 #19041

Conversation

maflcko commented May 21, 2020 • edited Loading

sipa commented May 21, 2020

Empact commented May 26, 2020

DrahtBot commented May 27, 2020 • edited Loading

Conflicts

maflcko commented May 30, 2020

maflcko commented May 31, 2020

practicalswift commented Jun 1, 2020

hebasto commented Jun 1, 2020

sipa commented Jun 1, 2020

maflcko commented Jun 1, 2020

maflcko commented Jun 1, 2020

hebasto left a comment

Choose a reason for hiding this comment

maflcko commented Jun 2, 2020

fanquake left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fanquake left a comment

Choose a reason for hiding this comment

hebasto commented Jun 4, 2020

maflcko commented May 21, 2020 •

edited

Loading

DrahtBot commented May 27, 2020 •

edited

Loading