[CodeGen] Respect pointer-overflow sanitizer for void pointers

[nikic]

Pointer arithmetic on void pointers (a GNU extension) was going through a different code path and bypassed the pointer-overflow sanitizer.

Fixes #66451.

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-codegen

Changes

Pointer arithmetic on void pointers (a GNU extension) was going through a different code path and bypassed the pointer-overflow sanitizer.

Fixes #66451.

---

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

4 Files Affected:

• (modified) clang/lib/CodeGen/CGExprScalar.cpp (+4-2)
• (modified) clang/test/CodeGen/PowerPC/ppc64-inline-asm.c (+1-1)
• (modified) clang/test/CodeGen/address-space.c (+1-1)
• (modified) clang/test/CodeGen/catch-nullptr-and-nonzero-offset.c (+43)

diff --git a/clang/lib/CodeGen/CGExprScalar.cpp b/clang/lib/CodeGen/CGExprScalar.cpp
index d76ce15b41ac570..93ab064bdf3915d 100644
--- a/clang/lib/CodeGen/CGExprScalar.cpp
+++ b/clang/lib/CodeGen/CGExprScalar.cpp
@@ -3723,10 +3723,12 @@ static Value *emitPointerArithmetic(CodeGenFunction &CGF,
   // Explicitly handle GNU void* and function pointer arithmetic extensions. The
   // GNU void* casts amount to no-ops since our void* type is i8*, but this is
   // future proof.
+  llvm::Type *elemTy;
   if (elementType->isVoidType() || elementType->isFunctionType())
-    return CGF.Builder.CreateGEP(CGF.Int8Ty, pointer, index, "add.ptr");
+    elemTy = CGF.Int8Ty;
+  else
+    elemTy = CGF.ConvertTypeForMem(elementType);
 
-  llvm::Type *elemTy = CGF.ConvertTypeForMem(elementType);
   if (CGF.getLangOpts().isSignedOverflowDefined())
     return CGF.Builder.CreateGEP(elemTy, pointer, index, "add.ptr");
 
diff --git a/clang/test/CodeGen/PowerPC/ppc64-inline-asm.c b/clang/test/CodeGen/PowerPC/ppc64-inline-asm.c
index e3f2c2ff5507ce5..005bf5c7fa14407 100644
--- a/clang/test/CodeGen/PowerPC/ppc64-inline-asm.c
+++ b/clang/test/CodeGen/PowerPC/ppc64-inline-asm.c
@@ -47,6 +47,6 @@ void testZ(void *addr) {
 void testZwOff(void *addr, long long off) {
   asm volatile ("dcbz %y0\n" :: "Z"(*(unsigned char *)(addr + off)) : "memory");
 // CHECK-LABEL: void @testZwOff(ptr noundef %addr, i64 noundef %off)
-// CHECK: %[[VAL:[^ ]+]] = getelementptr i8, ptr %addr, i64 %off
+// CHECK: %[[VAL:[^ ]+]] = getelementptr inbounds i8, ptr %addr, i64 %off
 // CHECK: call void asm sideeffect "dcbz ${0:y}\0A", "*Z,~{memory}"(ptr elementtype(i8) %[[VAL]])
 }
diff --git a/clang/test/CodeGen/address-space.c b/clang/test/CodeGen/address-space.c
index 83a434d88b8e8b0..c92fc0dd9687068 100644
--- a/clang/test/CodeGen/address-space.c
+++ b/clang/test/CodeGen/address-space.c
@@ -53,7 +53,7 @@ void test4(MyStruct __attribute__((address_space(2))) *pPtr) {
 // X86: [[ALLOCA:%.*]] = alloca ptr addrspace(1)
 // X86-NEXT: store ptr addrspace(1) %arg, ptr [[ALLOCA]]
 // X86-NEXT: load ptr addrspace(1), ptr [[ALLOCA]]
-// X86-NEXT: getelementptr i8, ptr addrspace(1)
+// X86-NEXT: getelementptr inbounds i8, ptr addrspace(1)
 // X86-NEXT: ret ptr addrspace(1)
 void __attribute__((address_space(1)))*
 void_ptr_arithmetic_test(void __attribute__((address_space(1))) *arg) {
diff --git a/clang/test/CodeGen/catch-nullptr-and-nonzero-offset.c b/clang/test/CodeGen/catch-nullptr-and-nonzero-offset.c
index 0e0a9b157464a64..015102940890a52 100644
--- a/clang/test/CodeGen/catch-nullptr-and-nonzero-offset.c
+++ b/clang/test/CodeGen/catch-nullptr-and-nonzero-offset.c
@@ -35,6 +35,7 @@
 // CHECK-SANITIZE-ANYRECOVER-C-DAG: @[[LINE_1400:.*]] = {{.*}}, i32 1400, i32 15 } }
 // CHECK-SANITIZE-ANYRECOVER-DAG: @[[LINE_1500:.*]] = {{.*}}, i32 1500, i32 15 } }
 // CHECK-SANITIZE-ANYRECOVER-DAG: @[[LINE_1600:.*]] = {{.*}}, i32 1600, i32 15 } }
+// CHECK-SANITIZE-ANYRECOVER-DAG: @[[LINE_1700:.*]] = {{.*}}, i32 1700, i32 15 } }
 
 #ifdef __cplusplus
 extern "C" {
@@ -427,6 +428,48 @@ char *allones_allones_OK(void) {
   return base + offset;
 }
 
+// C++ does not allow void* arithmetic even as a GNU extension. Replace void*
+// with char* in that case to keep test expectations the same.
+#ifdef __cplusplus
+char *void_ptr(char *base, unsigned long offset) {
+#else
+char *void_ptr(void *base, unsigned long offset) {
+#endif
+  // CHECK: define{{.*}} ptr @void_ptr(ptr noundef %[[BASE:.*]], i64 noundef %[[OFFSET:.*]])
+  // CHECK-NEXT:                      [[ENTRY:.*]]:
+  // CHECK-NEXT:                        %[[BASE_ADDR:.*]] = alloca ptr, align 8
+  // CHECK-NEXT:                        %[[OFFSET_ADDR:.*]] = alloca i64, align 8
+  // CHECK-NEXT:                        store ptr %[[BASE]], ptr %[[BASE_ADDR]], align 8
+  // CHECK-NEXT:                        store i64 %[[OFFSET]], ptr %[[OFFSET_ADDR]], align 8
+  // CHECK-NEXT:                        %[[BASE_RELOADED:.*]] = load ptr, ptr %[[BASE_ADDR]], align 8
+  // CHECK-NEXT:                        %[[OFFSET_RELOADED:.*]] = load i64, ptr %[[OFFSET_ADDR]], align 8
+  // CHECK-NEXT:                        %[[ADD_PTR:.*]] = getelementptr inbounds i8, ptr %[[BASE_RELOADED]], i64 %[[OFFSET_RELOADED]]
+  // CHECK-SANITIZE-NEXT:               %[[COMPUTED_OFFSET_AGGREGATE:.*]] = call { i64, i1 } @llvm.smul.with.overflow.i64(i64 1, i64 %[[OFFSET_RELOADED]]), !nosanitize
+  // CHECK-SANITIZE-NEXT:               %[[COMPUTED_OFFSET_OVERFLOWED:.*]] = extractvalue { i64, i1 } %[[COMPUTED_OFFSET_AGGREGATE]], 1, !nosanitize
+  // CHECK-SANITIZE-NEXT:               %[[OR_OV:.+]] = or i1 %[[COMPUTED_OFFSET_OVERFLOWED]], false, !nosanitize
+  // CHECK-SANITIZE-NEXT:               %[[COMPUTED_OFFSET:.*]] = extractvalue { i64, i1 } %[[COMPUTED_OFFSET_AGGREGATE]], 0, !nosanitize
+  // CHECK-SANITIZE-NEXT:               %[[BASE_RELOADED_INT:.*]] = ptrtoint ptr %[[BASE_RELOADED]] to i64, !nosanitize
+  // CHECK-SANITIZE-NEXT:               %[[COMPUTED_GEP:.*]] = add i64 %[[BASE_RELOADED_INT]], %[[COMPUTED_OFFSET]], !nosanitize
+  // CHECK-SANITIZE-NEXT:               %[[BASE_IS_NOT_NULLPTR:.*]] = icmp ne ptr %[[BASE_RELOADED]], null, !nosanitize
+  // CHECK-SANITIZE-NEXT:               %[[COMPUTED_GEP_IS_NOT_NULL:.*]] = icmp ne i64 %[[COMPUTED_GEP]], 0, !nosanitize
+  // CHECK-SANITIZE-C-NEXT:             %[[BOTH_POINTERS_ARE_NULL_OR_BOTH_ARE_NONNULL:.*]] = and i1 %[[BASE_IS_NOT_NULLPTR]], %[[COMPUTED_GEP_IS_NOT_NULL]], !nosanitize
+  // CHECK-SANITIZE-CPP-NEXT:           %[[BOTH_POINTERS_ARE_NULL_OR_BOTH_ARE_NONNULL:.*]] = icmp eq i1 %[[BASE_IS_NOT_NULLPTR]], %[[COMPUTED_GEP_IS_NOT_NULL]], !nosanitize
+  // CHECK-SANITIZE-NEXT:               %[[COMPUTED_OFFSET_DID_NOT_OVERFLOW:.*]] = xor i1 %[[OR_OV]], true, !nosanitize
+  // CHECK-SANITIZE-NEXT:               %[[COMPUTED_GEP_IS_UGE_BASE:.*]] = icmp uge i64 %[[COMPUTED_GEP]], %[[BASE_RELOADED_INT]], !nosanitize
+  // CHECK-SANITIZE-NEXT:               %[[GEP_DID_NOT_OVERFLOW:.*]] = and i1 %[[COMPUTED_GEP_IS_UGE_BASE]], %[[COMPUTED_OFFSET_DID_NOT_OVERFLOW]], !nosanitize
+  // CHECK-SANITIZE-NEXT:               %[[GEP_IS_OKAY:.*]] = and i1 %[[BOTH_POINTERS_ARE_NULL_OR_BOTH_ARE_NONNULL]], %[[GEP_DID_NOT_OVERFLOW]], !nosanitize
+  // CHECK-SANITIZE-NEXT:               br i1 %[[GEP_IS_OKAY]], label %[[CONT:.*]], label %[[HANDLER_POINTER_OVERFLOW:[^,]+]],{{.*}} !nosanitize
+  // CHECK-SANITIZE:                  [[HANDLER_POINTER_OVERFLOW]]:
+  // CHECK-SANITIZE-NORECOVER-NEXT:     call void @__ubsan_handle_pointer_overflow_abort(ptr @[[LINE_1700]], i64 %[[BASE_RELOADED_INT]], i64 %[[COMPUTED_GEP]])
+  // CHECK-SANITIZE-RECOVER-NEXT:       call void @__ubsan_handle_pointer_overflow(ptr @[[LINE_1700]], i64 %[[BASE_RELOADED_INT]], i64 %[[COMPUTED_GEP]])
+  // CHECK-SANITIZE-TRAP-NEXT:          call void @llvm.ubsantrap(i8 19){{.*}}, !nosanitize
+  // CHECK-SANITIZE-UNREACHABLE-NEXT:   unreachable, !nosanitize
+  // CHECK-SANITIZE:                  [[CONT]]:
+  // CHECK-NEXT:                        ret ptr %[[ADD_PTR]]
+#line 1700
+  return base + offset;
+}
+
 #ifdef __cplusplus
 }
 #endif

[JustinStitt]

LGTM!

FWIW, here's a test file that does not trip the sanitizer before this PR and does trip after the PR:

#include <stdio.h>

int main(int argc, char **argv) {
  void *p = NULL;
  printf("p: %p\n", p);
  p = p - argc;
  printf("p: %p\n", p);
  return 0;
}

[kees]

This is great! Thanks for fixing this!