[llvm] Bail out when meeting pointer with negative offset instead of … #120424
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…generating empty location Fix the regression detected by llvm/llvm-test-suite#188
llvmbot
added
llvm:analysis
|
@llvm/pr-subscribers-llvm-analysis Author: None (serge-sans-paille) Changes…generating empty location Fix the regression detected by llvm/llvm-test-suite#188 Full diff: https://github.com/llvm/llvm-project/pull/120424.diff 4 Files Affected:
diff --git a/llvm/lib/Analysis/MemoryBuiltins.cpp b/llvm/lib/Analysis/MemoryBuiltins.cpp
index 57b97999b08860..cc70e4a1e056e1 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 cba4da073ff2aa..564311da64a81f 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 f84ebee1442893..891a585724e655 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 212b4a432db3c4..0eec7f75014eb3 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
|
|
@llvm/pr-subscribers-llvm-transforms Author: None (serge-sans-paille) Changes…generating empty location Fix the regression detected by llvm/llvm-test-suite#188 Full diff: https://github.com/llvm/llvm-project/pull/120424.diff 4 Files Affected:
diff --git a/llvm/lib/Analysis/MemoryBuiltins.cpp b/llvm/lib/Analysis/MemoryBuiltins.cpp
index 57b97999b08860..cc70e4a1e056e1 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 cba4da073ff2aa..564311da64a81f 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 f84ebee1442893..891a585724e655 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 212b4a432db3c4..0eec7f75014eb3 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
|
fhahn
reviewed
Dec 18, 2024
| @@ -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(); | |||
There was a problem hiding this comment.
Is the comment above incorrect? IIUC the case @mstorsjo shared doesn't count UB
There was a problem hiding this comment.
Indeed; regularly with UB, when looking at various potential execution paths, the compiler can assume that the ones that are UB just won't happen at runtime. (Not sure how that translates best to this feature though, which is intended to protect against things at runtime that really are unintended.)
There was a problem hiding this comment.
Not at all, i'll update it. The idea would be 'if we are uncertain about the accuracy and the validity of the access, better be safe and bail out rather than return a potentially invalid result.
…ead of generating empty location
mstorsjo
approved these changes
Dec 19, 2024
There was a problem hiding this comment.
LGTM, this does indeed fix my issue.
I can't say I'm confident in how all of this analysis fits together with everything else that it may be used for, but indicating that we're unsure rather than confidently saying that the size is zero, sounds correct to me.
fhahn
reviewed
Dec 19, 2024
llvm/lib/Analysis/MemoryBuiltins.cpp
Outdated
Comment on lines
841
to
843
| // 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. |
There was a problem hiding this comment.
Suggested change
| // 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. |
fhahn
reviewed
Dec 19, 2024
| @@ -106,4 +107,23 @@ define i64 @select_gep_offsets(i1 %cond) { | |||
| ret i64 %res | |||
| } | |||
|
|
|||
| define i64 @select_gep_oob_offsets(i1 %cond) { | |||
There was a problem hiding this comment.
Is this actually guaranteed to be OOB? If not,p lease update the test and ideally add a brief comment explaining what this is testing
…ead of generating empty location
mstorsjo
reviewed
Dec 19, 2024
| @@ -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 | |||
…ead of generating empty location
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
…generating empty location
Fix the regression detected by llvm/llvm-test-suite#188