New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove some ThreadSanitizer warnings #4609

Merged
merged 15 commits into from Oct 20, 2017

Conversation

Projects
None yet
5 participants
@pirapira
Member

pirapira commented Oct 18, 2017

This should be the easier part of #4579. Adding mutex or atomic, avoiding deadlocks, or stopping threads before vptr table is destroyed.

@pirapira pirapira requested a review from gumb0 Oct 18, 2017

@pirapira

This comment has been minimized.

Show comment
Hide comment
@pirapira

pirapira Oct 18, 2017

Member

I don't know why circleCI->macOS-XCode9 says failed. It's still compiling the codebase.

Member

pirapira commented Oct 18, 2017

I don't know why circleCI->macOS-XCode9 says failed. It's still compiling the codebase.

Show outdated Hide outdated libp2p/Host.h
@@ -132,7 +132,6 @@ class EthereumHost: public p2p::HostCapability<EthereumPeer>, Worker
bool m_newTransactions = false;
bool m_newBlocks = false;
mutable RecursiveMutex x_sync;

This comment has been minimized.

@gumb0

gumb0 Oct 19, 2017

Member

I have some doubts about removing this mutex. Some operations that were atomic now are not atomic...

At least let's add RecursiveGuard l(x_sync); inside BlockChainSync::competeSync() and BlockChainSync::abortSync(), otherwise they now stay unprotected

@gumb0

gumb0 Oct 19, 2017

Member

I have some doubts about removing this mutex. Some operations that were atomic now are not atomic...

At least let's add RecursiveGuard l(x_sync); inside BlockChainSync::competeSync() and BlockChainSync::abortSync(), otherwise they now stay unprotected

This comment has been minimized.

@pirapira

pirapira Oct 19, 2017

Member

Yes, I think I should do the "at least" plan.

@pirapira

pirapira Oct 19, 2017

Member

Yes, I think I should do the "at least" plan.

This comment has been minimized.

@pirapira

pirapira Oct 19, 2017

Member

Oh, those RecursiveGuards are already added on develop.

@pirapira

pirapira Oct 19, 2017

Member

Oh, those RecursiveGuards are already added on develop.

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Oct 19, 2017

Codecov Report

Merging #4609 into develop will increase coverage by 0.44%.
The diff coverage is 60.6%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4609      +/-   ##
===========================================
+ Coverage    53.38%   53.83%   +0.44%     
===========================================
  Files         1608     1607       -1     
  Lines        69275    70061     +786     
  Branches      7093     7229     +136     
===========================================
+ Hits         36983    37717     +734     
+ Misses       31075    31062      -13     
- Partials      1217     1282      +65
Impacted Files Coverage Δ
libethereum/Client.cpp 29.43% <ø> (+0.06%) ⬆️
libethashseal/EthashClient.h 0% <ø> (ø) ⬆️
libethereum/BlockChainSync.h 18.18% <ø> (ø) ⬆️
libethereum/ClientTest.h 0% <ø> (ø) ⬆️
libp2p/Common.h 62.5% <ø> (+0.59%) ⬆️
libdevcore/Worker.h 75% <ø> (ø) ⬆️
libp2p/Host.h 75% <ø> (+5%) ⬆️
libweb3jsonrpc/IpcServerBase.h 0% <ø> (ø) ⬆️
libethereum/EthereumHost.h 0% <ø> (ø) ⬆️
libethashseal/EthashClient.cpp 1.66% <0%> (-0.09%) ⬇️
... and 269 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5f642b8...d4259d9. Read the comment docs.

codecov-io commented Oct 19, 2017

Codecov Report

Merging #4609 into develop will increase coverage by 0.44%.
The diff coverage is 60.6%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4609      +/-   ##
===========================================
+ Coverage    53.38%   53.83%   +0.44%     
===========================================
  Files         1608     1607       -1     
  Lines        69275    70061     +786     
  Branches      7093     7229     +136     
===========================================
+ Hits         36983    37717     +734     
+ Misses       31075    31062      -13     
- Partials      1217     1282      +65
Impacted Files Coverage Δ
libethereum/Client.cpp 29.43% <ø> (+0.06%) ⬆️
libethashseal/EthashClient.h 0% <ø> (ø) ⬆️
libethereum/BlockChainSync.h 18.18% <ø> (ø) ⬆️
libethereum/ClientTest.h 0% <ø> (ø) ⬆️
libp2p/Common.h 62.5% <ø> (+0.59%) ⬆️
libdevcore/Worker.h 75% <ø> (ø) ⬆️
libp2p/Host.h 75% <ø> (+5%) ⬆️
libweb3jsonrpc/IpcServerBase.h 0% <ø> (ø) ⬆️
libethereum/EthereumHost.h 0% <ø> (ø) ⬆️
libethashseal/EthashClient.cpp 1.66% <0%> (-0.09%) ⬇️
... and 269 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5f642b8...d4259d9. Read the comment docs.

@pirapira

This comment has been minimized.

Show comment
Hide comment
@pirapira

pirapira Oct 19, 2017

Member

Ready to merge?

