lebedev.ri created this revision.
lebedev.ri added reviewers: rjmccall, erichkeane, rsmith, thakis, jdoerfert.
lebedev.ri added a project: LLVM.
Herald added a subscriber: jfb.
lebedev.ri requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
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.
This has to change. D105338 <https://reviews.llvm.org/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.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D105728
Files:
clang/lib/CodeGen/CGExpr.cpp
clang/test/CodeGen/please-crash-now.c
Index: clang/test/CodeGen/please-crash-now.c
===================================================================
--- /dev/null
+++ clang/test/CodeGen/please-crash-now.c
@@ -0,0 +1,62 @@
+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py
+// RUN: %clang_cc1 -x c %s -triple x86_64-linux-gnu -emit-llvm -o -
| FileCheck %s --check-prefixes=ALL,WITHOUT_NULL
-implicit-check-not="call void @llvm.trap()"
+// RUN: %clang_cc1 -x c++ %s -triple x86_64-linux-gnu -emit-llvm -o -
| FileCheck %s --check-prefixes=ALL,WITHOUT_NULL
-implicit-check-not="call void @llvm.trap()"
+// RUN: %clang_cc1 -x c %s -triple x86_64-linux-gnu -emit-llvm -o -
-fno-delete-null-pointer-checks | FileCheck %s --check-prefixes=ALL
-implicit-check-not="call void @llvm.trap()"
+// RUN: %clang_cc1 -x c++ %s -triple x86_64-linux-gnu -emit-llvm -o -
-fno-delete-null-pointer-checks | FileCheck %s --check-prefixes=ALL
-implicit-check-not="call void @llvm.trap()"
+
+// ALL-LABEL: define {{.*}}store_into_volatile_nullptr_via_dereference
+// WITHOUT_NULL: call void @llvm.trap()
+void store_into_volatile_nullptr_via_dereference() {
+ (*((volatile int *)(0))) = 0;
+}
+
+// Not the pattern, must be a simple dereference.
+// ALL-LABEL: define {{.*}}store_into_volatile_nullptr_via_subscript_operator
+// WITHOUT_NULL: call void @llvm.trap()
+void store_into_volatile_nullptr_via_subscript_operator() {
+ ((volatile int *)(0))[0] = 0;
+}
+
+// For the rest of the tests, we don't emit trap.
+
+// Just dereferencing is not enough.
+// ALL-LABEL: define {{.*}}n0
+void n0() {
+ *((int *)(0));
+}
+
+// Just dereferencing via subscript expression is not enough either.
+// ALL-LABEL: define {{.*}}n1
+void n1() {
+ ((int *)(0))[0];
+}
+
+// Just dereferencing of a volatile pointer is still not enough.
+// ALL-LABEL: define {{.*}}n2
+void n2() {
+ *((volatile int *)(0));
+}
+
+// Just dereferencing of a volatile pointer via subscript expression is not
enough either.
+// ALL-LABEL: define {{.*}}n3
+void n3() {
+ ((volatile int *)(0))[0];
+}
+
+// Almost the pattern, but the pointer is not volatile.
+// ALL-LABEL: define {{.*}}n4
+void n4() {
+ (*((int *)(0))) = 0;
+}
+
+// Pointer subscript is not the pattern we are looking for.
+// ALL-LABEL: define {{.*}}n5
+void n5() {
+ ((int *)(0))[0] = 0;
+}
+
+// A pointer with integral value of `0` is valid in this address space.
+// ALL-LABEL: define {{.*}}n6
+void n6() {
+ (*((__attribute__((address_space(42))) volatile int *)(0))) = 0;
+}
Index: clang/lib/CodeGen/CGExpr.cpp
===================================================================
--- clang/lib/CodeGen/CGExpr.cpp
+++ clang/lib/CodeGen/CGExpr.cpp
@@ -1848,6 +1848,19 @@
Value = EmitToMemory(Value, Ty);
+ if (Volatile && !Ty->isAtomicType() &&
+ isa<llvm::ConstantPointerNull>(Addr.getPointer()) &&
+ !llvm::NullPointerIsDefined(CurFn, Addr.getAddressSpace())) {
+ // If this is a simple volatile store to a null pointer (and said null
+ // pointre is not defined), then we have a common idiom "please crash now".
+ // However, that is not the semantics LLVM IR provides, volatile operations
+ // are not defined as trapping in LLVM IR, so this store will be
+ // silently dropped by the optimization passes, and no trap will happen.
+ // Since this is a common idiom, for now, directly codegen it as trap.
+ EmitTrapCall(llvm::Intrinsic::trap);
+ return;
+ }
+
LValue AtomicLValue =
LValue::MakeAddr(Addr, Ty, getContext(), BaseInfo, TBAAInfo);
if (Ty->isAtomicType() ||
Index: clang/test/CodeGen/please-crash-now.c
===================================================================
--- /dev/null
+++ clang/test/CodeGen/please-crash-now.c
@@ -0,0 +1,62 @@
+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py
+// RUN: %clang_cc1 -x c %s -triple x86_64-linux-gnu -emit-llvm -o - | FileCheck %s --check-prefixes=ALL,WITHOUT_NULL -implicit-check-not="call void @llvm.trap()"
+// RUN: %clang_cc1 -x c++ %s -triple x86_64-linux-gnu -emit-llvm -o - | FileCheck %s --check-prefixes=ALL,WITHOUT_NULL -implicit-check-not="call void @llvm.trap()"
+// RUN: %clang_cc1 -x c %s -triple x86_64-linux-gnu -emit-llvm -o - -fno-delete-null-pointer-checks | FileCheck %s --check-prefixes=ALL -implicit-check-not="call void @llvm.trap()"
+// RUN: %clang_cc1 -x c++ %s -triple x86_64-linux-gnu -emit-llvm -o - -fno-delete-null-pointer-checks | FileCheck %s --check-prefixes=ALL -implicit-check-not="call void @llvm.trap()"
+
+// ALL-LABEL: define {{.*}}store_into_volatile_nullptr_via_dereference
+// WITHOUT_NULL: call void @llvm.trap()
+void store_into_volatile_nullptr_via_dereference() {
+ (*((volatile int *)(0))) = 0;
+}
+
+// Not the pattern, must be a simple dereference.
+// ALL-LABEL: define {{.*}}store_into_volatile_nullptr_via_subscript_operator
+// WITHOUT_NULL: call void @llvm.trap()
+void store_into_volatile_nullptr_via_subscript_operator() {
+ ((volatile int *)(0))[0] = 0;
+}
+
+// For the rest of the tests, we don't emit trap.
+
+// Just dereferencing is not enough.
+// ALL-LABEL: define {{.*}}n0
+void n0() {
+ *((int *)(0));
+}
+
+// Just dereferencing via subscript expression is not enough either.
+// ALL-LABEL: define {{.*}}n1
+void n1() {
+ ((int *)(0))[0];
+}
+
+// Just dereferencing of a volatile pointer is still not enough.
+// ALL-LABEL: define {{.*}}n2
+void n2() {
+ *((volatile int *)(0));
+}
+
+// Just dereferencing of a volatile pointer via subscript expression is not enough either.
+// ALL-LABEL: define {{.*}}n3
+void n3() {
+ ((volatile int *)(0))[0];
+}
+
+// Almost the pattern, but the pointer is not volatile.
+// ALL-LABEL: define {{.*}}n4
+void n4() {
+ (*((int *)(0))) = 0;
+}
+
+// Pointer subscript is not the pattern we are looking for.
+// ALL-LABEL: define {{.*}}n5
+void n5() {
+ ((int *)(0))[0] = 0;
+}
+
+// A pointer with integral value of `0` is valid in this address space.
+// ALL-LABEL: define {{.*}}n6
+void n6() {
+ (*((__attribute__((address_space(42))) volatile int *)(0))) = 0;
+}
Index: clang/lib/CodeGen/CGExpr.cpp
===================================================================
--- clang/lib/CodeGen/CGExpr.cpp
+++ clang/lib/CodeGen/CGExpr.cpp
@@ -1848,6 +1848,19 @@
Value = EmitToMemory(Value, Ty);
+ if (Volatile && !Ty->isAtomicType() &&
+ isa<llvm::ConstantPointerNull>(Addr.getPointer()) &&
+ !llvm::NullPointerIsDefined(CurFn, Addr.getAddressSpace())) {
+ // If this is a simple volatile store to a null pointer (and said null
+ // pointre is not defined), then we have a common idiom "please crash now".
+ // However, that is not the semantics LLVM IR provides, volatile operations
+ // are not defined as trapping in LLVM IR, so this store will be
+ // silently dropped by the optimization passes, and no trap will happen.
+ // Since this is a common idiom, for now, directly codegen it as trap.
+ EmitTrapCall(llvm::Intrinsic::trap);
+ return;
+ }
+
LValue AtomicLValue =
LValue::MakeAddr(Addr, Ty, getContext(), BaseInfo, TBAAInfo);
if (Ty->isAtomicType() ||
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
