https://github.com/jmorse updated
https://github.com/llvm/llvm-project/pull/124288
>From b60342427447c46d646d3de5b91fcbdbcb8585d3 Mon Sep 17 00:00:00 2001
From: Jeremy Morse <jeremy.mo...@sony.com>
Date: Thu, 23 Jan 2025 17:06:04 +0000
Subject: [PATCH 1/2] [NFC][DebugInfo] Rewrite more call-sites to insert with
iterators
As part of the "RemoveDIs" work to eliminate debug intrinsics, we're
replacing methods that use Instruction*'s as positions with iterators. The
call-sites updated in this patch are those where the dyn_cast_or_null cast
utility doesn't compose well with iterator insertion. It can distinguish
between nullptr and a "present" (non-null) Instruction pointer, but not
between a legal and illegal instruction iterator. This can lead to
end-iterator dereferences and thus crashes.
We can improve this in the future (as parent-pointers can now be accessed
from ilist nodes), but for the moment, add explicit tests for end()
iterators at the five call sites affected by this.
---
clang/lib/CodeGen/CGObjCRuntime.cpp | 12 +++--
llvm/lib/IR/Verifier.cpp | 5 +-
llvm/lib/Transforms/Coroutines/CoroFrame.cpp | 48 +++++++++++---------
llvm/lib/Transforms/Coroutines/CoroSplit.cpp | 15 +++++-
4 files changed, 49 insertions(+), 31 deletions(-)
diff --git a/clang/lib/CodeGen/CGObjCRuntime.cpp
b/clang/lib/CodeGen/CGObjCRuntime.cpp
index b438a92a4fd627..3732c635595f7c 100644
--- a/clang/lib/CodeGen/CGObjCRuntime.cpp
+++ b/clang/lib/CodeGen/CGObjCRuntime.cpp
@@ -230,11 +230,13 @@ void CGObjCRuntime::EmitTryCatchStmt(CodeGenFunction &CGF,
CodeGenFunction::LexicalScope Cleanups(CGF,
Handler.Body->getSourceRange());
SaveAndRestore RevertAfterScope(CGF.CurrentFuncletPad);
if (useFunclets) {
- llvm::Instruction *CPICandidate = Handler.Block->getFirstNonPHI();
- if (auto *CPI = dyn_cast_or_null<llvm::CatchPadInst>(CPICandidate)) {
- CGF.CurrentFuncletPad = CPI;
- CPI->setOperand(2, CGF.getExceptionSlot().emitRawPointer(CGF));
- CGF.EHStack.pushCleanup<CatchRetScope>(NormalCleanup, CPI);
+ llvm::BasicBlock::iterator CPICandidate =
Handler.Block->getFirstNonPHIIt();
+ if (CPICandidate != Handler.Block->end()) {
+ if (auto *CPI = dyn_cast_or_null<llvm::CatchPadInst>(CPICandidate)) {
+ CGF.CurrentFuncletPad = CPI;
+ CPI->setOperand(2, CGF.getExceptionSlot().emitRawPointer(CGF));
+ CGF.EHStack.pushCleanup<CatchRetScope>(NormalCleanup, CPI);
+ }
}
}
diff --git a/llvm/lib/IR/Verifier.cpp b/llvm/lib/IR/Verifier.cpp
index 54de812517438b..c6c2a93baabb6d 100644
--- a/llvm/lib/IR/Verifier.cpp
+++ b/llvm/lib/IR/Verifier.cpp
@@ -6521,8 +6521,9 @@ void Verifier::visitIntrinsicCall(Intrinsic::ID ID,
CallBase &Call) {
const ColorVector &CV = BlockEHFuncletColors.find(CallBB)->second;
assert(CV.size() > 0 && "Uncolored block");
for (BasicBlock *ColorFirstBB : CV)
- if (dyn_cast_or_null<FuncletPadInst>(ColorFirstBB->getFirstNonPHI()))
- InEHFunclet = true;
+ if (auto It = ColorFirstBB->getFirstNonPHIIt(); It !=
ColorFirstBB->end())
+ if (dyn_cast_or_null<FuncletPadInst>(&*It))
+ InEHFunclet = true;
// Check for funclet operand bundle
bool HasToken = false;
diff --git a/llvm/lib/Transforms/Coroutines/CoroFrame.cpp
b/llvm/lib/Transforms/Coroutines/CoroFrame.cpp
index 73d4fb9065831e..f146d2c205f125 100644
--- a/llvm/lib/Transforms/Coroutines/CoroFrame.cpp
+++ b/llvm/lib/Transforms/Coroutines/CoroFrame.cpp
@@ -1448,34 +1448,38 @@ static void rewritePHIs(BasicBlock &BB) {
// Special case for CleanupPad: all EH blocks must have the same unwind edge
// so we need to create an additional "dispatcher" block.
- if (auto *CleanupPad =
- dyn_cast_or_null<CleanupPadInst>(BB.getFirstNonPHI())) {
- SmallVector<BasicBlock *, 8> Preds(predecessors(&BB));
- for (BasicBlock *Pred : Preds) {
- if (CatchSwitchInst *CS =
- dyn_cast<CatchSwitchInst>(Pred->getTerminator())) {
- // CleanupPad with a CatchSwitch predecessor: therefore this is an
- // unwind destination that needs to be handle specially.
- assert(CS->getUnwindDest() == &BB);
- (void)CS;
- rewritePHIsForCleanupPad(&BB, CleanupPad);
- return;
+ if (!BB.empty()) {
+ if (auto *CleanupPad =
+ dyn_cast_or_null<CleanupPadInst>(BB.getFirstNonPHIIt())) {
+ SmallVector<BasicBlock *, 8> Preds(predecessors(&BB));
+ for (BasicBlock *Pred : Preds) {
+ if (CatchSwitchInst *CS =
+ dyn_cast<CatchSwitchInst>(Pred->getTerminator())) {
+ // CleanupPad with a CatchSwitch predecessor: therefore this is an
+ // unwind destination that needs to be handle specially.
+ assert(CS->getUnwindDest() == &BB);
+ (void)CS;
+ rewritePHIsForCleanupPad(&BB, CleanupPad);
+ return;
+ }
}
}
}
LandingPadInst *LandingPad = nullptr;
PHINode *ReplPHI = nullptr;
- if ((LandingPad = dyn_cast_or_null<LandingPadInst>(BB.getFirstNonPHI()))) {
- // ehAwareSplitEdge will clone the LandingPad in all the edge blocks.
- // We replace the original landing pad with a PHINode that will collect the
- // results from all of them.
- ReplPHI = PHINode::Create(LandingPad->getType(), 1, "");
- ReplPHI->insertBefore(LandingPad->getIterator());
- ReplPHI->takeName(LandingPad);
- LandingPad->replaceAllUsesWith(ReplPHI);
- // We will erase the original landing pad at the end of this function after
- // ehAwareSplitEdge cloned it in the transition blocks.
+ if (!BB.empty()) {
+ if ((LandingPad =
dyn_cast_or_null<LandingPadInst>(BB.getFirstNonPHIIt()))) {
+ // ehAwareSplitEdge will clone the LandingPad in all the edge blocks.
+ // We replace the original landing pad with a PHINode that will collect
the
+ // results from all of them.
+ ReplPHI = PHINode::Create(LandingPad->getType(), 1, "");
+ ReplPHI->insertBefore(LandingPad->getIterator());
+ ReplPHI->takeName(LandingPad);
+ LandingPad->replaceAllUsesWith(ReplPHI);
+ // We will erase the original landing pad at the end of this function
after
+ // ehAwareSplitEdge cloned it in the transition blocks.
+ }
}
SmallVector<BasicBlock *, 8> Preds(predecessors(&BB));
diff --git a/llvm/lib/Transforms/Coroutines/CoroSplit.cpp
b/llvm/lib/Transforms/Coroutines/CoroSplit.cpp
index ff5df12c398c56..202e635abc9148 100644
--- a/llvm/lib/Transforms/Coroutines/CoroSplit.cpp
+++ b/llvm/lib/Transforms/Coroutines/CoroSplit.cpp
@@ -823,7 +823,16 @@ static void updateScopeLine(Instruction *ActiveSuspend,
if (!ActiveSuspend)
return;
- auto *Successor = ActiveSuspend->getNextNonDebugInstruction();
+ // No subsequent instruction -> fallback to the location of ActiveSuspend.
+ if (!ActiveSuspend->getNextNonDebugInstruction()) {
+ if (auto DL = ActiveSuspend->getDebugLoc())
+ if (SPToUpdate.getFile() == DL->getFile())
+ SPToUpdate.setScopeLine(DL->getLine());
+ return;
+ }
+
+ BasicBlock::iterator Successor =
+ ActiveSuspend->getNextNonDebugInstruction()->getIterator();
// Corosplit splits the BB around ActiveSuspend, so the meaningful
// instructions are not in the same BB.
if (auto *Branch = dyn_cast_or_null<BranchInst>(Successor);
@@ -832,7 +841,9 @@ static void updateScopeLine(Instruction *ActiveSuspend,
// Find the first successor of ActiveSuspend with a non-zero line location.
// If that matches the file of ActiveSuspend, use it.
- for (; Successor; Successor = Successor->getNextNonDebugInstruction()) {
+ BasicBlock *PBB = Successor->getParent();
+ for (; Successor != PBB->end(); Successor = std::next(Successor)) {
+ Successor = skipDebugIntrinsics(Successor);
auto DL = Successor->getDebugLoc();
if (!DL || DL.getLine() == 0)
continue;
>From 021643588d58e548d1c6fe21eeecc526300f902e Mon Sep 17 00:00:00 2001
From: Jeremy Morse <jeremy.mo...@sony.com>
Date: Mon, 27 Jan 2025 17:31:35 +0000
Subject: [PATCH 2/2] clang-format
---
clang/lib/CodeGen/CGObjCRuntime.cpp | 3 ++-
llvm/lib/IR/Verifier.cpp | 3 ++-
llvm/lib/Transforms/Coroutines/CoroFrame.cpp | 3 ++-
llvm/lib/Transforms/Coroutines/CoroSplit.cpp | 2 +-
4 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/clang/lib/CodeGen/CGObjCRuntime.cpp
b/clang/lib/CodeGen/CGObjCRuntime.cpp
index 3732c635595f7c..a7f5c913f42fc1 100644
--- a/clang/lib/CodeGen/CGObjCRuntime.cpp
+++ b/clang/lib/CodeGen/CGObjCRuntime.cpp
@@ -230,7 +230,8 @@ void CGObjCRuntime::EmitTryCatchStmt(CodeGenFunction &CGF,
CodeGenFunction::LexicalScope Cleanups(CGF,
Handler.Body->getSourceRange());
SaveAndRestore RevertAfterScope(CGF.CurrentFuncletPad);
if (useFunclets) {
- llvm::BasicBlock::iterator CPICandidate =
Handler.Block->getFirstNonPHIIt();
+ llvm::BasicBlock::iterator CPICandidate =
+ Handler.Block->getFirstNonPHIIt();
if (CPICandidate != Handler.Block->end()) {
if (auto *CPI = dyn_cast_or_null<llvm::CatchPadInst>(CPICandidate)) {
CGF.CurrentFuncletPad = CPI;
diff --git a/llvm/lib/IR/Verifier.cpp b/llvm/lib/IR/Verifier.cpp
index c6c2a93baabb6d..8432779c107dec 100644
--- a/llvm/lib/IR/Verifier.cpp
+++ b/llvm/lib/IR/Verifier.cpp
@@ -6521,7 +6521,8 @@ void Verifier::visitIntrinsicCall(Intrinsic::ID ID,
CallBase &Call) {
const ColorVector &CV = BlockEHFuncletColors.find(CallBB)->second;
assert(CV.size() > 0 && "Uncolored block");
for (BasicBlock *ColorFirstBB : CV)
- if (auto It = ColorFirstBB->getFirstNonPHIIt(); It !=
ColorFirstBB->end())
+ if (auto It = ColorFirstBB->getFirstNonPHIIt();
+ It != ColorFirstBB->end())
if (dyn_cast_or_null<FuncletPadInst>(&*It))
InEHFunclet = true;
diff --git a/llvm/lib/Transforms/Coroutines/CoroFrame.cpp
b/llvm/lib/Transforms/Coroutines/CoroFrame.cpp
index f146d2c205f125..e104b3548c0266 100644
--- a/llvm/lib/Transforms/Coroutines/CoroFrame.cpp
+++ b/llvm/lib/Transforms/Coroutines/CoroFrame.cpp
@@ -1469,7 +1469,8 @@ static void rewritePHIs(BasicBlock &BB) {
LandingPadInst *LandingPad = nullptr;
PHINode *ReplPHI = nullptr;
if (!BB.empty()) {
- if ((LandingPad =
dyn_cast_or_null<LandingPadInst>(BB.getFirstNonPHIIt()))) {
+ if ((LandingPad =
+ dyn_cast_or_null<LandingPadInst>(BB.getFirstNonPHIIt()))) {
// ehAwareSplitEdge will clone the LandingPad in all the edge blocks.
// We replace the original landing pad with a PHINode that will collect
the
// results from all of them.
diff --git a/llvm/lib/Transforms/Coroutines/CoroSplit.cpp
b/llvm/lib/Transforms/Coroutines/CoroSplit.cpp
index 202e635abc9148..00513126dd1fc9 100644
--- a/llvm/lib/Transforms/Coroutines/CoroSplit.cpp
+++ b/llvm/lib/Transforms/Coroutines/CoroSplit.cpp
@@ -832,7 +832,7 @@ static void updateScopeLine(Instruction *ActiveSuspend,
}
BasicBlock::iterator Successor =
- ActiveSuspend->getNextNonDebugInstruction()->getIterator();
+ ActiveSuspend->getNextNonDebugInstruction()->getIterator();
// Corosplit splits the BB around ActiveSuspend, so the meaningful
// instructions are not in the same BB.
if (auto *Branch = dyn_cast_or_null<BranchInst>(Successor);
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
