[clang] [llvm] [NFC][DebugInfo] Rewrite more call-sites to insert with iterators (PR #124288)
Jeremy Morse via llvm-commits
llvm-commits at lists.llvm.org
Fri Jan 24 07:23:04 PST 2025
https://github.com/jmorse created https://github.com/llvm/llvm-project/pull/124288
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, to avoid crashes.
It's not 100% clear to me that all these sites actually _need_ to handle the "next" instruction being non-present, but that's what they _do_, so we're going to support it.
>From b60342427447c46d646d3de5b91fcbdbcb8585d3 Mon Sep 17 00:00:00 2001
From: Jeremy Morse <jeremy.morse at sony.com>
Date: Thu, 23 Jan 2025 17:06:04 +0000
Subject: [PATCH] [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;
More information about the llvm-commits
mailing list