Skip to content

Commit

Permalink
[ValueTracking] Provide getUnderlyingObjectAggressive fallback (#123019)
Browse files Browse the repository at this point in the history
This callsite assumes `getUnderlyingObjectAggressive` returns a non-null
pointer:

https://github.com/llvm/llvm-project/blob/273a94b3d5a78cd9122c7b3bbb5d5a87147735d2/llvm/lib/Transforms/IPO/FunctionAttrs.cpp#L124

But it can return null when there are cycles in the value chain so there
is no more `Worklist` item anymore to explore, in which case it just
returns `Object` at the end of the function without ever setting it:
https://github.com/llvm/llvm-project/blob/9b5857a68381652dbea2a0c9efa734b6c4cf38c9/llvm/lib/Analysis/ValueTracking.cpp#L6866-L6867
https://github.com/llvm/llvm-project/blob/9b5857a68381652dbea2a0c9efa734b6c4cf38c9/llvm/lib/Analysis/ValueTracking.cpp#L6889

`getUnderlyingObject` does not seem to return null either judging by
looking at its code and its callsites, so I think it is not likely to be
the author's intention that `getUnderlyingObjectAggressive` returns
null.

So this checks whether `Object` is null at the end, and if so, falls
back to the original first value.

---

The test case here was reduced by bugpoint and further reduced manually,
but I find it hard to reduce it further.

To trigger this bug, the memory operation should not be reachable from
the entry BB, because the `phi`s should form a cycle without introducing
another value from the entry. I tried a minimal `phi` cycle with three
BBs (entry BB + two BBs in a cycle), but it was skipped here:
https://github.com/llvm/llvm-project/blob/273a94b3d5a78cd9122c7b3bbb5d5a87147735d2/llvm/lib/Transforms/IPO/FunctionAttrs.cpp#L121-L122
To get the result that's not `ModRefInfo::NoModRef`, the length of `phi`
chain needed to be greater than the `MaxLookup` value set in this
function:

https://github.com/llvm/llvm-project/blob/02403f4e450b86d93197dd34045ff40a34b21494/llvm/lib/Analysis/BasicAliasAnalysis.cpp#L744

But just lengthening the `phi` chain to 8 didn't trigger the same error
in `getUnderlyingObjectAggressive` because `getUnderlyingObject` here
passes through a single-chain `phi`s so not all `phi`s end up in
`Visited`:

https://github.com/llvm/llvm-project/blob/9b5857a68381652dbea2a0c9efa734b6c4cf38c9/llvm/lib/Analysis/ValueTracking.cpp#L6863

So I just submit here the smallest test case I managed to create.

---

Fixes #117308 and fixes #122166.
  • Loading branch information
aheejin authored Jan 15, 2025
1 parent 6655c53 commit 5a90168
Show file tree
Hide file tree
Showing 2 changed files with 53 additions and 1 deletion.
2 changes: 1 addition & 1 deletion llvm/lib/Analysis/ValueTracking.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6886,7 +6886,7 @@ const Value *llvm::getUnderlyingObjectAggressive(const Value *V) {
return FirstObject;
} while (!Worklist.empty());

return Object;
return Object ? Object : FirstObject;
}

/// This is the function that does the work of looking through basic
Expand Down
52 changes: 52 additions & 0 deletions llvm/test/Transforms/FunctionAttrs/phi_cycle.ll
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
; RUN: opt -passes=function-attrs -S < %s

; Regression test for a null-returning bug of getUnderlyingObjectAggressive().
; This should not crash.
define void @phi_cycle() {
bb:
unreachable

bb1: ; preds = %bb17
br label %bb2

bb2: ; preds = %bb5, %bb1
%phi = phi ptr [ %phi6, %bb1 ], [ %phi6, %bb5 ]
br i1 poison, label %bb4, label %bb3

bb3: ; preds = %bb2
%getelementptr = getelementptr inbounds i8, ptr %phi, i32 poison
br label %bb5

bb4: ; preds = %bb2
br label %bb7

bb5: ; preds = %bb15, %bb3
%phi6 = phi ptr [ %getelementptr, %bb3 ], [ %phi16, %bb15 ]
br i1 poison, label %bb17, label %bb2

bb7: ; preds = %bb15, %bb4
%phi8 = phi ptr [ %phi, %bb4 ], [ %phi16, %bb15 ]
br i1 poison, label %bb11, label %bb9

bb9: ; preds = %bb7
%getelementptr10 = getelementptr inbounds i8, ptr %phi8, i32 1
store i8 poison, ptr %phi8, align 1
br label %bb15

bb11: ; preds = %bb7
br i1 poison, label %bb13, label %bb12

bb12: ; preds = %bb11
br label %bb13

bb13: ; preds = %bb12, %bb11
%getelementptr14 = getelementptr inbounds i8, ptr %phi8, i32 poison
br label %bb15

bb15: ; preds = %bb13, %bb9
%phi16 = phi ptr [ %getelementptr14, %bb13 ], [ %getelementptr10, %bb9 ]
br i1 poison, label %bb5, label %bb7

bb17: ; preds = %bb5
br label %bb1
}

0 comments on commit 5a90168

Please sign in to comment.