Drop some TSan suppressions #19983

pull hebasto wants to merge 1 commits into bitcoin:master from hebasto:200920-supp changing 1 files +0 −6
  1. hebasto commented at 2:35 pm on September 20, 2020: member
    It seems possible now to drop some TSan suppressions.
  2. DrahtBot added the label Wallet on Sep 20, 2020
  3. hebasto force-pushed on Sep 20, 2020
  4. practicalswift commented at 3:36 pm on September 20, 2020: contributor

    Concept ACK

    Great to see deadlock:UpdateTip go :)

  5. hebasto marked this as ready for review on Sep 20, 2020
  6. hebasto commented at 4:15 pm on September 20, 2020: member

    @practicalswift

    Great to see deadlock:UpdateTip go :)

    I must confess that, unlike #19982, I’m not as much confident in this PR removals as all of them are found empirically.

  7. hebasto commented at 5:28 pm on September 20, 2020: member

    Some more CI TSan builds:

  8. DrahtBot commented at 6:45 pm on September 20, 2020: member

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

    Conflicts

    No conflicts as of last run.

  9. hebasto force-pushed on Sep 21, 2020
  10. DrahtBot added the label Needs rebase on Nov 12, 2020
  11. hebasto force-pushed on Nov 14, 2020
  12. hebasto commented at 6:36 pm on November 14, 2020: member
    Rebased 49c3a5287d74d3739724d9f7f6c7cff3ec398330 -> 730e5ffc2e9f86fded9283543c0e8dc0759d6c6f (pr19983.02 -> pr19983.03) due to the conflict with #20285.
  13. DrahtBot removed the label Needs rebase on Nov 14, 2020
  14. hebasto force-pushed on Nov 15, 2020
  15. DrahtBot added the label Needs rebase on Nov 25, 2020
  16. hebasto force-pushed on Nov 28, 2020
  17. hebasto force-pushed on Nov 28, 2020
  18. hebasto commented at 8:10 pm on November 28, 2020: member
    Rebased 71518e39d8da6b55e0ad04acc0b6781290573425 -> 27752af19a701702e94fbf721a4ba2d2c0a7d0ea (pr19983.04 -> pr19983.06) due to the conflict with #19337.
  19. DrahtBot removed the label Needs rebase on Nov 28, 2020
  20. hebasto force-pushed on Dec 3, 2020
  21. DrahtBot added the label Needs rebase on Dec 8, 2020
  22. hebasto force-pushed on Dec 10, 2020
  23. hebasto commented at 6:51 pm on December 10, 2020: member
    Rebased 3c2e969172cbb2796441eb218e5102a19a1bfd5d -> 89952ae75cc1f0c2e7ec225bbef074f411e3e327 (pr19983.07 -> pr19983.08) due to the conflict with #19425.
  24. DrahtBot removed the label Needs rebase on Dec 10, 2020
  25. hebasto force-pushed on Dec 11, 2020
  26. hebasto commented at 7:39 am on December 11, 2020: member
    Rebased 89952ae75cc1f0c2e7ec225bbef074f411e3e327 -> b0b6092874f717f3156c043e803600938c7ff571 (pr19983.08 -> pr19983.09) due to the conflict with #19982.
  27. practicalswift commented at 2:56 pm on December 11, 2020: contributor
    cr ACK b0b6092874f717f3156c043e803600938c7ff571: patch looks correct and doesn’t touch any non-test code
  28. DrahtBot added the label Needs rebase on Dec 21, 2020
  29. hebasto force-pushed on Dec 22, 2020
  30. hebasto commented at 8:32 am on December 22, 2020: member
    Rebased b0b6092874f717f3156c043e803600938c7ff571 -> a17f69f57e42e08ca95469a9997ec1f49fe2bcf9 (pr19983.09 -> pr19983.10) due to the conflict with #20683.
  31. in test/functional/test_framework/test_node.py:311 in a17f69f57e outdated 
    307@@ -308,7 +308,7 @@ def get_wallet_rpc(self, wallet_name):
