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

[Roman Lebedev via Phabricator via cfe-commits]

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() ||


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D105728.357607.patch
Type: text/x-patch
Size: 3642 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20210709/1c5ae67e/attachment-0001.bin>