Reland "[Support] Assert that DomTree nodes share parent"" #102782
Merged
Conversation
This file contains 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
|
@llvm/pr-subscribers-llvm-transforms @llvm/pr-subscribers-llvm-support Author: Vitaly Buka (vitalybuka) ChangesReverts llvm/llvm-project#102780 Full diff: https://github.com/llvm/llvm-project/pull/102782.diff 5 Files Affected:
diff --git a/llvm/include/llvm/Support/GenericDomTree.h b/llvm/include/llvm/Support/GenericDomTree.h
index 7e2b68e6faea29..45ef38b965b752 100644
--- a/llvm/include/llvm/Support/GenericDomTree.h
+++ b/llvm/include/llvm/Support/GenericDomTree.h
@@ -397,6 +397,8 @@ class DominatorTreeBase {
/// may (but is not required to) be null for a forward (backwards)
/// statically unreachable block.
DomTreeNodeBase<NodeT> *getNode(const NodeT *BB) const {
+ assert((!BB || Parent == NodeTrait::getParent(const_cast<NodeT *>(BB))) &&
+ "cannot get DomTreeNode of block with different parent");
if (auto Idx = getNodeIndex(BB); Idx && *Idx < DomTreeNodes.size())
return DomTreeNodes[*Idx].get();
return nullptr;
diff --git a/llvm/lib/Analysis/TypeMetadataUtils.cpp b/llvm/lib/Analysis/TypeMetadataUtils.cpp
index 67ce1540112bb7..9ec0785eb5034d 100644
--- a/llvm/lib/Analysis/TypeMetadataUtils.cpp
+++ b/llvm/lib/Analysis/TypeMetadataUtils.cpp
@@ -33,6 +33,8 @@ findCallsAtConstantOffset(SmallVectorImpl<DevirtCallSite> &DevirtCalls,
// after indirect call promotion and inlining, where we may have uses
// of the vtable pointer guarded by a function pointer check, and a fallback
// indirect call.
+ if (CI->getFunction() != User->getFunction())
+ continue;
if (!DT.dominates(CI, User))
continue;
if (isa<BitCastInst>(User)) {
diff --git a/llvm/lib/Transforms/Scalar/AlignmentFromAssumptions.cpp b/llvm/lib/Transforms/Scalar/AlignmentFromAssumptions.cpp
index f3422a705dca7a..8555ef5c22f827 100644
--- a/llvm/lib/Transforms/Scalar/AlignmentFromAssumptions.cpp
+++ b/llvm/lib/Transforms/Scalar/AlignmentFromAssumptions.cpp
@@ -208,6 +208,7 @@ bool AlignmentFromAssumptionsPass::processAssumption(CallInst *ACall,
continue;
if (Instruction *K = dyn_cast<Instruction>(J))
+ if (K->getFunction() == ACall->getFunction())
WorkList.push_back(K);
}
diff --git a/llvm/lib/Transforms/Scalar/LoopFuse.cpp b/llvm/lib/Transforms/Scalar/LoopFuse.cpp
index 8512b2accbe7c2..fe0e30d1965e05 100644
--- a/llvm/lib/Transforms/Scalar/LoopFuse.cpp
+++ b/llvm/lib/Transforms/Scalar/LoopFuse.cpp
@@ -1729,7 +1729,9 @@ struct LoopFuser {
// mergeLatch may remove the only block in FC1.
SE.forgetLoop(FC1.L);
SE.forgetLoop(FC0.L);
- SE.forgetLoopDispositions();
+ // Forget block dispositions as well, so that there are no dangling
+ // pointers to erased/free'ed blocks.
+ SE.forgetBlockAndLoopDispositions();
// Move instructions from FC0.Latch to FC1.Latch.
// Note: mergeLatch requires an updated DT.
@@ -2023,7 +2025,9 @@ struct LoopFuser {
// mergeLatch may remove the only block in FC1.
SE.forgetLoop(FC1.L);
SE.forgetLoop(FC0.L);
- SE.forgetLoopDispositions();
+ // Forget block dispositions as well, so that there are no dangling
+ // pointers to erased/free'ed blocks.
+ SE.forgetBlockAndLoopDispositions();
// Move instructions from FC0.Latch to FC1.Latch.
// Note: mergeLatch requires an updated DT.
diff --git a/llvm/test/Transforms/AlignmentFromAssumptions/domtree-crash.ll b/llvm/test/Transforms/AlignmentFromAssumptions/domtree-crash.ll
new file mode 100644
index 00000000000000..c7fc1dc6996718
--- /dev/null
+++ b/llvm/test/Transforms/AlignmentFromAssumptions/domtree-crash.ll
@@ -0,0 +1,33 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
+; RUN: opt -passes=alignment-from-assumptions -S < %s | FileCheck %s
+
+; The alignment assumption is a global, which has users in a different
+; function. Test that in this case the dominator tree is only queried with
+; blocks from the same function.
+
+@global = external constant [192 x i8]
+
+define void @fn1() {
+; CHECK-LABEL: define void @fn1() {
+; CHECK-NEXT: call void @llvm.assume(i1 false) [ "align"(ptr @global, i64 1) ]
+; CHECK-NEXT: ret void
+;
+ call void @llvm.assume(i1 false) [ "align"(ptr @global, i64 1) ]
+ ret void
+}
+
+define void @fn2() {
+; CHECK-LABEL: define void @fn2() {
+; CHECK-NEXT: ret void
+; CHECK: [[LOOP:.*]]:
+; CHECK-NEXT: [[GEP:%.*]] = getelementptr inbounds i8, ptr @global, i64 0
+; CHECK-NEXT: [[LOAD:%.*]] = load i64, ptr [[GEP]], align 1
+; CHECK-NEXT: br label %[[LOOP]]
+;
+ ret void
+
+loop:
+ %gep = getelementptr inbounds i8, ptr @global, i64 0
+ %load = load i64, ptr %gep, align 1
+ br label %loop
+}
|
|
@aengelke |
|
Thanks @dyung for the reproducer, that was very helpful -- reduced further to a test case. I now was able to track it down to SLPVectorize and made a (imho) uncontroversial change to fix it.
Sure, nothing to be sorry about! :) |
nikic
reviewed
Aug 11, 2024
| ; CHECK-NEXT: ret void | ||
| ; | ||
| entry: | ||
| %0 = getelementptr i8, ptr null, i64 28 |
There was a problem hiding this comment.
Can you replace null with a global here to make the code not be immediate UB?
Is the code iterating over null users or over 0.000000e+00/1.000000e+00 users? If it's the latter, than this code probably needs a larger fix, because iterating over ConstantData users is not allowed in general. (Also applies to null, but in that case a global is a plausible replacement.)
There was a problem hiding this comment.
It's iterating over the ConstantData users. I'm not familiar with SLP, please advise how to fix the larger problem. (If iterating over such users is not allowed, why is there no assertion for this?)
There was a problem hiding this comment.
It's iterating over the ConstantData users. I'm not familiar with SLP, please advise how to fix the larger problem.
Adding an isa<ConstantData>(V) break; in this function should work. (You should still keep your check, in case we're iterating over GV users.)
If iterating over such users is not allowed, why is there no assertion for this?
Nobody has implemented it :)
llvm/test/Transforms/SLPVectorizer/const-in-different-functions.ll
Outdated
Show resolved
Hide resolved
|
(CI failure (timeout on lldb-api :: functionalities/fork/concurrent_vfork/TestConcurrentVFork.py) seems unrelated) |
|
Hi, I think we're seeing an assertion failure in two-stage builds after this patch. I'm still bisecting to be sure, but given the assertion we're seeing, I think its related. Would you mind taking a look, and reverting if its going to take a while to fix? First Failing Bot: https://ci.chromium.org/ui/p/fuchsia/builders/prod/clang-linux-x64/b8739716235612224273/overview You should be able to reproduce with any 2-stage build, as long as you're also building clang-format. |
|
The below now crashes with |
bwendling
pushed a commit
to bwendling/llvm-project
that referenced
this pull request
Aug 15, 2024
A dominance query of a block that is in a different function is ill-defined, so assert that getNode() is only called for blocks that are in the same function. There are three cases, where this behavior did occur. LoopFuse didn't explicitly do this, but didn't invalidate the SCEV block dispositions, leaving dangling pointers to free'ed basic blocks behind, causing use-after-free. We do, however, want to be able to dereference basic blocks inside the dominator tree, so that we can refer to them by a number stored inside the basic block. Reverts llvm#102780 Reland llvm#101198 Fixes llvm#102784 Co-authored-by: Alexis Engelke <engelke@in.tum.de>
|
Hi, I hit the new assertion with Result: Edit: I tested this on main commit f1779ae. |
|
That looks like a use-after-free of I have absolutely no idea of when nodes should be freed (or what the code does). Tagging some people who show up in git blame -- @cheshire @tkremenek how to fix? |
|
Haven't touched the code in 6 years, @haoNoQ should have the current contacts. |
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.
Reverts #102780
Reland #101198
Fixes #102784