ci: Enable tsan on linux64 build #4563

gabriel-bjg · 2021-11-02T16:20:39Z

This pull request enables thread sanitiser for linux64 build (including unit tests & functional tests) and fixes/suppresses the following:

Switch ubsan to run on linux32 as tsan can't run there (linker does not support thread sanitizer)
Fix data race in CCoinJoinClientManager (triggered by concurrent access on fMixing)
Suppress data races triggered in zmq namespace (same suppression exists in bitcoin)
Suppress deadlock false positive the sme way bitcoin does it (i.e. deadlock:CChainState::ConnectTip). False positive description:

One thread:
generateBlocks|mining.cpp:124
    CreateNewBlock
        LOCK2(cs_main, cs_mempool) | called from miner.cpp:128
            TestBlockValidity | called from miner.cpp:235
                    ConnectBlock | called from validation.cpp:4200
                        ProcessSpecialTxsInBlock | called from validation.cpp:2123
                            […]
                                NotifyMasternodeListChanged
                                    UpdateCachesAndClean
                                        LOCK(cs_main, cs_governance) | called from governance/governance.cpp:352
Lock order: cs_main, cs_mempool, cs_governance

The other thread:
AddGovernanceObject
    LOCK2(cs_main, cs_governance) | called from governance/governance.cpp:293
        IsValidLocally | called from governance/governance.cpp:298
            […]
                GetTransaction
                    mempool.get(hash) | called from validation.cpp:893
                        LOCK(cs_mempool) | called from txmempool.cpp:1219
Lock order: cs_main, cs_governance, cs_mempool

