[llvm] [Support] Assert that DomTree nodes share parent (PR #101198)
via llvm-commits
llvm-commits at lists.llvm.org
Tue Jul 30 08:38:47 PDT 2024
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-llvm-transforms
Author: Alexis Engelke (aengelke)
<details>
<summary>Changes</summary>
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 two 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.
---
Depends on #<!-- -->101195, included here as first commit (only look at the second commit, will rebase once #<!-- -->101195 is merged).
---
Full diff: https://github.com/llvm/llvm-project/pull/101198.diff
9 Files Affected:
- (modified) llvm/include/llvm/Analysis/GenericDomTreeUpdater.h (+1-1)
- (modified) llvm/include/llvm/Support/GenericDomTree.h (+2)
- (modified) llvm/lib/Analysis/DomTreeUpdater.cpp (+3-5)
- (modified) llvm/lib/Analysis/TypeMetadataUtils.cpp (+2)
- (modified) llvm/lib/Analysis/ValueTracking.cpp (+4)
- (modified) llvm/lib/CodeGen/EarlyIfConversion.cpp (+27-19)
- (modified) llvm/lib/Target/AArch64/AArch64ConditionalCompares.cpp (+2-1)
- (modified) llvm/lib/Transforms/Scalar/LoopFuse.cpp (+6-2)
- (modified) llvm/unittests/IR/DominatorTreeTest.cpp (+1-2)
``````````diff
diff --git a/llvm/include/llvm/Analysis/GenericDomTreeUpdater.h b/llvm/include/llvm/Analysis/GenericDomTreeUpdater.h
index 84ed882c6de84..ca4ce68b85cbc 100644
--- a/llvm/include/llvm/Analysis/GenericDomTreeUpdater.h
+++ b/llvm/include/llvm/Analysis/GenericDomTreeUpdater.h
@@ -232,7 +232,7 @@ class GenericDomTreeUpdater {
/// insertEdge/deleteEdge or is unnecessary in the batch update.
bool isUpdateValid(typename DomTreeT::UpdateType Update) const;
- /// Erase Basic Block node that has been unlinked from Function
+ /// Erase Basic Block node before it is unlinked from Function
/// in the DomTree and PostDomTree.
void eraseDelBBNode(BasicBlockT *DelBB);
diff --git a/llvm/include/llvm/Support/GenericDomTree.h b/llvm/include/llvm/Support/GenericDomTree.h
index e05e5f0f842e3..00bf607fbc8b1 100644
--- a/llvm/include/llvm/Support/GenericDomTree.h
+++ b/llvm/include/llvm/Support/GenericDomTree.h
@@ -359,6 +359,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");
auto I = DomTreeNodes.find(BB);
if (I != DomTreeNodes.end())
return I->second.get();
diff --git a/llvm/lib/Analysis/DomTreeUpdater.cpp b/llvm/lib/Analysis/DomTreeUpdater.cpp
index 6895317c1d03a..351bd66e389bc 100644
--- a/llvm/lib/Analysis/DomTreeUpdater.cpp
+++ b/llvm/lib/Analysis/DomTreeUpdater.cpp
@@ -42,9 +42,8 @@ bool DomTreeUpdater::forceFlushDeletedBB() {
// delete only has an UnreachableInst inside.
assert(BB->size() == 1 && isa<UnreachableInst>(BB->getTerminator()) &&
"DelBB has been modified while awaiting deletion.");
- BB->removeFromParent();
eraseDelBBNode(BB);
- delete BB;
+ BB->eraseFromParent();
}
DeletedBBs.clear();
Callbacks.clear();
@@ -63,9 +62,8 @@ void DomTreeUpdater::deleteBB(BasicBlock *DelBB) {
return;
}
- DelBB->removeFromParent();
eraseDelBBNode(DelBB);
- delete DelBB;
+ DelBB->eraseFromParent();
}
void DomTreeUpdater::callbackDeleteBB(
@@ -77,8 +75,8 @@ void DomTreeUpdater::callbackDeleteBB(
return;
}
- DelBB->removeFromParent();
eraseDelBBNode(DelBB);
+ DelBB->removeFromParent();
Callback(DelBB);
delete DelBB;
}
diff --git a/llvm/lib/Analysis/TypeMetadataUtils.cpp b/llvm/lib/Analysis/TypeMetadataUtils.cpp
index 67ce1540112bb..9ec0785eb5034 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/Analysis/ValueTracking.cpp b/llvm/lib/Analysis/ValueTracking.cpp
index 285284dc27071..f813cba0167e6 100644
--- a/llvm/lib/Analysis/ValueTracking.cpp
+++ b/llvm/lib/Analysis/ValueTracking.cpp
@@ -526,6 +526,10 @@ bool llvm::isValidAssumeForContext(const Instruction *Inv,
return AllowEphemerals || !isEphemeralValueOf(Inv, CxtI);
}
+ // Inv and CxtI are in different functions.
+ if (Inv->getFunction() != CxtI->getFunction())
+ return false;
+
// Inv and CxtI are in different blocks.
if (DT) {
if (DT->dominates(Inv, CxtI))
diff --git a/llvm/lib/CodeGen/EarlyIfConversion.cpp b/llvm/lib/CodeGen/EarlyIfConversion.cpp
index d506c625d8ca5..0de8112fb72c8 100644
--- a/llvm/lib/CodeGen/EarlyIfConversion.cpp
+++ b/llvm/lib/CodeGen/EarlyIfConversion.cpp
@@ -181,8 +181,8 @@ class SSAIfConv {
bool canConvertIf(MachineBasicBlock *MBB, bool Predicate = false);
/// convertIf - If-convert the last block passed to canConvertIf(), assuming
- /// it is possible. Add any erased blocks to RemovedBlocks.
- void convertIf(SmallVectorImpl<MachineBasicBlock *> &RemovedBlocks,
+ /// it is possible. Add any blocks that are to be erased to RemoveBlocks.
+ void convertIf(SmallVectorImpl<MachineBasicBlock *> &RemoveBlocks,
bool Predicate = false);
};
} // end anonymous namespace
@@ -678,9 +678,9 @@ void SSAIfConv::rewritePHIOperands() {
/// convertIf - Execute the if conversion after canConvertIf has determined the
/// feasibility.
///
-/// Any basic blocks erased will be added to RemovedBlocks.
+/// Any basic blocks that need to be erased will be added to RemoveBlocks.
///
-void SSAIfConv::convertIf(SmallVectorImpl<MachineBasicBlock *> &RemovedBlocks,
+void SSAIfConv::convertIf(SmallVectorImpl<MachineBasicBlock *> &RemoveBlocks,
bool Predicate) {
assert(Head && Tail && TBB && FBB && "Call canConvertIf first.");
@@ -721,15 +721,18 @@ void SSAIfConv::convertIf(SmallVectorImpl<MachineBasicBlock *> &RemovedBlocks,
DebugLoc HeadDL = Head->getFirstTerminator()->getDebugLoc();
TII->removeBranch(*Head);
- // Erase the now empty conditional blocks. It is likely that Head can fall
+ // Mark the now empty conditional blocks for removal and move them to the end.
+ // It is likely that Head can fall
// through to Tail, and we can join the two blocks.
if (TBB != Tail) {
- RemovedBlocks.push_back(TBB);
- TBB->eraseFromParent();
+ RemoveBlocks.push_back(TBB);
+ if (TBB != &TBB->getParent()->back())
+ TBB->moveAfter(&TBB->getParent()->back());
}
if (FBB != Tail) {
- RemovedBlocks.push_back(FBB);
- FBB->eraseFromParent();
+ RemoveBlocks.push_back(FBB);
+ if (FBB != &FBB->getParent()->back())
+ FBB->moveAfter(&FBB->getParent()->back());
}
assert(Head->succ_empty() && "Additional head successors?");
@@ -740,8 +743,9 @@ void SSAIfConv::convertIf(SmallVectorImpl<MachineBasicBlock *> &RemovedBlocks,
Head->splice(Head->end(), Tail,
Tail->begin(), Tail->end());
Head->transferSuccessorsAndUpdatePHIs(Tail);
- RemovedBlocks.push_back(Tail);
- Tail->eraseFromParent();
+ RemoveBlocks.push_back(Tail);
+ if (Tail != &Tail->getParent()->back())
+ Tail->moveAfter(&Tail->getParent()->back());
} else {
// We need a branch to Tail, let code placement work it out later.
LLVM_DEBUG(dbgs() << "Converting to unconditional branch.\n");
@@ -1062,11 +1066,13 @@ bool EarlyIfConverter::tryConvertIf(MachineBasicBlock *MBB) {
while (IfConv.canConvertIf(MBB) && shouldConvertIf()) {
// If-convert MBB and update analyses.
invalidateTraces();
- SmallVector<MachineBasicBlock*, 4> RemovedBlocks;
- IfConv.convertIf(RemovedBlocks);
+ SmallVector<MachineBasicBlock *, 4> RemoveBlocks;
+ IfConv.convertIf(RemoveBlocks);
Changed = true;
- updateDomTree(DomTree, IfConv, RemovedBlocks);
- updateLoops(Loops, RemovedBlocks);
+ updateDomTree(DomTree, IfConv, RemoveBlocks);
+ for (MachineBasicBlock *MBB : RemoveBlocks)
+ MBB->eraseFromParent();
+ updateLoops(Loops, RemoveBlocks);
}
return Changed;
}
@@ -1200,11 +1206,13 @@ bool EarlyIfPredicator::tryConvertIf(MachineBasicBlock *MBB) {
bool Changed = false;
while (IfConv.canConvertIf(MBB, /*Predicate*/ true) && shouldConvertIf()) {
// If-convert MBB and update analyses.
- SmallVector<MachineBasicBlock *, 4> RemovedBlocks;
- IfConv.convertIf(RemovedBlocks, /*Predicate*/ true);
+ SmallVector<MachineBasicBlock *, 4> RemoveBlocks;
+ IfConv.convertIf(RemoveBlocks, /*Predicate*/ true);
Changed = true;
- updateDomTree(DomTree, IfConv, RemovedBlocks);
- updateLoops(Loops, RemovedBlocks);
+ updateDomTree(DomTree, IfConv, RemoveBlocks);
+ for (MachineBasicBlock *MBB : RemoveBlocks)
+ MBB->eraseFromParent();
+ updateLoops(Loops, RemoveBlocks);
}
return Changed;
}
diff --git a/llvm/lib/Target/AArch64/AArch64ConditionalCompares.cpp b/llvm/lib/Target/AArch64/AArch64ConditionalCompares.cpp
index 49e5211af50cc..9669a393bc2b9 100644
--- a/llvm/lib/Target/AArch64/AArch64ConditionalCompares.cpp
+++ b/llvm/lib/Target/AArch64/AArch64ConditionalCompares.cpp
@@ -711,7 +711,6 @@ void SSACCmpConv::convert(SmallVectorImpl<MachineBasicBlock *> &RemovedBlocks) {
Head->updateTerminator(CmpBB->getNextNode());
RemovedBlocks.push_back(CmpBB);
- CmpBB->eraseFromParent();
LLVM_DEBUG(dbgs() << "Result:\n" << *Head);
++NumConverted;
}
@@ -918,6 +917,8 @@ bool AArch64ConditionalCompares::tryConvert(MachineBasicBlock *MBB) {
CmpConv.convert(RemovedBlocks);
Changed = true;
updateDomTree(RemovedBlocks);
+ for (MachineBasicBlock *MBB : RemovedBlocks)
+ MBB->eraseFromParent();
updateLoops(RemovedBlocks);
}
return Changed;
diff --git a/llvm/lib/Transforms/Scalar/LoopFuse.cpp b/llvm/lib/Transforms/Scalar/LoopFuse.cpp
index 8512b2accbe7c..fe0e30d1965e0 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/unittests/IR/DominatorTreeTest.cpp b/llvm/unittests/IR/DominatorTreeTest.cpp
index 44bde74ad350f..555348c65a63d 100644
--- a/llvm/unittests/IR/DominatorTreeTest.cpp
+++ b/llvm/unittests/IR/DominatorTreeTest.cpp
@@ -607,11 +607,10 @@ TEST(DominatorTree, DeletingEdgesIntroducesInfiniteLoop2) {
SwitchC->removeCase(SwitchC->case_begin());
DT->deleteEdge(C, C2);
PDT->deleteEdge(C, C2);
- C2->removeFromParent();
EXPECT_EQ(DT->getNode(C2), nullptr);
PDT->eraseNode(C2);
- delete C2;
+ C2->eraseFromParent();
EXPECT_TRUE(DT->verify());
EXPECT_TRUE(PDT->verify());
``````````
</details>
https://github.com/llvm/llvm-project/pull/101198
More information about the llvm-commits
mailing list