Add ThreadSanitizer #4046
Add ThreadSanitizer #4046
Conversation
lrstewart
force-pushed
the
tsan
branch
2 times, most recently
from
7579524
to
fc6a581
Compare
There was a problem hiding this comment.
Successful Codebuild run: https://us-west-2.console.aws.amazon.com/codesuite/codebuild/024603541914/projects/s2nGeneralBatch/batch/s2nGeneralBatch%3A2b3f572b-6ea8-48e4-9c0a-666f0281fbd0/details?region=us-west-2
Codebuild run correctly detecting problems (I removed the suppression): https://us-west-2.console.aws.amazon.com/codesuite/codebuild/024603541914/projects/s2nGeneralBatch/batch/s2nGeneralBatch%3A8a260694-3eeb-4461-8d80-635bd0052197?region=us-west-2
| set_property( | ||
| TEST | ||
| ${test_case_name} | ||
| PROPERTY | ||
| ENVIRONMENT LD_PRELOAD=$<TARGET_FILE:allocator_overrides>) | ||
|
|
||
| set_property( | ||
| TEST | ||
| ${test_case_name} | ||
| PROPERTY | ||
| ENVIRONMENT S2N_DONT_MLOCK=1) | ||
| set_property(TEST ${test_case_name} PROPERTY ENVIRONMENT ${UNIT_TEST_ENVS}) |
There was a problem hiding this comment.
Setting ENVIRONMENT like this overrides instead of appends. So the first call to set LD_PRELOAD has no affect on how the tests are ultimately run, but the call to S2N_DONT_MLOCK does. CMake 3.22 added ENVIRONMENT_MODIFICATION to support modifying instead of completely overriding, but our CI uses some very old versions of CMake and we shouldn't assume our customers are using the latest either.
I fixed this problem by constructing the full set of environment variables as a string so that I could set ENVIRONMENT once.
| post_build: | ||
| on-failure: ABORT | ||
| commands: | ||
| - CTEST_OUTPUT_ON_FAILURE=1 CTEST_PARALLEL_LEVEL=$(nproc) make -C build test |
There was a problem hiding this comment.
nit: Would it make sense to add this to the environment variables that you set in the CMakeLists.txt? That way all of our CI would get it for free. It would make it hard to suppress failed test output, but that doesn't seem like something that would ever be necessary.
There was a problem hiding this comment.
Oh that's a good idea! Then we don't have to remember it everywhere. I'll do that in a separate PR.
Description of changes:
Adds ThreadSanitizer support to detect currency issues. We can enable it in the CI by adding:
To s2nGeneralBatch's buildspec.
Testing:
I'll post links to CodeBuild jobs as comments.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.