Make ASAN actually do something and fix all the things #46336
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Keno
force-pushed
the
kf/reviveasan
branch
2 times, most recently
from
8a04e68
to
1acecb7
Compare
Keno
added a commit
that referenced
this pull request
Aug 15, 2022
Analogous to (and dependent on) #46336, but for msan. The biggest change is a workaround for LLVM's lack for TLS relocation support (I had a fix for that about 7 years ago at https://reviews.llvm.org/D8815, but that was x86_64-linux-gnu only, while this workaround works everywhere - though we should consider resurrecting my patch for performance at some point). The rest is mostly build fixes and plumbing to get the sanitizer flags through to the dependencies.
This was referenced Aug 15, 2022
We've had the ability to build with ASAN for quite a while, but at some point the LLVM pass stopped doing anything unless you also annotated the function with the `sanitize_address` attribute, so if you built Julia with asan enabled, very few things were actually built with asan and those that were didn't have one of asan's most crucial features enabled: Tracking out of bounds stack accesses. This PR enabled asan-stack handling, adds the appropriate asan poison handling to our tasks and fixes a plethora of other things that didn't work with asan enabled. The result of this PR (together with a few other PRs still pending) should be that asan passes tests on master. This was a significant amount of work, and the changes required quite subtle. As a result, I think we should make sure to quickly set up CI to test this configuration and make sure it doesn't regress.
Keno
added a commit
that referenced
this pull request
Aug 17, 2022
Analogous to (and dependent on) #46336, but for msan. The biggest change is a workaround for LLVM's lack for TLS relocation support (I had a fix for that about 7 years ago at https://reviews.llvm.org/D8815, but that was x86_64-linux-gnu only, while this workaround works everywhere - though we should consider resurrecting my patch for performance at some point). The rest is mostly build fixes and plumbing to get the sanitizer flags through to the dependencies.
Keno
added a commit
that referenced
this pull request
Aug 18, 2022
Analogous to (and dependent on) #46336, but for msan. The biggest change is a workaround for LLVM's lack for TLS relocation support (I had a fix for that about 7 years ago at https://reviews.llvm.org/D8815, but that was x86_64-linux-gnu only, while this workaround works everywhere - though we should consider resurrecting my patch for performance at some point). The rest is mostly build fixes and plumbing to get the sanitizer flags through to the dependencies.
Keno
added a commit
that referenced
this pull request
Aug 19, 2022
Analogous to (and dependent on) #46336, but for msan. The biggest change is a workaround for LLVM's lack for TLS relocation support (I had a fix for that about 7 years ago at https://reviews.llvm.org/D8815, but that was x86_64-linux-gnu only, while this workaround works everywhere - though we should consider resurrecting my patch for performance at some point). The rest is mostly build fixes and plumbing to get the sanitizer flags through to the dependencies.
vtjnash
reviewed
Sep 2, 2022
| @@ -76,17 +76,16 @@ void jl_init_stack_limits(int ismaster, void **stack_lo, void **stack_hi) | |||
| #if defined(_COMPILER_GCC_) && __GNUC__ >= 12 | |||
| #pragma GCC diagnostic ignored "-Wdangling-pointer" | |||
| #endif | |||
| *stack_hi = (void*)&stacksize; | |||
There was a problem hiding this comment.
why are we using &stacksize rather than the value of it as stackaddr + stacksize
There was a problem hiding this comment.
I don't know, but the former is slightly smaller of course. I guess it may prevent us from smashing any extra information the system may have put at the stack base.
There was a problem hiding this comment.
we should never smash this; we just use it to decide if it hit a StackOverflow
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We've had the ability to build with ASAN for quite a while, but
at some point the LLVM pass stopped doing anything unless you
also annotated the function with the
sanitize_addressattribute,so if you built Julia with asan enabled, very few things were
actually built with asan and those that were didn't have one
of asan's most crucial features enabled: Tracking out of bounds
stack accesses.
This PR enabled asan-stack handling, adds the appropriate asan
poison handling to our tasks and fixes a plethora of other things
that didn't work with asan enabled. The result of this PR (together
with a few other PRs still pending) should be that asan passes
tests on master.
This was a significant amount of work, and the changes required
quite subtle. As a result, I think we should make sure to quickly
set up CI to test this configuration and make sure it doesn't
regress.