This kind of lock order inversion looks like a bug in tsan (similar known issue: google/sanitizers#814) and can be easily reproduced with 3 locks by changing the order of the last 2 locks: thread1 locks A, B, C and thread2 locks A, C, B (assuming a reversed unlocking order). Godbolt example here: https://godbolt.org/z/81Y6frYKv (same example with std::scoped_lock https://godbolt.org/z/175x6Yv9h)

I will keep the PR in draft until everything is green and passing in gitlab ci.

test/sanitizer_suppressions/tsan

PastaPastaPasta · 2021-11-02T21:00:57Z

I question whether we should retain one run of functional tests that doesn't include any sanitizer so that it is nice and fast..

gabriel-bjg · 2021-11-03T17:10:31Z

I question whether we should retain one run of functional tests that doesn't include any sanitizer so that it is nice and fast..

I could do that, but we need a target without sanitizers at all and a new job for testing as tsan and ubsan can't run together (see: google/sanitizers#1106). We could use the linux64_cxx17 build target (with thread sanitizer removed from there) and create a new testing job derived from it, so we would have 3 testing jobs: linux32-test (ubsan), linux64-test (tsan), linux64-test-no-sanitizers (we can discuss to improve their namings).

PastaPastaPasta · 2021-11-03T17:15:21Z

I question whether we should retain one run of functional tests that doesn't include any sanitizer so that it is nice and fast..

I could do that, but we need a target without sanitizers at all and a new job for testing as tsan and ubsan can't run together (see: google/sanitizers#1106). We could use the linux64_cxx17 build target (with thread sanitizer removed from there) and create a new testing job derived from it, so we would have 3 testing jobs: linux32-test (ubsan), linux64-test (tsan), linux64-test-no-sanitizers (we can discuss to improve their namings).

Yes, I think it makes sense to have the following
linux64 - pure linux64, no sanitizers, unit and functional tests
linux64-thread - linux64, thread sanitizer, unit and functional tests
linux32-undef - linux32, undefined sanitizer, unit and functional tests

github-actions · 2021-11-30T11:14:45Z

This pull request has conflicts, please rebase.

UdjinM6 · 2021-12-12T16:57:17Z

pls see 3748656 (NOTE: I kept pure linux32 build/tests untouched just in case - they should not slow things down that much and we can always drop them later if needed)

UdjinM6

One tiny issue, LGTM otherwise

.gitlab-ci.yml

UdjinM6

utACK

PastaPastaPasta

LGTM, one issue

PastaPastaPasta · 2021-12-28T04:48:00Z

src/coinjoin/client.h

@@ -178,7 +179,7 @@ class CCoinJoinClientManager
    // TODO: or map<denom, CCoinJoinClientSession> ??
    std::deque<CCoinJoinClientSession> deqSessions GUARDED_BY(cs_deqsessions);

-    bool fMixing{false};
+    std::atomic<bool> fMixing{false};


you should update

dash/src/coinjoin/client.cpp

Lines 238 to 241 in c46306a

if (IsMixing()) {

return false;

}

return fMixing = true;

to be actually atomic (likely use compare_exchange)?

github-actions · 2021-12-28T18:10:37Z

This pull request has conflicts, please rebase.

PastaPastaPasta

utACK for squash merge

PastaPastaPasta

feature_llmq_signing.py failed twice in ci so far..

gabriel-bjg · 2022-01-07T19:34:49Z

feature_llmq_signing.py failed twice in ci so far..

I tried to reproduce locally, but after running the test that's failing for 2 hours in a loop I gave up. I increased the timeout of wait_for_sigs on the failing testcase (https://github.com/dashpay/dash/pull/4563/files#diff-78ac439373346c70f2c3236e1f34e274df611f2f2367bef84464bd0e59226940R106) and it does not fail anymore in the CI.

PastaPastaPasta

utACK for squash merge

restarted ci test run, hopefully it passes

test/functional/feature_llmq_signing.py

Make fMixing atomic as it has concurrent access Add tsan suppression for zmq namespace Suppress deadlock false positive in ConnectTip Switch ubsan target to linux32 Add new test job for linux64_cxx17 target without any sanitizers Increase rpc time out for block reward reallocation test Fix heap use after free in CConnman::GetExtraOutboundCount() Different builds for linux32 and linux64 tsan and ubsan Increase timeout for llmq_signing functional test

UdjinM6

utACK

PastaPastaPasta

utACK for squash merge

Make fMixing atomic as it has concurrent access Add tsan suppression for zmq namespace Suppress deadlock false positive in ConnectTip Switch ubsan target to linux32 Add new test job for linux64_cxx17 target without any sanitizers Increase rpc time out for block reward reallocation test Fix heap use after free in CConnman::GetExtraOutboundCount() Different builds for linux32 and linux64 tsan and ubsan Increase timeout for llmq_signing functional test

gabriel-bjg force-pushed the thread-sanitizers-in-gitlab-ci branch from 5d891a6 to d391967 Compare November 2, 2021 20:31

PastaPastaPasta reviewed Nov 2, 2021

View reviewed changes

test/sanitizer_suppressions/tsan Outdated Show resolved Hide resolved

gabriel-bjg force-pushed the thread-sanitizers-in-gitlab-ci branch from d391967 to 0d6c51b Compare November 3, 2021 17:04

gabriel-bjg force-pushed the thread-sanitizers-in-gitlab-ci branch 7 times, most recently from 7462c2f to 12f2c27 Compare November 4, 2021 19:30

github-actions bot added the needs rebase label Nov 30, 2021

gabriel-bjg force-pushed the thread-sanitizers-in-gitlab-ci branch from 12f2c27 to 4a1b159 Compare December 12, 2021 08:40

github-actions bot removed the needs rebase label Dec 12, 2021

gabriel-bjg force-pushed the thread-sanitizers-in-gitlab-ci branch from 4a1b159 to 55d2233 Compare December 16, 2021 09:21

UdjinM6 reviewed Dec 17, 2021

View reviewed changes

.gitlab-ci.yml Outdated Show resolved Hide resolved

gabriel-bjg force-pushed the thread-sanitizers-in-gitlab-ci branch 3 times, most recently from 9ba7ec9 to 65a77d1 Compare December 22, 2021 17:00

gabriel-bjg marked this pull request as ready for review December 22, 2021 18:57

UdjinM6 previously approved these changes Dec 23, 2021

View reviewed changes

PastaPastaPasta requested changes Dec 28, 2021

View reviewed changes

github-actions bot added the needs rebase label Dec 28, 2021

gabriel-bjg dismissed UdjinM6’s stale review via 1c5e378 January 4, 2022 15:43

gabriel-bjg force-pushed the thread-sanitizers-in-gitlab-ci branch from 65a77d1 to 1c5e378 Compare January 4, 2022 15:43

github-actions bot removed the needs rebase label Jan 4, 2022

gabriel-bjg force-pushed the thread-sanitizers-in-gitlab-ci branch 2 times, most recently from a74691f to c5c0eaf Compare January 5, 2022 09:27

PastaPastaPasta approved these changes Jan 6, 2022

View reviewed changes

PastaPastaPasta requested changes Jan 6, 2022

View reviewed changes

gabriel-bjg force-pushed the thread-sanitizers-in-gitlab-ci branch from c5c0eaf to 5209f25 Compare January 6, 2022 17:28

gabriel-bjg changed the title ~~Enable tsan on linux32 build~~ Enable tsan on linux64 build Jan 6, 2022

gabriel-bjg force-pushed the thread-sanitizers-in-gitlab-ci branch from 5209f25 to e8cc8e9 Compare January 6, 2022 19:13

PastaPastaPasta previously approved these changes Jan 7, 2022

View reviewed changes

UdjinM6 requested changes Jan 9, 2022

View reviewed changes

test/functional/feature_llmq_signing.py Outdated Show resolved Hide resolved

gabriel-bjg dismissed PastaPastaPasta’s stale review via 99f8609 January 9, 2022 14:44

gabriel-bjg force-pushed the thread-sanitizers-in-gitlab-ci branch from e8cc8e9 to 99f8609 Compare January 9, 2022 14:44

UdjinM6 approved these changes Jan 10, 2022

View reviewed changes

UdjinM6 added this to the 18 milestone Jan 10, 2022

PastaPastaPasta approved these changes Jan 10, 2022

View reviewed changes

PastaPastaPasta changed the title ~~Enable tsan on linux64 build~~ ci: Enable tsan on linux64 build Jan 10, 2022

PastaPastaPasta merged commit f73fce7 into dashpay:develop Jan 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ci: Enable tsan on linux64 build #4563

ci: Enable tsan on linux64 build #4563

gabriel-bjg commented Nov 2, 2021 •

edited

Loading

PastaPastaPasta commented Nov 2, 2021

gabriel-bjg commented Nov 3, 2021

PastaPastaPasta commented Nov 3, 2021

github-actions bot commented Nov 30, 2021

UdjinM6 commented Dec 12, 2021

UdjinM6 left a comment

UdjinM6 left a comment

PastaPastaPasta left a comment

PastaPastaPasta Dec 28, 2021

github-actions bot commented Dec 28, 2021

PastaPastaPasta left a comment

PastaPastaPasta left a comment

gabriel-bjg commented Jan 7, 2022

PastaPastaPasta left a comment

UdjinM6 left a comment

PastaPastaPasta left a comment

ci: Enable tsan on linux64 build #4563

ci: Enable tsan on linux64 build #4563

Conversation

gabriel-bjg commented Nov 2, 2021 • edited Loading

PastaPastaPasta commented Nov 2, 2021

gabriel-bjg commented Nov 3, 2021

PastaPastaPasta commented Nov 3, 2021

github-actions bot commented Nov 30, 2021

UdjinM6 commented Dec 12, 2021

UdjinM6 left a comment

Choose a reason for hiding this comment

UdjinM6 left a comment

Choose a reason for hiding this comment

PastaPastaPasta left a comment

Choose a reason for hiding this comment

PastaPastaPasta Dec 28, 2021

Choose a reason for hiding this comment

github-actions bot commented Dec 28, 2021

PastaPastaPasta left a comment

Choose a reason for hiding this comment

PastaPastaPasta left a comment

Choose a reason for hiding this comment

gabriel-bjg commented Jan 7, 2022

PastaPastaPasta left a comment

Choose a reason for hiding this comment

UdjinM6 left a comment

Choose a reason for hiding this comment

PastaPastaPasta left a comment

Choose a reason for hiding this comment

gabriel-bjg commented Nov 2, 2021 •

edited

Loading