Page MenuHomePhabricator

[clang][Codegen] Directly lower `(*((volatile int *)(0))) = 0;` into a `call void @llvm.trap()`
AcceptedPublic

Authored by lebedev.ri on Jul 9 2021, 1:10 PM.

Details

Summary

We have a problem.

There is a common C/C++ idiom, (*((volatile int *)(0))) = 0;,
which some users write expecting that it will trap.
Currently clang naively lowers it into a volatile store.

But in LLVM, a volatile store is non-trapping:
https://reviews.llvm.org/D53184 ([LangRef] Clarify semantics of volatile operations.),
and that does not seem to be going to change.

So LLVM optimization passes are allowed to erase such stores (because storing to null is undefined),
even though they currently don't do that, because it will break the C world.
The latter has to change. D105338 tried to, but it caused the sky to fall :)

Here, we teach clang to emit the pattern that the user should have written in the first place,
so that the workarounds can be dropped from LLVM side of things.

This fires when we have a volatile store to a null pointer literal,
and said null pointer is not a valid pointer.

Diff Detail

Unit TestsFailed

TimeTest
7,610 msx64 debian > SanitizerCommon-asan-x86_64-Linux.Linux::signal_line.cpp
Script: -- : 'RUN: at line 5'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -gline-tables-only -fsanitize=address -m64 -funwind-tables -ldl /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/sanitizer_common/TestCases/Linux/signal_line.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/sanitizer_common/asan-x86_64-Linux/Linux/Output/signal_line.cpp.tmp -O0
1,190 msx64 debian > SanitizerCommon-lsan-x86_64-Linux.Linux::signal_line.cpp
Script: -- : 'RUN: at line 5'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -gline-tables-only -fsanitize=leak -m64 -funwind-tables -ldl /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/sanitizer_common/TestCases/Linux/signal_line.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/sanitizer_common/lsan-x86_64-Linux/Linux/Output/signal_line.cpp.tmp -O0
2,510 msx64 debian > SanitizerCommon-msan-x86_64-Linux.Linux::signal_line.cpp
Script: -- : 'RUN: at line 5'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -gline-tables-only -fsanitize=memory -m64 -funwind-tables -ldl /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/sanitizer_common/TestCases/Linux/signal_line.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/sanitizer_common/msan-x86_64-Linux/Linux/Output/signal_line.cpp.tmp -O0
1,860 msx64 debian > SanitizerCommon-tsan-x86_64-Linux.Linux::signal_line.cpp
Script: -- : 'RUN: at line 5'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -gline-tables-only -fsanitize=thread -m64 -funwind-tables -ldl /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/sanitizer_common/TestCases/Linux/signal_line.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/sanitizer_common/tsan-x86_64-Linux/Linux/Output/signal_line.cpp.tmp -O0
930 msx64 debian > SanitizerCommon-ubsan-x86_64-Linux.Linux::signal_line.cpp
Script: -- : 'RUN: at line 5'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -gline-tables-only -fsanitize=undefined -m64 -funwind-tables -ldl /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/sanitizer_common/TestCases/Linux/signal_line.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/sanitizer_common/ubsan-x86_64-Linux/Linux/Output/signal_line.cpp.tmp -O0

Event Timeline

lebedev.ri created this revision.Jul 9 2021, 1:10 PM
lebedev.ri requested review of this revision.Jul 9 2021, 1:10 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 9 2021, 1:10 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

This seems logical to me. The comment needs some love, so I made my best suggestion.

clang/lib/CodeGen/CGExpr.cpp
1855
1856
lebedev.ri updated this revision to Diff 357648.Jul 9 2021, 2:54 PM
lebedev.ri marked 2 inline comments as done.
lebedev.ri edited the summary of this revision. (Show Details)

This seems logical to me. The comment needs some love, so I made my best suggestion.

Thank you for taking a look!
Adjusted comments.

erichkeane accepted this revision.Jul 9 2021, 2:55 PM

Give this a bit for other reviewers (it is afterall friday afternoon in the US:)), but this seems like an improvement to me.

This revision is now accepted and ready to land.Jul 9 2021, 2:55 PM

gcc apparently lowers this sort of construct to a volatile load/store followed by a trap. Not sure where the trap is coming from.

I'm not sure this solves any problem that really needs to be solved. LLVM isn't going to optimize out a volatile load/store to null. Well, unless you have a volatile load followed by __builtin_unreachable() or equivalent, but that applies to any volatile load. I'd rather not make assumptions about what the user is trying to do, or change the behavior of the user's code if they use -fno-delete-null-pointer-checks, or change the behavior of user code depending on how they write nullptr.

On a related note, it might be worth considering turning on TrapUnreachable/NoTrapAfterNoreturn by default in the backend; it's probably cheap in terms of codesize, and it might reduce user confusion around unreachable branches.