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
Time | Test | |
---|---|---|
1,110 ms | linux > MemorySanitizer-X86_64.MemorySanitizer-X86_64::libatomic_load_exceptions.cpp Script:
--
: 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang --driver-mode=g++ -fsanitize=memory -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer -fno-optimize-sibling-calls -m64 -gline-tables-only -fexceptions -fsanitize-memory-track-origins=2 -latomic -O0 /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/msan/libatomic_load_exceptions.cpp -o /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/msan/X86_64/Output/libatomic_load_exceptions.cpp.tmp && not /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/msan/X86_64/Output/libatomic_load_exceptions.cpp.tmp 2>&1 | FileCheck /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/msan/libatomic_load_exceptions.cpp --check-prefix=CHECK --check-prefix=CHECK-SHADOW
| |
410 ms | linux > MemorySanitizer-lld-X86_64.MemorySanitizer-lld-X86_64::libatomic_load_exceptions.cpp Script:
--
: 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang --driver-mode=g++ -fsanitize=memory -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer -fno-optimize-sibling-calls -m64 -fuse-ld=lld -gline-tables-only -fexceptions -fsanitize-memory-track-origins=2 -latomic -O0 /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/msan/libatomic_load_exceptions.cpp -o /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/msan/lld-X86_64/Output/libatomic_load_exceptions.cpp.tmp && not /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/msan/lld-X86_64/Output/libatomic_load_exceptions.cpp.tmp 2>&1 | FileCheck /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/msan/libatomic_load_exceptions.cpp --check-prefix=CHECK --check-prefix=CHECK-SHADOW
| |
110 ms | windows > Clang.CodeGen::debug-info-unused-types.c Script:
--
: 'RUN: at line 1'; c:\ws\w16-1\llvm-project\premerge-checks\build\bin\clang.exe -fno-eliminate-unused-debug-types -g -emit-llvm -S -o - C:\ws\w16-1\llvm-project\premerge-checks\clang\test\CodeGen\debug-info-unused-types.c | c:\ws\w16-1\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w16-1\llvm-project\premerge-checks\clang\test\CodeGen\debug-info-unused-types.c
|
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 |