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

[lebedev.ri]

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.

[erichkeane]

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]

In D105728#2868281, @erichkeane wrote:

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]

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

[lebedev.ri]

@rsmith ping

[lebedev.ri]

@rsmith @efriedma ping

[efriedma]

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.

clang/lib/CodeGen/CGExpr.cpp

void CodeGenFunction::EmitStoreOfScalar(llvm::Value *Value, Address Addr,
   }
 
   Value = EmitToMemory(Value, Ty);
 
   LValue AtomicLValue =
       LValue::MakeAddr(Addr, Ty, getContext(), BaseInfo, TBAAInfo);
   if (Ty->isAtomicType() ||
       (!isInit && LValueIsSuitableForInlineAtomic(AtomicLValue))) {
     EmitAtomicStore(RValue::get(Value), AtomicLValue, isInit);
     return;
   }
 
+  if (Volatile && isa<llvm::ConstantPointerNull>(Addr.getPointer()) &&
+      !llvm::NullPointerIsDefined(CurFn, Addr.getAddressSpace())) {
+    // If this is a simple volatile store to a null pointer
+    // (and said null pointer is not defined for this address space),
+    // 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;
+  }
+
   llvm::StoreInst *Store = Builder.CreateStore(Value, Addr, Volatile);
   if (isNontemporal) {
     llvm::MDNode *Node =
         llvm::MDNode::get(Store->getContext(),
                           llvm::ConstantAsMetadata::get(Builder.getInt32(1)));
     Store->setMetadata(CGM.getModule().getMDKindID("nontemporal"), Node);
   }
 

clang/test/CodeGen/please-crash-now.c

+// 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;
+}