From b9baaec5ad175c05ee9789586d5d7a2e0b63ccc7 Mon Sep 17 00:00:00 2001 From: Heejin Ahn Date: Wed, 15 Jan 2025 02:07:33 +0000 Subject: [PATCH] [ValueTracking] Provide getUnderlyingObjectAggressive fallback 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. --- llvm/lib/Analysis/ValueTracking.cpp | 2 +- .../Transforms/FunctionAttrs/phi_cycle.ll | 52 +++++++++++++++++++ 2 files changed, 53 insertions(+), 1 deletion(-) create mode 100644 llvm/test/Transforms/FunctionAttrs/phi_cycle.ll diff --git a/llvm/lib/Analysis/ValueTracking.cpp b/llvm/lib/Analysis/ValueTracking.cpp index 4b246c013e96f..119aba7729f85 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 0000000000000..137becd76588e --- /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 +}