Have the front-end use the `nounwind` attribute on atomic libcalls. This prevents us from seeing `invoke __atomic_load` in MSAN, which is problematic as it has no successor for instrumentation to be added.
Details
- Reviewers
eugenis vitalybuka
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp | ||
---|---|---|
3518 | TODO: Remove |
__libatomic_load might come at the end of the function, with no succeeding BB
Not exactly. It may come at the end of a BB.
We should probably fix it in clang by adding nounwind to libatomic calls, and ignore invokes here.
llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp | ||
---|---|---|
3508 | Check if the function already exists. | |
3510 | addVisibility(GlobalValue::HiddenVisibility); |
Simplified by returning to the old implementation, but having libatomic calls made nounwind (so we never see them as invokes).
clang/lib/CodeGen/CGAtomic.cpp | ||
---|---|---|
315 | This needs a clang test, and better move it to a separate change. | |
llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp | ||
3509 | Better explicitly check that this an invoke and not call. Call always has a next instruction. And move this check to the parent function to apply regular call instrumentation to the unrecognized call. | |
3511 | It has a successor. Two successors, actually. |
llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp | ||
---|---|---|
3513 | TODO: Remove this redudant call |
llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp | ||
---|---|---|
3513 | you've already checked it with assert(isa<CallInst>) - which could be changed to isTerminator(), and this could be simplified to NextIRB(CB.getNextNode()). |
LGTM with 1 comment
llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp | ||
---|---|---|
3576 | Probably need the same check for atomic_store. |
llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp | ||
---|---|---|
3576 | Although it would be good for consistency I don't think it's strictly needed since with atomic_store we don't need to use getNextNode() |
llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp | ||
---|---|---|
3576 | ah, good point. LGTM |
This needs a clang test, and better move it to a separate change.