[Clang][CodeGen] Check isUnderlyingBasePointerConstantNull in emitPointerArithmetic

[dtcxzyw]

Do not set inbounds on GEP when the pointer operand is a constant null.

Note: This patch is fragile :( For a more complex case (int*)0 + offset1 + offset2, we still set inbounds for the second addition.

Related issue: #137833

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-codegen

Author: Yingwei Zheng (dtcxzyw)

Changes

Do not set inbounds on GEP when the pointer operand is a constant null.

Note: This patch is fragile :( For a more complex case (int*)0 + offset1 + offset2, we still set inbounds for the second addition.

Related issue: #137833

---

Full diff: https://github.com/llvm/llvm-project/pull/137849.diff

2 Files Affected:

• (modified) clang/lib/CodeGen/CGExprScalar.cpp (+2-1)
• (modified) clang/test/CodeGen/catch-nullptr-and-nonzero-offset.c (+12)

diff --git a/clang/lib/CodeGen/CGExprScalar.cpp b/clang/lib/CodeGen/CGExprScalar.cpp
index 8dbbcdaef25d8..d214d2af52563 100644
--- a/clang/lib/CodeGen/CGExprScalar.cpp
+++ b/clang/lib/CodeGen/CGExprScalar.cpp
@@ -4238,7 +4238,8 @@ static Value *emitPointerArithmetic(CodeGenFunction &CGF,
   else
     elemTy = CGF.ConvertTypeForMem(elementType);
 
-  if (CGF.getLangOpts().PointerOverflowDefined)
+  if (CGF.getLangOpts().PointerOverflowDefined ||
+      CGF.isUnderlyingBasePointerConstantNull(pointerOperand))
     return CGF.Builder.CreateGEP(elemTy, pointer, index, "add.ptr");
 
   return CGF.EmitCheckedInBoundsGEP(
diff --git a/clang/test/CodeGen/catch-nullptr-and-nonzero-offset.c b/clang/test/CodeGen/catch-nullptr-and-nonzero-offset.c
index 63b6db2c2adeb..c5ae3f8bcc368 100644
--- a/clang/test/CodeGen/catch-nullptr-and-nonzero-offset.c
+++ b/clang/test/CodeGen/catch-nullptr-and-nonzero-offset.c
@@ -431,6 +431,18 @@ char *void_ptr(void *base, unsigned long offset) {
   return base + offset;
 }
 
+int *constant_null_add(long offset) {
+  // CHECK: define{{.*}} ptr @constant_null_add(i64 noundef %[[OFFSET:.*]])
+  // CHECK-NEXT: [[ENTRY:.*]]:
+  // CHECK-NEXT:   %[[OFFSET_ADDR:.*]] = alloca i64, align 8
+  // CHECK-NEXT:   store i64 %[[OFFSET]], ptr %[[OFFSET_ADDR]], align 8
+  // CHECK-NEXT:   %[[OFFSET_RELOADED:.*]] = load i64, ptr %[[OFFSET_ADDR]], align 8
+  // CHECK-NEXT:   %[[ADD_PTR:.*]] = getelementptr i32, ptr null, i64 %[[OFFSET_RELOADED]]
+  // CHECK-NEXT:   ret ptr %[[ADD_PTR]]
+#line 1800
+  return (int *)0 + offset;
+}
+
 #ifdef __cplusplus
 }
 #endif

[nikic]

While we should omit inbounds, I think it would be good to not suppress the sanitizer. If the sanitizer doesn't warn about it, then things will never get better :(

[efriedma-quic]

What's the expected interaction between isUnderlyingBasePointerConstantNull and BinaryOperator::isNullPointerArithmeticExtension ? They aren't quite the same thing, but they seem pretty closely related.

[dtcxzyw]

BinaryOperator::isNullPointerArithmeticExtension only allows the pointer operand to be a constant null. But isUnderlyingBasePointerConstantNull also catches ((struct S*)0)->field.
I think it is safe to remove this workaround and fall through to the isUnderlyingBasePointerConstantNull check.

[nikic]

An important difference is that isNullPointerArithmeticExtension() uses inttoptr, which means we get back a pointer with provenance, while this case only drops the inbounds, but still keeps the gep null, resulting in a pointer with nullary provenance, so any loads/stores on it are UB.

(Using inttoptr is of course only possible if we know for sure that the base pointer is null, it doesn't work for the "we have a select where the result might be null" case.)

[dtcxzyw]

While we should omit inbounds, I think it would be good to not suppress the sanitizer. If the sanitizer doesn't warn about it, then things will never get better :(

Fixed.