308     def version_is_at_least(self, ver):
309         return self.version is None or self.version >= ver
310 
311-    def stop_node(self, expected_stderr='', *, wait=0, wait_until_stopped=True):
312+    def stop_node(self, expected_stderr='', ignored_stderr='', *, wait=0, wait_until_stopped=True):
    MarcoFalke commented at 9:58 am on December 22, 2020: 
    0    def stop_node(self, expected_stderr='', *, ignored_stderr='', wait=0, wait_until_stopped=True):
    hebasto commented at 10:45 am on December 22, 2020:
    Thanks! Updated.
  32. DrahtBot removed the label Needs rebase on Dec 22, 2020
  33. hebasto force-pushed on Dec 22, 2020
  34. hebasto commented at 10:45 am on December 22, 2020: member

    Updated a17f69f57e42e08ca95469a9997ec1f49fe2bcf9 -> 354d2cba44747ca34b6833e0f58901394a03ae49 (pr19983.10 -> pr19983.11, diff):

  35. practicalswift commented at 10:52 am on December 22, 2020: contributor
    cr ACK 354d2cba44747ca34b6833e0f58901394a03ae49: patch looks correct and doesn’t touch any non-test code
  36. Update TSan suppressions 3e1571285f
  37. in test/functional/feature_maxuploadtarget.py:165 in 354d2cba44 outdated 
    160@@ -161,6 +161,9 @@ def run_test(self):
161         assert_equal(len(peer_info), 1)  # node is still connected
162         assert_equal(peer_info[0]['permissions'], ['download'])
163 
164+        # Ignore non-critical TSan warnings.
165+        self.stop_node(0, ignored_stderr="WARNING: too long mutex cycle found")
    MarcoFalke commented at 1:57 pm on December 22, 2020:

    I can’t reproduce this locally. How often did you have to run this test?

    I am running with:

     0diff --git a/test/sanitizer_suppressions/tsan b/test/sanitizer_suppressions/tsan
 1index 986e09605..020f3da9b 100644
 2--- a/test/sanitizer_suppressions/tsan
 3+++ b/test/sanitizer_suppressions/tsan
 4@@ -29,12 +29,7 @@ race:zmq::*
 5 race:bitcoin-qt
 6 # deadlock (TODO fix)
 7 deadlock:CConnman::ForNode
 8-deadlock:CConnman::GetNodeStats
 9 deadlock:CChainState::ConnectTip
10-deadlock:UpdateTip
11-
12-# WalletBatch (unidentified deadlock)
13-deadlock:WalletBatch
14 
15 # Intentional deadlock in tests
16 deadlock:TestPotentialDeadLockDetected
    hebasto commented at 2:06 pm on December 22, 2020:

    I can’t reproduce this locally.

    What is -stdlib compiled against?

    MarcoFalke commented at 2:15 pm on December 22, 2020:
    I used ./ci/test/00_setup_env_native_tsan.sh to compile
    hebasto commented at 3:19 pm on December 22, 2020:

    Indeed. On master (df127ecede804ecb0a70bf45cbca73af3c3e7761) with your patch I have no “WARNING: too long mutex cycle found” from the thread sanitizer when running test/functional/feature_maxuploadtarget.py . Maybe the reason is the updated clang?

    Instead, the skipping -DDEBUG_LOCKORDER from CPPFLAGS causes another warning even on master w/o your patch:

     0$ test/functional/feature_maxuploadtarget.py 
 12020-12-22T15:02:39.792000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_731auno_
 22020-12-22T15:02:57.821000Z TestFramework (INFO): Peer 0 disconnected after downloading old block too many times
 32020-12-22T15:03:42.603000Z TestFramework (INFO): Peer 1 able to repeatedly download new block
 42020-12-22T15:03:42.712000Z TestFramework (INFO): Peer 1 disconnected after trying to download old block
 52020-12-22T15:03:42.712000Z TestFramework (INFO): Advancing system time on node to clear counters...
 62020-12-22T15:03:42.867000Z TestFramework (INFO): Peer 2 able to download old block
 72020-12-22T15:03:42.873000Z TestFramework (INFO): Restarting node 0 with download permission and 1MB maxuploadtarget
 82020-12-22T15:03:45.050000Z TestFramework (INFO): Peer still connected after trying to download old block (download permission)
 92020-12-22T15:03:45.103000Z TestFramework (INFO): Stopping nodes
