[ValueTracking] Provide getUnderlyingObjectAggressive fallback

[aheejin]

This callsite assumes getUnderlyingObjectAggressive returns a non-null pointer:

llvm-project/llvm/lib/Transforms/IPO/FunctionAttrs.cpp

Line 124 in 273a94b

const Value *UO = getUnderlyingObjectAggressive(Loc.Ptr);

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:

llvm-project/llvm/lib/Analysis/ValueTracking.cpp

Lines 6866 to 6867 in 9b5857a

	if (!Visited.insert(P).second)
	continue;

llvm-project/llvm/lib/Analysis/ValueTracking.cpp

Line 6889 in 9b5857a

return Object;

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 phis 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:

llvm-project/llvm/lib/Transforms/IPO/FunctionAttrs.cpp

Lines 121 to 122 in 273a94b

	if (isNoModRef(MR))
	return;

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:

llvm-project/llvm/lib/Analysis/BasicAliasAnalysis.cpp

Line 744 in 02403f4

unsigned MaxLookup = 8;

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

llvm-project/llvm/lib/Analysis/ValueTracking.cpp

Line 6863 in 9b5857a

P = First ? FirstObject : getUnderlyingObject(P);

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

---

Fixes #117308 and fixes #122166.

[llvmbot]

@llvm/pr-subscribers-llvm-analysis

@llvm/pr-subscribers-llvm-transforms

Author: Heejin Ahn (aheejin)

Changes

This callsite assumes getUnderlyingObjectAggressive returns a non-null pointer:

llvm-project/llvm/lib/Transforms/IPO/FunctionAttrs.cpp

Line 124 in 273a94b

const Value *UO = getUnderlyingObjectAggressive(Loc.Ptr);

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:

llvm-project/llvm/lib/Analysis/ValueTracking.cpp

Lines 6866 to 6867 in 9b5857a

	if (!Visited.insert(P).second)
	continue;

llvm-project/llvm/lib/Analysis/ValueTracking.cpp

Line 6889 in 9b5857a

return Object;

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 phis 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:

llvm-project/llvm/lib/Transforms/IPO/FunctionAttrs.cpp

Lines 121 to 122 in 273a94b

	if (isNoModRef(MR))
	return;

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:

llvm-project/llvm/lib/Analysis/BasicAliasAnalysis.cpp

Line 744 in 02403f4

unsigned MaxLookup = 8;

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

llvm-project/llvm/lib/Analysis/ValueTracking.cpp

Line 6863 in 9b5857a

P = First ? FirstObject : getUnderlyingObject(P);

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

---

Fixes #117308 and fixes #122166.

---

Full diff: https://github.com/llvm/llvm-project/pull/123019.diff

2 Files Affected:

• (modified) llvm/lib/Analysis/ValueTracking.cpp (+1-1)
• (added) llvm/test/Transforms/FunctionAttrs/phi_cycle.ll (+52)

diff --git a/llvm/lib/Analysis/ValueTracking.cpp b/llvm/lib/Analysis/ValueTracking.cpp
index 4b246c013e96f1..119aba7729f85a 100644
--- a/llvm/lib/Analysis/ValueTracking.cpp
+++ b/llvm/lib/Analysis/ValueTracking.cpp
@@ -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
diff --git a/llvm/test/Transforms/FunctionAttrs/phi_cycle.ll b/llvm/test/Transforms/FunctionAttrs/phi_cycle.ll
new file mode 100644
index 00000000000000..137becd76588e1
--- /dev/null
+++ b/llvm/test/Transforms/FunctionAttrs/phi_cycle.ll
@@ -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
+}

[dtcxzyw]

LGTM. Thank you!

[nikic]

LGTM, thanks!