https://github.com/AdamMagierFOSS created https://github.com/llvm/llvm-project/pull/225139
Follow up to #223446. EmitGEPOffsetInBytes recreated the offset by walking the GEP value that EmitCheckedInBoundsGEP had just created. That required a separate path for the case where CreateGEP folded the result, because a folded GEP need not be a GEP at all: gep(null, 1) becomes inttoptr(1), and gep(@g, 0) becomes @g. Pass ElemTy and IdxList instead and walk those directly. This removes the constant path, the cast to GEPOperator, and two asserts that restated the caller's own behaviour. This is not quite NFC. The constant path reported OffsetOverflows as false unconditionally, so a constant GEP whose byte offset wrapped to exactly zero satisfied the TotalOffset == Zero early return and emitted no check. The unified path computes the flag, so such a GEP now emits one. It is provably valid, since TotalOffset == 0 makes the computed address equal the base, so this is extra IR at -O0 rather than a change in behaviour. Add constant-base cases to ubsan-pointer-overflow-constant-fold.c, including the wraps-to-zero case: it passes before this change only because no check is emitted at all. Assisted-by: Kiro CLI / Claude Opus 5 >From 2e9c58e48f73d9b2c3e8d48ca6d3c5924681dd5d Mon Sep 17 00:00:00 2001 From: Adam Magier <[email protected]> Date: Fri, 18 Sep 2026 22:45:09 +0200 Subject: [PATCH] [clang][CodeGen] Compute the pointer-overflow offset from the index list Follow up to #223446. EmitGEPOffsetInBytes recreated the offset by walking the GEP value that EmitCheckedInBoundsGEP had just created. That required a separate path for the case where CreateGEP folded the result, because a folded GEP need not be a GEP at all: gep(null, 1) becomes inttoptr(1), and gep(@g, 0) becomes @g. Pass ElemTy and IdxList instead and walk those directly. This removes the constant path, the cast to GEPOperator, and two asserts that restated the caller's own behaviour. This is not quite NFC. The constant path reported OffsetOverflows as false unconditionally, so a constant GEP whose byte offset wrapped to exactly zero satisfied the TotalOffset == Zero early return and emitted no check. The unified path computes the flag, so such a GEP now emits one. It is provably valid, since TotalOffset == 0 makes the computed address equal the base, so this is extra IR at -O0 rather than a change in behaviour. Add constant-base cases to ubsan-pointer-overflow-constant-fold.c, including the wraps-to-zero case: it passes before this change only because no check is emitted at all. Assisted-by: Kiro CLI / Claude Opus 5 --- clang/lib/CodeGen/CGExprScalar.cpp | 36 ++++++------------- .../ubsan-pointer-overflow-constant-fold.c | 34 ++++++++++++++++++ 2 files changed, 45 insertions(+), 25 deletions(-) diff --git a/clang/lib/CodeGen/CGExprScalar.cpp b/clang/lib/CodeGen/CGExprScalar.cpp index eaeb2c0d820bc..2a88466ad564a 100644 --- a/clang/lib/CodeGen/CGExprScalar.cpp +++ b/clang/lib/CodeGen/CGExprScalar.cpp @@ -6361,35 +6361,20 @@ struct GEPOffsetAndOverflow { llvm::Value *OffsetOverflows; }; -/// Evaluate given GEPVal, which is either an inbounds GEP, or a constant, -/// and compute the total offset it applies from it's base pointer BasePtr. +/// Compute the total offset in bytes that indexing BasePtr with ElemTy and +/// IdxList applies, using checked arithmetic. /// Returns offset in bytes and a boolean flag whether an overflow happened /// during evaluation. -static GEPOffsetAndOverflow EmitGEPOffsetInBytes(Value *BasePtr, Value *GEPVal, - llvm::LLVMContext &VMContext, - CodeGenModule &CGM, - CGBuilderTy &Builder) { +static GEPOffsetAndOverflow +EmitGEPOffsetInBytes(Value *BasePtr, llvm::Type *ElemTy, + ArrayRef<Value *> IdxList, llvm::LLVMContext &VMContext, + CodeGenModule &CGM, CGBuilderTy &Builder) { const auto &DL = CGM.getDataLayout(); // The total (signed) byte offset for the GEP. llvm::Value *TotalOffset = nullptr; - // Was the GEP already reduced to a constant? - if (isa<llvm::Constant>(GEPVal)) { - // Compute the offset by casting both pointers to integers and subtracting: - // GEPVal = BasePtr + ptr(Offset) <--> Offset = int(GEPVal) - int(BasePtr) - Value *BasePtr_int = Builder.CreatePtrToAddr(BasePtr); - Value *GEPVal_int = Builder.CreatePtrToAddr(GEPVal); - TotalOffset = Builder.CreateSub(GEPVal_int, BasePtr_int); - return {TotalOffset, /*OffsetOverflows=*/Builder.getFalse()}; - } - - auto *GEP = cast<llvm::GEPOperator>(GEPVal); - assert(GEP->getPointerOperand() == BasePtr && - "BasePtr must be the base of the GEP."); - assert(GEP->isInBounds() && "Expected inbounds GEP"); - - auto *IntPtrTy = DL.getAddressType(GEP->getPointerOperandType()); + auto *IntPtrTy = DL.getAddressType(BasePtr->getType()); // Grab references to the signed add/mul overflow intrinsics for intptr_t. auto *Zero = llvm::ConstantInt::getNullValue(IntPtrTy); @@ -6427,7 +6412,8 @@ static GEPOffsetAndOverflow EmitGEPOffsetInBytes(Value *BasePtr, Value *GEPVal, }; // Determine the total byte offset by looking at each GEP operand. - for (auto GTI = llvm::gep_type_begin(GEP), GTE = llvm::gep_type_end(GEP); + for (auto GTI = llvm::gep_type_begin(ElemTy, IdxList), + GTE = llvm::gep_type_end(ElemTy, IdxList); GTI != GTE; ++GTI) { llvm::Value *LocalOffset; auto *Index = GTI.getOperand(); @@ -6493,8 +6479,8 @@ CodeGenFunction::EmitCheckedInBoundsGEP(llvm::Type *ElemTy, Value *Ptr, SanitizerDebugLocation SanScope(this, {CheckOrdinal}, CheckHandler); llvm::Type *IntPtrTy = DL.getAddressType(PtrTy); - GEPOffsetAndOverflow EvaluatedGEP = - EmitGEPOffsetInBytes(Ptr, GEPVal, getLLVMContext(), CGM, Builder); + GEPOffsetAndOverflow EvaluatedGEP = EmitGEPOffsetInBytes( + Ptr, ElemTy, IdxList, getLLVMContext(), CGM, Builder); auto *Zero = llvm::ConstantInt::getNullValue(IntPtrTy); diff --git a/clang/test/CodeGen/ubsan-pointer-overflow-constant-fold.c b/clang/test/CodeGen/ubsan-pointer-overflow-constant-fold.c index 36d48e1f86c8e..424c487f772f3 100644 --- a/clang/test/CodeGen/ubsan-pointer-overflow-constant-fold.c +++ b/clang/test/CodeGen/ubsan-pointer-overflow-constant-fold.c @@ -32,3 +32,37 @@ void test_zero_offset_no_overflow(void) { // Genuine zero offset should not emit a check. readings[0][0]; } + +// The cases above index through a pointer loaded from a global, so the GEP has +// a runtime base. The cases below index a global array directly, so the GEP +// itself folds to a constant expression. + +struct sensor_reading direct[2]; + +// CHECK-LABEL: define {{.*}}@test_constant_gep_offset +// CHECK: icmp ne i16 add (i16 ptrtoaddr (ptr @direct to i16), i16 8), 0, !nosanitize +// CHECK: call void @__ubsan_handle_pointer_overflow +void test_constant_gep_offset(void) { + // A plain index: 1 * 8 = 8, no overflow. + direct[1]; +} + +// CHECK-LABEL: define {{.*}}@test_constant_gep_offset_overflow +// CHECK: icmp ne i16 add (i16 ptrtoaddr (ptr @direct to i16), i16 -32768), 0, !nosanitize +// CHECK: call void @__ubsan_handle_pointer_overflow +void test_constant_gep_offset_overflow(void) { + // 4096 * 8 = 32768 overflows i16 and wraps to -32768. + direct[4096]; +} + +// CHECK-LABEL: define {{.*}}@test_constant_gep_wraps_to_zero +// CHECK: br i1 true, label %[[CONT:[^,]+]], label %[[HANDLER:[^,]+]], {{.*}}!nosanitize +// CHECK: [[HANDLER]]: +// CHECK: call void @__ubsan_handle_pointer_overflow +void test_constant_gep_wraps_to_zero(void) { + // 8192 * 8 = 65536 wraps to 0 in i16. The offset is zero but computing it + // overflowed, so the zero-offset early return does not apply and a check is + // emitted. The check is trivially valid, since a zero offset leaves the + // computed address equal to the base. + direct[8192]; +} _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