10Traceback (most recent call last):
11  File "test/functional/feature_maxuploadtarget.py", line 166, in <module>
12    MaxUploadTest().main()
13  File "/home/hebasto/code/gh/bitcoin/test/functional/test_framework/test_framework.py", line 149, in main
14    exit_code = self.shutdown()
15  File "/home/hebasto/code/gh/bitcoin/test/functional/test_framework/test_framework.py", line 278, in shutdown
16    self.stop_nodes()
17  File "/home/hebasto/code/gh/bitcoin/test/functional/test_framework/test_framework.py", line 525, in stop_nodes
18    node.stop_node(wait=wait, wait_until_stopped=False)
19  File "/home/hebasto/code/gh/bitcoin/test/functional/test_framework/test_node.py", line 333, in stop_node
20    raise AssertionError("Unexpected stderr {} != {}".format(stderr, expected_stderr))
21AssertionError: Unexpected stderr ==================
22WARNING: ThreadSanitizer: double lock of a mutex (pid=365627)
23    [#0](/bitcoin-bitcoin/0/) pthread_mutex_lock <null> (bitcoind+0xa8cf6)
24    [#1](/bitcoin-bitcoin/1/) std::__1::mutex::lock() <null> (libc++.so.1+0x83505)
25    [#2](/bitcoin-bitcoin/2/) CConnman::ThreadSocketHandler() /home/hebasto/code/gh/bitcoin/src/net.cpp:1542:9 (bitcoind+0x1780d4)
26    [#3](/bitcoin-bitcoin/3/) decltype(*(std::__1::forward<CConnman*&>(fp0)).*fp()) std::__1::__invoke<void (CConnman::*&)(), CConnman*&, void>(void (CConnman::*&)(), CConnman*&) /usr/lib/llvm-10/bin/../include/c++/v1/type_traits:3480:1 (bitcoind+0x1a325a)
27    [#4](/bitcoin-bitcoin/4/) std::__1::__bind_return<void (CConnman::*)(), std::__1::tuple<CConnman*>, std::__1::tuple<>, __is_valid_bind_return<void (CConnman::*)(), std::__1::tuple<CConnman*>, std::__1::tuple<> >::value>::type std::__1::__apply_functor<void (CConnman::*)(), std::__1::tuple<CConnman*>, 0ul, std::__1::tuple<> >(void (CConnman::*&)(), std::__1::tuple<CConnman*>&, std::__1::__tuple_indices<0ul>, std::__1::tuple<>&&) /usr/lib/llvm-10/bin/../include/c++/v1/functional:2770:12 (bitcoind+0x1a325a)
28    [#5](/bitcoin-bitcoin/5/) std::__1::__bind_return<void (CConnman::*)(), std::__1::tuple<CConnman*>, std::__1::tuple<>, __is_valid_bind_return<void (CConnman::*)(), std::__1::tuple<CConnman*>, std::__1::tuple<> >::value>::type std::__1::__bind<void (CConnman::*)(), CConnman*>::operator()<>() /usr/lib/llvm-10/bin/../include/c++/v1/functional:2803:20 (bitcoind+0x1a325a)
29    [#6](/bitcoin-bitcoin/6/) decltype(std::__1::forward<std::__1::__bind<void (CConnman::*)(), CConnman*>&>(fp)()) std::__1::__invoke<std::__1::__bind<void (CConnman::*)(), CConnman*>&>(std::__1::__bind<void (CConnman::*)(), CConnman*>&) /usr/lib/llvm-10/bin/../include/c++/v1/type_traits:3539:1 (bitcoind+0x1a325a)
30    [#7](/bitcoin-bitcoin/7/) void std::__1::__invoke_void_return_wrapper<void>::__call<std::__1::__bind<void (CConnman::*)(), CConnman*>&>(std::__1::__bind<void (CConnman::*)(), CConnman*>&) /usr/lib/llvm-10/bin/../include/c++/v1/__functional_base:348:9 (bitcoind+0x1a325a)
31    [#8](/bitcoin-bitcoin/8/) std::__1::__function::__alloc_func<std::__1::__bind<void (CConnman::*)(), CConnman*>, std::__1::allocator<std::__1::__bind<void (CConnman::*)(), CConnman*> >, void ()>::operator()() /usr/lib/llvm-10/bin/../include/c++/v1/functional:1540:16 (bitcoind+0x1a325a)
32    [#9](/bitcoin-bitcoin/9/) std::__1::__function::__func<std::__1::__bind<void (CConnman::*)(), CConnman*>, std::__1::allocator<std::__1::__bind<void (CConnman::*)(), CConnman*> >, void ()>::operator()() /usr/lib/llvm-10/bin/../include/c++/v1/functional:1714:12 (bitcoind+0x1a325a)
33    [#10](/bitcoin-bitcoin/10/) std::__1::__function::__value_func<void ()>::operator()() const /usr/lib/llvm-10/bin/../include/c++/v1/functional:1867:16 (bitcoind+0x148cfe)
34    [#11](/bitcoin-bitcoin/11/) std::__1::function<void ()>::operator()() const /usr/lib/llvm-10/bin/../include/c++/v1/functional:2473:12 (bitcoind+0x148cfe)
35    [#12](/bitcoin-bitcoin/12/) void TraceThread<std::__1::function<void ()> >(char const*, std::__1::function<void ()>) /home/hebasto/code/gh/bitcoin/src/./util/system.h:438:9 (bitcoind+0x148cfe)
36    [#13](/bitcoin-bitcoin/13/) decltype(std::__1::forward<void (*)(char const*, std::__1::function<void ()>)>(fp)(std::__1::forward<char const*>(fp0), std::__1::forward<std::__1::function<void ()> >(fp0))) std::__1::__invoke<void (*)(char const*, std::__1::function<void ()>), char const*, std::__1::function<void ()> >(void (*&&)(char const*, std::__1::function<void ()>), char const*&&, std::__1::function<void ()>&&) /usr/lib/llvm-10/bin/../include/c++/v1/type_traits:3539:1 (bitcoind+0x1a342c)
37    [#14](/bitcoin-bitcoin/14/) void std::__1::__thread_execute<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, void (*)(char const*, std::__1::function<void ()>), char const*, std::__1::function<void ()>, 2ul, 3ul>(std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, void (*)(char const*, std::__1::function<void ()>), char const*, std::__1::function<void ()> >&, std::__1::__tuple_indices<2ul, 3ul>) /usr/lib/llvm-10/bin/../include/c++/v1/thread:273:5 (bitcoind+0x1a342c)
38    [#15](/bitcoin-bitcoin/15/) void* std::__1::__thread_proxy<std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, void (*)(char const*, std::__1::function<void ()>), char const*, std::__1::function<void ()> > >(void*) /usr/lib/llvm-10/bin/../include/c++/v1/thread:284:5 (bitcoind+0x1a342c)
39
40  Location is heap block of size 328920 at 0x7f3445476000 allocated by main thread:
41    [#0](/bitcoin-bitcoin/0/) operator new(unsigned long) <null> (bitcoind+0x11883b)
42    [#1](/bitcoin-bitcoin/1/) std::__1::__unique_if<CConnman>::__unique_single std::__1::make_unique<CConnman, unsigned long, unsigned long, bool>(unsigned long&&, unsigned long&&, bool&&) /usr/lib/llvm-10/bin/../include/c++/v1/memory:3028:28 (bitcoind+0x134039)
43    [#2](/bitcoin-bitcoin/2/) std::__1::unique_ptr<CConnman, std::__1::default_delete<CConnman> > MakeUnique<CConnman, unsigned long, unsigned long, bool>(unsigned long&&, unsigned long&&, bool&&) /home/hebasto/code/gh/bitcoin/src/./util/memory.h:17:12 (bitcoind+0x134039)
44    [#3](/bitcoin-bitcoin/3/) AppInitMain(util::Ref const&, NodeContext&, interfaces::BlockAndHeaderTipInfo*) /home/hebasto/code/gh/bitcoin/src/init.cpp:1386:20 (bitcoind+0x134039)
45    [#4](/bitcoin-bitcoin/4/) AppInit(int, char**) /home/hebasto/code/gh/bitcoin/src/bitcoind.cpp:133:43 (bitcoind+0x11b773)
46    [#5](/bitcoin-bitcoin/5/) main /home/hebasto/code/gh/bitcoin/src/bitcoind.cpp:161:13 (bitcoind+0x11b773)
47
48  Mutex M132842 (0x7f34454c63f8) created at:
49    [#0](/bitcoin-bitcoin/0/) pthread_mutex_lock <null> (bitcoind+0xa8cf6)
50    [#1](/bitcoin-bitcoin/1/) std::__1::mutex::lock() <null> (libc++.so.1+0x83505)
51    [#2](/bitcoin-bitcoin/2/) AppInitMain(util::Ref const&, NodeContext&, interfaces::BlockAndHeaderTipInfo*) /home/hebasto/code/gh/bitcoin/src/init.cpp:1981:24 (bitcoind+0x13d0a3)
52    [#3](/bitcoin-bitcoin/3/) AppInit(int, char**) /home/hebasto/code/gh/bitcoin/src/bitcoind.cpp:133:43 (bitcoind+0x11b773)
53    [#4](/bitcoin-bitcoin/4/) main /home/hebasto/code/gh/bitcoin/src/bitcoind.cpp:161:13 (bitcoind+0x11b773)
54
55SUMMARY: ThreadSanitizer: double lock of a mutex (/home/hebasto/code/gh/bitcoin/src/bitcoind+0xa8cf6) in pthread_mutex_lock
56================== != 
57[node 0] Cleaning up leftover process
    hebasto commented at 3:19 pm on December 22, 2020:
    Going to update this PR after more tests.
    hebasto commented at 6:30 pm on December 22, 2020:
    Updated.
  38. hebasto force-pushed on Dec 22, 2020
  39. hebasto commented at 6:28 pm on December 22, 2020: member

    Updated 354d2cba44747ca34b6833e0f58901394a03ae49 -> 3e1571285f4a0edf59d51bbdeee028be3038b6dc (pr19983.11 -> pr19983.12):

  40. in test/sanitizer_suppressions/tsan:39 in 3e1571285f 
    34-deadlock:CConnman::GetNodeStats
35 deadlock:CChainState::ConnectTip
36-deadlock:UpdateTip
37-
38-# WalletBatch (unidentified deadlock)
39-deadlock:WalletBatch
    MarcoFalke commented at 6:45 pm on December 22, 2020:
    What are the steps to verify this patch? Note that the deadlocks are intermittent, so there is no way to run the tests and reproduce them or not reproduce them reliably.
    hebasto commented at 6:48 pm on December 22, 2020:
    TSan searches for potential deadlocks, i.e., it verifies consistency of lock ordering in the code, right?
    MarcoFalke commented at 7:02 pm on December 22, 2020:
    The lock order graph is generated at runtime and is not deterministic. It depends on the code paths that are executed, and our tests exercise different code paths in each run.
    hebasto commented at 7:13 pm on December 22, 2020:

    What are the steps to verify this patch?

    I don’t know (besides tracking code changes since these suppressions were introduced).

    Having a smaller set of suppressions is better, though. If this patch appears wrong in the next year, it could be reverted :)

  41. MarcoFalke commented at 8:55 pm on December 22, 2020: member

    If this patch appears wrong in the next year, it could be reverted :)

    Ok, fair enough. Let’s give it a try.

  42. MarcoFalke merged this on Dec 22, 2020
  43. MarcoFalke closed this on Dec 22, 2020
  44. sidhujag referenced this in commit d51adb2fa9 on Dec 23, 2020
  45. hebasto deleted the branch on Dec 23, 2020
  46. DrahtBot locked this on Feb 15, 2022


hebasto practicalswift DrahtBot MarcoFalke

Labels
Wallet

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-07-03 13:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me