[NFC-ish] Finish MSAN handling (Refs. #6513) #6516
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
Somehow, initially i missed that there was MSan support, so it might be good to actually mention that we don't need to run any MSan passes here, and that we didn't forget to run them. Secondly, it seems inconsistent not annotate the functions with `Attribute::SanitizeMemory`, like we do for others. I suppose it isn't strictly required, since they are used to actually drive the instrumentation passes, and we don't run MSan pass, but they are also used to disable some LLVM optimizations, and that //might// be important. Or not, but then i suppose there should be a comment about it?
LebedevRI
changed the title
alexreinking
approved these changes
Dec 28, 2021
|
Failures are again unrelated... same arm64/llvm14 failure. |
alexreinking
added
the
code_cleanup
Requesting a review from @steven-johnson (whenever he's back) to address this. IIRC he implemented MSAN support and I'm not sure why this wouldn't be annotated. |
steven-johnson
approved these changes
Jan 3, 2022
|
Thanks for the fix! |
|
@alexreinking @steven-johnson Thank you! |
alexreinking
pushed a commit
that referenced
this pull request
Jan 5, 2022
Somehow, initially i missed that there was MSan support, so it might be good to actually mention that we don't need to run any MSan passes here, and that we didn't forget to run them. Secondly, it seems inconsistent not annotate the functions with `Attribute::SanitizeMemory`, like we do for others. I suppose it isn't strictly required, since they are used to actually drive the instrumentation passes, and we don't run MSan pass, but they are also used to disable some LLVM optimizations, and that //might// be important. Or not, but then i suppose there should be a comment about it? Co-authored-by: Steven Johnson <srj@google.com> (cherry picked from commit 7eb9949)
alexreinking
pushed a commit
that referenced
this pull request
Jan 6, 2022
Somehow, initially i missed that there was MSan support, so it might be good to actually mention that we don't need to run any MSan passes here, and that we didn't forget to run them. Secondly, it seems inconsistent not annotate the functions with `Attribute::SanitizeMemory`, like we do for others. I suppose it isn't strictly required, since they are used to actually drive the instrumentation passes, and we don't run MSan pass, but they are also used to disable some LLVM optimizations, and that //might// be important. Or not, but then i suppose there should be a comment about it? Co-authored-by: Steven Johnson <srj@google.com> (cherry picked from commit 7eb9949)
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.
Somehow, initially i missed that there was MSan support,
so it might be good to actually mention that we don't need to run
any MSan passes here, and that we didn't forget to run them.
Secondly, it seems inconsistent not annotate the functions
with
Attribute::SanitizeMemory, like we do for others.I suppose it isn't strictly required, since they are used
to actually drive the instrumentation passes, and we don't
run MSan pass, but they are also used to disable some LLVM optimizations,
and that //might// be important. Or not, but then i suppose
there should be a comment about it?
Let me know if i should replace the second change with a comment too.