Member

pirapira commented Oct 19, 2017

Ready to merge?

@gumb0

gumb0 approved these changes Oct 19, 2017

Yes, looks good 👍

@pirapira

This comment has been minimized.

Show comment
Hide comment
@pirapira

pirapira Oct 20, 2017

Member

Rebased to fix the Travis macos build.

Member

pirapira commented Oct 20, 2017

Rebased to fix the Travis macos build.

@pirapira

This comment has been minimized.

Show comment
Hide comment
@pirapira

pirapira Oct 20, 2017

Member

Now CircleCI's macos fails.

Member

pirapira commented Oct 20, 2017

Now CircleCI's macos fails.

@pirapira

This comment has been minimized.

Show comment
Hide comment
@pirapira

pirapira Oct 20, 2017

Member
  1/398 Test   #1: RandomCodeTests/rndCode .....***Exception: SegFault  1.81 sec
Member

pirapira commented Oct 20, 2017

  1/398 Test   #1: RandomCodeTests/rndCode .....***Exception: SegFault  1.81 sec
@pirapira

This comment has been minimized.

Show comment
Hide comment
@pirapira

pirapira Oct 20, 2017

Member

I started

$ while true; do ./testeth -t 'RandomCodeTests/rndCode' &>> rndCode.log; done

locally.

Member

pirapira commented Oct 20, 2017

I started

$ while true; do ./testeth -t 'RandomCodeTests/rndCode' &>> rndCode.log; done

locally.

@pirapira

This comment has been minimized.

Show comment
Hide comment
@pirapira

pirapira Oct 20, 2017

Member

@winsvega does 'testeth -t RandomCodeTests/rndCode' generate a different test every time it is executed?

Member

pirapira commented Oct 20, 2017

@winsvega does 'testeth -t RandomCodeTests/rndCode' generate a different test every time it is executed?

@pirapira

This comment has been minimized.

Show comment
Hide comment
@pirapira

pirapira Oct 20, 2017

Member

I cannot reproduce the problem. I need to try on my mac when I can reach it.

Member

pirapira commented Oct 20, 2017

I cannot reproduce the problem. I need to try on my mac when I can reach it.

@chfast

This comment has been minimized.

Show comment
Hide comment
@chfast

chfast Oct 20, 2017

Collaborator

I noticed the same failure in other branch.

Collaborator

chfast commented Oct 20, 2017

I noticed the same failure in other branch.

@winsvega

This comment has been minimized.

Show comment
Hide comment
@winsvega

winsvega Oct 20, 2017

Contributor

the test is different every time thats right. so it potentially could have generated a test which failed on mac.
the test case is just a unit test for this function. because this function should produce state test output.

Contributor

winsvega commented Oct 20, 2017

the test is different every time thats right. so it potentially could have generated a test which failed on mac.
the test case is just a unit test for this function. because this function should produce state test output.

@chfast

This comment has been minimized.

Show comment
Hide comment
@chfast

chfast Oct 20, 2017

Collaborator

No. This is failing all the time.

Collaborator

chfast commented Oct 20, 2017

No. This is failing all the time.

@pirapira

This comment has been minimized.

Show comment
Hide comment
@pirapira

pirapira Oct 20, 2017

Member

I cannot reproduce this issue with

Apple LLVM version 9.0.0 (clang-900.0.38)

On CircleCI, I see

Apple LLVM version 9.0.0 (clang-900.0.37)

Maybe I should have a look at Apple LLVM's release notes.

Member

pirapira commented Oct 20, 2017

I cannot reproduce this issue with

Apple LLVM version 9.0.0 (clang-900.0.38)

On CircleCI, I see

Apple LLVM version 9.0.0 (clang-900.0.37)

Maybe I should have a look at Apple LLVM's release notes.

@chfast

This comment has been minimized.

Show comment
Hide comment
@chfast

chfast Oct 20, 2017

Collaborator

On Circle CI there is an option to "Rebuild with SSH". It will add your GitHub public key to the container and you will be able to login there with SSH.
Maybe after the build you will be able to inspect the crash output or use gdb/lldb.

Collaborator

chfast commented Oct 20, 2017

On Circle CI there is an option to "Rebuild with SSH". It will add your GitHub public key to the container and you will be able to login there with SSH.
Maybe after the build you will be able to inspect the crash output or use gdb/lldb.

@pirapira pirapira merged commit 92877e7 into develop Oct 20, 2017

9 checks passed

ci/circleci: Linux-Clang5 Your tests passed on CircleCI!
Details
ci/circleci: Linux-GCC6-Debug Your tests passed on CircleCI!
Details
ci/circleci: macOS-XCode9 Your tests passed on CircleCI!
Details
codecov/patch 60.6% of diff hit (target 50%)
Details
codecov/project 53.83% (+0.44%) compared to 5f642b8
Details
codecov/project/app 49.7% (+0.56%) compared to 5f642b8
Details
codecov/project/tests 82.86% (+<.01%) compared to 5f642b8
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@pirapira pirapira deleted the tsan-easier branch Oct 20, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment