[ValueTracking] Remove faulty dereference of "InsertBefore" #85034
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
In 2fe81ed [NFC][RemoveDIs] Insert instruction using iterators in Transforms/ we changed if (*req_idx != *i) return FindInsertedValue(I->getAggregateOperand(), idx_range, - InsertBefore); + *InsertBefore); } but there is no guarantee that is InsertBefore is non-empty at that point, which we e.g can see in the added testcase. Instead just pass on the optional InsertBefore in the recursive call to FindInsertedValue, as we do at several other places already.
|
@llvm/pr-subscribers-llvm-analysis Author: None (mikaelholmen) ChangesIn 2fe81ed
Instead just pass on the optional InsertBefore in the recursive call to FindInsertedValue, as we do at several other places already. Full diff: https://github.com/llvm/llvm-project/pull/85034.diff 2 Files Affected:
diff --git a/llvm/lib/Analysis/ValueTracking.cpp b/llvm/lib/Analysis/ValueTracking.cpp
index 371ad41ee96562..8a4a2c4f92a0dc 100644
--- a/llvm/lib/Analysis/ValueTracking.cpp
+++ b/llvm/lib/Analysis/ValueTracking.cpp
@@ -5709,7 +5709,7 @@ llvm::FindInsertedValue(Value *V, ArrayRef<unsigned> idx_range,
// looking for, then.
if (*req_idx != *i)
return FindInsertedValue(I->getAggregateOperand(), idx_range,
- *InsertBefore);
+ InsertBefore);
}
// If we end up here, the indices of the insertvalue match with those
// requested (though possibly only partially). Now we recursively look at
diff --git a/llvm/test/Analysis/Lint/crash_empty_iterator.ll b/llvm/test/Analysis/Lint/crash_empty_iterator.ll
new file mode 100644
index 00000000000000..2fbecbcef5cfb4
--- /dev/null
+++ b/llvm/test/Analysis/Lint/crash_empty_iterator.ll
@@ -0,0 +1,22 @@
+; RUN: opt -passes="lint" -S < %s | FileCheck %s
+
+; After 2fe81edef6f0b
+; [NFC][RemoveDIs] Insert instruction using iterators in Transforms/
+; this crashed in FindInsertedValue when dereferencing an empty
+; optional iterator.
+; Just see that it doesn't crash anymore.
+
+; CHECK-LABEL: @test1
+
+%struct = type { i32, i32 }
+
+define void @test1() {
+entry:
+ %.fca.1.insert = insertvalue %struct zeroinitializer, i32 0, 1
+ %0 = extractvalue %struct %.fca.1.insert, 0
+ %1 = tail call %struct @foo(i32 %0)
+ ret void
+}
+
+declare %struct @foo(i32)
+
|
|
This follows a problem I noticed here: |
mikaelholmen
referenced
this pull request
in mahtohappy/llvm-project
Mar 13, 2024
As part of the RemoveDIs project we need LLVM to insert instructions using iterators wherever possible, so that the iterators can carry a bit of debug-info. This commit implements some of that by updating the contents of llvm/lib/Transforms/Utils to always use iterator-versions of instruction constructors. There are two general flavours of update: * Almost all call-sites just call getIterator on an instruction * Several make use of an existing iterator (scenarios where the code is actually significant for debug-info) The underlying logic is that any call to getFirstInsertionPt or similar APIs that identify the start of a block need to have that iterator passed directly to the insertion function, without being converted to a bare Instruction pointer along the way. Noteworthy changes: * FindInsertedValue now takes an optional iterator rather than an instruction pointer, as we need to always insert with iterators, * I've added a few iterator-taking versions of some value-tracking and DomTree methods -- they just unwrap the iterator. These are purely convenience methods to avoid extra syntax in some passes. * A few calls to getNextNode become std::next instead (to keep in the theme of using iterators for positions), * SeparateConstOffsetFromGEP has it's insertion-position field changed. Noteworthy because it's not a purely localised spelling change. All this should be NFC.
|
Thanks for taking care of this! |
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.
In 2fe81ed
[NFC][RemoveDIs] Insert instruction using iterators in Transforms/
we changed
but there is no guarantee that is InsertBefore is non-empty at that point,
which we e.g can see in the added testcase.
Instead just pass on the optional InsertBefore in the recursive call to FindInsertedValue, as we do at several other places already.