From ab73245ab7e8d2b258d9639b9351adfe539a2f10 Mon Sep 17 00:00:00 2001 From: serge-sans-paille Date: Wed, 18 Dec 2024 14:38:40 +0100 Subject: [PATCH 1/4] [llvm] Bail out when meeting pointer with negative offset instead of generating empty location Fix the regression detected by https://github.com/llvm/llvm-test-suite/pull/188 --- llvm/lib/Analysis/MemoryBuiltins.cpp | 3 +-- .../builtin-object-size-phi.ll | 2 +- .../builtin-object-size-range.ll | 22 ++++++++++++++++++- .../objectsize_basic.ll | 4 ++-- 4 files changed, 25 insertions(+), 6 deletions(-) diff --git a/llvm/lib/Analysis/MemoryBuiltins.cpp b/llvm/lib/Analysis/MemoryBuiltins.cpp index 57b97999b0886..cc70e4a1e056e 100644 --- a/llvm/lib/Analysis/MemoryBuiltins.cpp +++ b/llvm/lib/Analysis/MemoryBuiltins.cpp @@ -841,8 +841,7 @@ OffsetSpan ObjectSizeOffsetVisitor::computeImpl(Value *V) { // This is UB, and we'd rather return an empty location then. if (Options.EvalMode == ObjectSizeOpts::Mode::Min || Options.EvalMode == ObjectSizeOpts::Mode::Max) { - ORT.Before = APInt::getZero(ORT.Before.getBitWidth()); - ORT.After = APInt::getZero(ORT.Before.getBitWidth()); + return ObjectSizeOffsetVisitor::unknown(); } // Otherwise it's fine, caller can handle negative offset. } diff --git a/llvm/test/Transforms/LowerConstantIntrinsics/builtin-object-size-phi.ll b/llvm/test/Transforms/LowerConstantIntrinsics/builtin-object-size-phi.ll index cba4da073ff2a..564311da64a81 100644 --- a/llvm/test/Transforms/LowerConstantIntrinsics/builtin-object-size-phi.ll +++ b/llvm/test/Transforms/LowerConstantIntrinsics/builtin-object-size-phi.ll @@ -143,7 +143,7 @@ define dso_local i64 @pick_max_one_oob(i1 %c0, i1 %c1) { ; CHECK-NEXT: br label [[IF_END]] ; CHECK: if.end: ; CHECK-NEXT: [[P_END:%.*]] = phi ptr [ [[P_ELSE]], [[IF_ELSE]] ], [ [[P_THEN]], [[IF_THEN]] ] -; CHECK-NEXT: [[OBJSIZE:%.*]] = select i1 [[C1:%.*]], i64 1, i64 0 +; CHECK-NEXT: [[OBJSIZE:%.*]] = select i1 [[C1:%.*]], i64 -1, i64 0 ; CHECK-NEXT: ret i64 [[OBJSIZE]] ; %p = alloca [2 x i8], align 1 diff --git a/llvm/test/Transforms/LowerConstantIntrinsics/builtin-object-size-range.ll b/llvm/test/Transforms/LowerConstantIntrinsics/builtin-object-size-range.ll index f84ebee144289..891a585724e65 100644 --- a/llvm/test/Transforms/LowerConstantIntrinsics/builtin-object-size-range.ll +++ b/llvm/test/Transforms/LowerConstantIntrinsics/builtin-object-size-range.ll @@ -78,7 +78,8 @@ define i64 @select_neg_oob_offset(i1 %c0, i1 %c1) { ; CHECK-NEXT: [[PTR:%.*]] = alloca i8, i64 10, align 1 ; CHECK-NEXT: [[OFFSET:%.*]] = select i1 [[C0:%.*]], i64 -3, i64 -4 ; CHECK-NEXT: [[PTR_SLIDE:%.*]] = getelementptr inbounds i8, ptr [[PTR]], i64 [[OFFSET]] -; CHECK-NEXT: ret i64 0 +; CHECK-NEXT: [[RES:%.*]] = select i1 [[C1:%.*]], i64 -1, i64 0 +; CHECK-NEXT: ret i64 [[RES]] ; %ptr = alloca i8, i64 10 %offset = select i1 %c0, i64 -3, i64 -4 @@ -106,4 +107,23 @@ define i64 @select_gep_offsets(i1 %cond) { ret i64 %res } +define i64 @select_gep_oob_offsets(i1 %cond) { +; CHECK-LABEL: @select_gep_oob_offsets( +; CHECK-NEXT: [[BASE1:%.*]] = alloca [288 x i8], align 16 +; CHECK-NEXT: [[SELECT0:%.*]] = select i1 [[COND:%.*]], i64 -4, i64 -64 +; CHECK-NEXT: [[SELECT1:%.*]] = select i1 [[COND]], i64 16, i64 64 +; CHECK-NEXT: [[GEP0:%.*]] = getelementptr inbounds nuw i8, ptr [[BASE1]], i64 [[SELECT1]] +; CHECK-NEXT: [[GEP1:%.*]] = getelementptr inbounds i8, ptr [[GEP0]], i64 [[SELECT0]] +; CHECK-NEXT: ret i64 -1 +; + %base1 = alloca [288 x i8], align 16 + %select0 = select i1 %cond, i64 -4, i64 -64 + %select1 = select i1 %cond, i64 16, i64 64 + %gep0 = getelementptr inbounds nuw i8, ptr %base1, i64 %select1 + %gep1 = getelementptr inbounds i8, ptr %gep0, i64 %select0 + %call = call i64 @llvm.objectsize.i64.p0(ptr %gep1, i1 false, i1 true, i1 false) + ret i64 %call +} + + attributes #0 = { nounwind allocsize(0) } diff --git a/llvm/test/Transforms/LowerConstantIntrinsics/objectsize_basic.ll b/llvm/test/Transforms/LowerConstantIntrinsics/objectsize_basic.ll index 212b4a432db3c..0eec7f75014eb 100644 --- a/llvm/test/Transforms/LowerConstantIntrinsics/objectsize_basic.ll +++ b/llvm/test/Transforms/LowerConstantIntrinsics/objectsize_basic.ll @@ -214,7 +214,7 @@ define i64 @wrapping_gep_neg(i1 %c) { ; CHECK-NEXT: [[OBJ:%.*]] = alloca i8, i64 4, align 1 ; CHECK-NEXT: [[SLIDE:%.*]] = getelementptr i8, ptr [[OBJ]], i64 9223372036854775807 ; CHECK-NEXT: [[SLIDE_BIS:%.*]] = getelementptr i8, ptr [[SLIDE]], i64 9223372036854775807 -; CHECK-NEXT: ret i64 0 +; CHECK-NEXT: ret i64 -1 ; %obj = alloca i8, i64 4 %slide = getelementptr i8, ptr %obj, i64 9223372036854775807 @@ -269,7 +269,7 @@ define i64 @out_of_bound_negative_gep(i1 %c) { ; CHECK-LABEL: @out_of_bound_negative_gep( ; CHECK-NEXT: [[OBJ:%.*]] = alloca i8, i32 4, align 1 ; CHECK-NEXT: [[SLIDE:%.*]] = getelementptr i8, ptr [[OBJ]], i8 -8 -; CHECK-NEXT: ret i64 0 +; CHECK-NEXT: ret i64 -1 ; %obj = alloca i8, i32 4 %slide = getelementptr i8, ptr %obj, i8 -8 From b228a0bb79bcd8cee99c461f3453fc5848c326d4 Mon Sep 17 00:00:00 2001 From: serge-sans-paille Date: Thu, 19 Dec 2024 00:00:20 +0100 Subject: [PATCH 2/4] fixup! [llvm] Bail out when meeting pointer with negative offset instead of generating empty location --- llvm/lib/Analysis/MemoryBuiltins.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/llvm/lib/Analysis/MemoryBuiltins.cpp b/llvm/lib/Analysis/MemoryBuiltins.cpp index cc70e4a1e056e..809d4e406bc15 100644 --- a/llvm/lib/Analysis/MemoryBuiltins.cpp +++ b/llvm/lib/Analysis/MemoryBuiltins.cpp @@ -838,7 +838,11 @@ OffsetSpan ObjectSizeOffsetVisitor::computeImpl(Value *V) { // We end up pointing on a location that's outside of the original object. if (ORT.knownBefore() && ORT.Before.isNegative()) { - // This is UB, and we'd rather return an empty location then. + // This means that we *may* be accessing memory before the allocation. It's + // unsure though, so bail out instead of returning a potentially misleading + // result. + // TODO: working with ranges instead of value would make it possible to take + // a better decision. if (Options.EvalMode == ObjectSizeOpts::Mode::Min || Options.EvalMode == ObjectSizeOpts::Mode::Max) { return ObjectSizeOffsetVisitor::unknown(); From a2ef6ffebfb507a40bef2cad281516ea1b35f29b Mon Sep 17 00:00:00 2001 From: serge-sans-paille Date: Thu, 19 Dec 2024 22:40:52 +0100 Subject: [PATCH 3/4] fixup! [llvm] Bail out when meeting pointer with negative offset instead of generating empty location --- llvm/lib/Analysis/MemoryBuiltins.cpp | 6 +++--- .../LowerConstantIntrinsics/builtin-object-size-range.ll | 6 ++++-- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/llvm/lib/Analysis/MemoryBuiltins.cpp b/llvm/lib/Analysis/MemoryBuiltins.cpp index 809d4e406bc15..6b7a3e1ffe347 100644 --- a/llvm/lib/Analysis/MemoryBuiltins.cpp +++ b/llvm/lib/Analysis/MemoryBuiltins.cpp @@ -838,9 +838,9 @@ OffsetSpan ObjectSizeOffsetVisitor::computeImpl(Value *V) { // We end up pointing on a location that's outside of the original object. if (ORT.knownBefore() && ORT.Before.isNegative()) { - // This means that we *may* be accessing memory before the allocation. It's - // unsure though, so bail out instead of returning a potentially misleading - // result. + // This means that we *may* be accessing memory before the allocation. + // Conservatively return an unknown size. + // // TODO: working with ranges instead of value would make it possible to take // a better decision. if (Options.EvalMode == ObjectSizeOpts::Mode::Min || diff --git a/llvm/test/Transforms/LowerConstantIntrinsics/builtin-object-size-range.ll b/llvm/test/Transforms/LowerConstantIntrinsics/builtin-object-size-range.ll index 891a585724e65..50aefaaccc27d 100644 --- a/llvm/test/Transforms/LowerConstantIntrinsics/builtin-object-size-range.ll +++ b/llvm/test/Transforms/LowerConstantIntrinsics/builtin-object-size-range.ll @@ -107,8 +107,8 @@ define i64 @select_gep_offsets(i1 %cond) { ret i64 %res } -define i64 @select_gep_oob_offsets(i1 %cond) { -; CHECK-LABEL: @select_gep_oob_offsets( +define i64 @select_gep_oob_overapproximated_offsets(i1 %cond) { +; CHECK-LABEL: @select_gep_oob_overapproximated_offsets( ; CHECK-NEXT: [[BASE1:%.*]] = alloca [288 x i8], align 16 ; CHECK-NEXT: [[SELECT0:%.*]] = select i1 [[COND:%.*]], i64 -4, i64 -64 ; CHECK-NEXT: [[SELECT1:%.*]] = select i1 [[COND]], i64 16, i64 64 @@ -119,6 +119,8 @@ define i64 @select_gep_oob_offsets(i1 %cond) { %base1 = alloca [288 x i8], align 16 %select0 = select i1 %cond, i64 -4, i64 -64 %select1 = select i1 %cond, i64 16, i64 64 +; This nevers actually goes oob, but because we approcimate each select +; independently, this actually ranges in [16 - 64 ; 64 - 4] instead of [64 - 64; 16 - 4] %gep0 = getelementptr inbounds nuw i8, ptr %base1, i64 %select1 %gep1 = getelementptr inbounds i8, ptr %gep0, i64 %select0 %call = call i64 @llvm.objectsize.i64.p0(ptr %gep1, i1 false, i1 true, i1 false) From 0a71d2628443872ad2e7e336be4c595984083747 Mon Sep 17 00:00:00 2001 From: serge-sans-paille Date: Fri, 20 Dec 2024 00:18:10 +0100 Subject: [PATCH 4/4] fixup! [llvm] Bail out when meeting pointer with negative offset instead of generating empty location --- .../LowerConstantIntrinsics/builtin-object-size-range.ll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/llvm/test/Transforms/LowerConstantIntrinsics/builtin-object-size-range.ll b/llvm/test/Transforms/LowerConstantIntrinsics/builtin-object-size-range.ll index 50aefaaccc27d..00af652bb7f69 100644 --- a/llvm/test/Transforms/LowerConstantIntrinsics/builtin-object-size-range.ll +++ b/llvm/test/Transforms/LowerConstantIntrinsics/builtin-object-size-range.ll @@ -119,7 +119,7 @@ define i64 @select_gep_oob_overapproximated_offsets(i1 %cond) { %base1 = alloca [288 x i8], align 16 %select0 = select i1 %cond, i64 -4, i64 -64 %select1 = select i1 %cond, i64 16, i64 64 -; This nevers actually goes oob, but because we approcimate each select +; This never actually goes oob, but because we approximate each select ; independently, this actually ranges in [16 - 64 ; 64 - 4] instead of [64 - 64; 16 - 4] %gep0 = getelementptr inbounds nuw i8, ptr %base1, i64 %select1 %gep1 = getelementptr inbounds i8, ptr %gep0, i64 %select0