Skip to content

Revert "[Support] Assert that DomTree nodes share parent" #102780

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 11, 2024

Conversation

vitalybuka
Copy link
Collaborator

@vitalybuka vitalybuka merged commit 3c3df1b into main Aug 11, 2024
8 of 11 checks passed
@vitalybuka vitalybuka deleted the revert-101198-domtreecleanup4 branch August 11, 2024 01:36
@llvmbot
Copy link
Member

llvmbot commented Aug 11, 2024

@llvm/pr-subscribers-llvm-support

@llvm/pr-subscribers-llvm-analysis

Author: Vitaly Buka (vitalybuka)

Changes

Reverts llvm/llvm-project#101198

Breaks multiple bots:
https://lab.llvm.org/buildbot/#/builders/72/builds/2103
https://lab.llvm.org/buildbot/#/builders/164/builds/1909
https://lab.llvm.org/buildbot/#/builders/66/builds/2706


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

5 Files Affected:

  • (modified) llvm/include/llvm/Support/GenericDomTree.h (-2)
  • (modified) llvm/lib/Analysis/TypeMetadataUtils.cpp (-2)
  • (modified) llvm/lib/Transforms/Scalar/AlignmentFromAssumptions.cpp (-1)
  • (modified) llvm/lib/Transforms/Scalar/LoopFuse.cpp (+2-6)
  • (removed) llvm/test/Transforms/AlignmentFromAssumptions/domtree-crash.ll (-33)
diff --git a/llvm/include/llvm/Support/GenericDomTree.h b/llvm/include/llvm/Support/GenericDomTree.h
index 45ef38b965b752..7e2b68e6faea29 100644
--- a/llvm/include/llvm/Support/GenericDomTree.h
+++ b/llvm/include/llvm/Support/GenericDomTree.h
@@ -397,8 +397,6 @@ 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 9ec0785eb5034d..67ce1540112bb7 100644
--- a/llvm/lib/Analysis/TypeMetadataUtils.cpp
+++ b/llvm/lib/Analysis/TypeMetadataUtils.cpp
@@ -33,8 +33,6 @@ 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 8555ef5c22f827..f3422a705dca7a 100644
--- a/llvm/lib/Transforms/Scalar/AlignmentFromAssumptions.cpp
+++ b/llvm/lib/Transforms/Scalar/AlignmentFromAssumptions.cpp
@@ -208,7 +208,6 @@ 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 fe0e30d1965e05..8512b2accbe7c2 100644
--- a/llvm/lib/Transforms/Scalar/LoopFuse.cpp
+++ b/llvm/lib/Transforms/Scalar/LoopFuse.cpp
@@ -1729,9 +1729,7 @@ struct LoopFuser {
     // mergeLatch may remove the only block in FC1.
     SE.forgetLoop(FC1.L);
     SE.forgetLoop(FC0.L);
-    // Forget block dispositions as well, so that there are no dangling
-    // pointers to erased/free'ed blocks.
-    SE.forgetBlockAndLoopDispositions();
+    SE.forgetLoopDispositions();
 
     // Move instructions from FC0.Latch to FC1.Latch.
     // Note: mergeLatch requires an updated DT.
@@ -2025,9 +2023,7 @@ struct LoopFuser {
     // mergeLatch may remove the only block in FC1.
     SE.forgetLoop(FC1.L);
     SE.forgetLoop(FC0.L);
-    // Forget block dispositions as well, so that there are no dangling
-    // pointers to erased/free'ed blocks.
-    SE.forgetBlockAndLoopDispositions();
+    SE.forgetLoopDispositions();
 
     // 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
deleted file mode 100644
index c7fc1dc6996718..00000000000000
--- a/llvm/test/Transforms/AlignmentFromAssumptions/domtree-crash.ll
+++ /dev/null
@@ -1,33 +0,0 @@
-; 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
-}

vitalybuka added a commit that referenced this pull request Aug 11, 2024
aengelke added a commit that referenced this pull request Aug 13, 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 #102780
Reland #101198
Fixes #102784

Co-authored-by: Alexis Engelke <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants