llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang Author: Adam Magier (AdamMagierFOSS) <details> <summary>Changes</summary> 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 --- Full diff: https://github.com/llvm/llvm-project/pull/225139.diff 2 Files Affected: - (modified) clang/lib/CodeGen/CGExprScalar.cpp (+11-25) - (modified) clang/test/CodeGen/ubsan-pointer-overflow-constant-fold.c (+34) ``````````diff diff --git a/clang/lib/CodeGen/CGExprScalar.cpp b/clang/lib/CodeGen/CGExprScalar.cpp index eaeb2c0d820bc4..2a88466ad564ad 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 36d48e1f86c8e8..424c487f772f3d 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]; +} `````````` </details> https://github.com/llvm/llvm-project/pull/225139 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
