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

MarcoFalke commented at 4:33 pm on May 21, 2020: member

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)

MarcoFalke force-pushed on May 21, 2020

sipa commented at 4:52 pm on May 21, 2020: member

What does this change? Why?

MarcoFalke force-pushed on May 21, 2020

MarcoFalke marked this as a draft on May 21, 2020

MarcoFalke force-pushed on May 21, 2020

DrahtBot added the label Tests on May 21, 2020

Empact commented at 9:04 pm on May 26, 2020: member

@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 commented at 8:14 pm on May 27, 2020: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#19077 (wallet: Add sqlite as an alternative wallet database and use it for new descriptor wallets by achow101)
#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.

MarcoFalke force-pushed on May 28, 2020

MarcoFalke force-pushed on May 29, 2020

MarcoFalke force-pushed on May 30, 2020

MarcoFalke renamed this:
~~ci: tsan with -stdlib=libc++~~
ci: tsan with -stdlib=libc++-10
on May 30, 2020

MarcoFalke marked this as ready for review on May 30, 2020

cirrus: Remove no longer needed install step fac2eeeb9d

ci: Deduplicate DOCKER_EXEC fa2ffe87f7

ci: Set halt_on_error=1 for tsan fa0d5ee112

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

test: Extend tsan suppressions for clang stdlib fa906bf298

ci: Mute depends logs completely fa0cc02c0a

MarcoFalke commented at 1:19 pm on May 30, 2020: member

This fails because the suppressions can not be applied:

 0==8563==WARNING: invalid path to external symbolizer!
 1
 2==8563==WARNING: Failed to use and restart external symbolizer!
 3
 4==================
 5
 6WARNING: ThreadSanitizer: double lock of a mutex (pid=8563)
 7
 8    [#0](/bitcoin-bitcoin/0/) <null> <null> (bitcoind+0x7e8d6)
 9
10    [#1](/bitcoin-bitcoin/1/) <null> <null> (libc++.so.1+0x83505)
11
12    [#2](/bitcoin-bitcoin/2/) <null> <null> (bitcoind+0x13458a)
13
14    [#3](/bitcoin-bitcoin/3/) <null> <null> (bitcoind+0x360bdc)
15
16    [#4](/bitcoin-bitcoin/4/) <null> <null> (bitcoind+0x36b491)
17
18    [#5](/bitcoin-bitcoin/5/) <null> <null> (bitcoind+0x36af3e)
19
20    [#6](/bitcoin-bitcoin/6/) <null> <null> (bitcoind+0x3334e0)
21
22    [#7](/bitcoin-bitcoin/7/) <null> <null> (bitcoind+0x38568a)
23
24    [#8](/bitcoin-bitcoin/8/) <null> <null> (bitcoind+0x11a65a)
25
26    [#9](/bitcoin-bitcoin/9/) <null> <null> (bitcoind+0x600b6e)
27
28  Location is global '<null>' at 0x000000000000 (bitcoind+0x000001347f30)
29
30  Mutex M131381 (0x55c3485def30) created at:
31
32    [#0](/bitcoin-bitcoin/0/) <null> <null> (bitcoind+0x7e8d6)
33
34    [#1](/bitcoin-bitcoin/1/) <null> <null> (libc++.so.1+0x83505)
35
36    [#2](/bitcoin-bitcoin/2/) <null> <null> (bitcoind+0xf136c)
37
38    [#3](/bitcoin-bitcoin/3/) <null> <null> (libc.so.6+0x270b2)
39
40SUMMARY: ThreadSanitizer: double lock of a mutex (/home/travis/build/bitcoin/bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/bitcoind+0x7e8d6) 
41
42==================

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

MarcoFalke force-pushed on May 30, 2020

test: Add more tsan suppressions fa563cef61

MarcoFalke force-pushed on May 31, 2020

ci: Install llvm to get llvm symbolizer fa7c850915

ci: Remove unused workaround faf62e6ed0

MarcoFalke force-pushed on May 31, 2020

MarcoFalke commented at 11:05 pm on May 31, 2020: member

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 commented at 7:39 am on June 1, 2020: contributor

ACK faf62e6ed0ca45db44c370844c3515eb5a8cda12

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

hebasto commented at 8:48 am on June 1, 2020: member

Concept ACK.

Could removing llvm path in faf62e6ed0ca45db44c370844c3515eb5a8cda12 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 commented at 9:00 am on June 1, 2020: member

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

MarcoFalke commented at 10:41 am on June 1, 2020: member

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.

MarcoFalke commented at 10:44 am on June 1, 2020: member

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:

0src/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 commented at 10:57 am on June 1, 2020: member

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

MarcoFalke commented at 10:51 pm on June 2, 2020: member

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

in ci/test/05_before_script.sh:36 in faf62e6ed0

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

fanquake commented at 8:58 am on June 3, 2020:

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

MarcoFalke commented at 9:42 am on June 3, 2020:

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. #15822
Not sure if we can bump qt to a more recent version to get rid of the warnings #18580
Not sure how to solve #16368

MarcoFalke commented at 9:43 am on June 3, 2020:

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

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

MarcoFalke commented at 11:17 am on June 3, 2020:

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

fanquake commented at 1:52 pm on June 3, 2020:

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.

MarcoFalke commented at 2:02 pm on June 3, 2020:

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.

MarcoFalke commented at 2:03 pm on June 3, 2020:

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

fanquake commented at 2:05 pm on June 3, 2020:

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 commented at 9:00 am on June 3, 2020: member

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

fanquake approved

fanquake commented at 1:54 pm on June 3, 2020: member

ACK faf62e6ed0ca45db44c370844c3515eb5a8cda12

fanquake merged this on Jun 3, 2020

fanquake closed this on Jun 3, 2020

MarcoFalke deleted the branch on Jun 3, 2020

sidhujag referenced this in commit 07d81bbfb9 on Jun 3, 2020

hebasto commented at 6:48 am on June 4, 2020: member

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

jasonbcox referenced this in commit 93237957b8 on Sep 24, 2020

MarcoFalke referenced this in commit b8e5bbdf93 on Apr 14, 2021

DrahtBot locked this on Feb 15, 2022

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

Conflicts