[NFC][DebugInfo] Rewrite more call-sites to insert with iterators #124288
Conversation
This file contains hidden or 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
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.
llvmbot
added
clang
|
@llvm/pr-subscribers-llvm-ir @llvm/pr-subscribers-llvm-transforms Author: Jeremy Morse (jmorse) ChangesAs 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. Full diff: https://github.com/llvm/llvm-project/pull/124288.diff 4 Files Affected:
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;
|
|
@llvm/pr-subscribers-clang Author: Jeremy Morse (jmorse) ChangesAs 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. Full diff: https://github.com/llvm/llvm-project/pull/124288.diff 4 Files Affected:
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;
|
|
@llvm/pr-subscribers-clang-codegen Author: Jeremy Morse (jmorse) ChangesAs 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. Full diff: https://github.com/llvm/llvm-project/pull/124288.diff 4 Files Affected:
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;
|
You can test this locally with the following command:git-clang-format --diff c546b5317c518987a5f45dd4c4d25321a955c758 021643588d58e548d1c6fe21eeecc526300f902e --extensions cpp -- clang/lib/CodeGen/CGObjCRuntime.cpp llvm/lib/IR/Verifier.cpp llvm/lib/Transforms/Coroutines/CoroFrame.cpp llvm/lib/Transforms/Coroutines/CoroSplit.cppView the diff from clang-format here.diff --git a/llvm/lib/Transforms/Coroutines/CoroFrame.cpp b/llvm/lib/Transforms/Coroutines/CoroFrame.cpp
index e104b3548c..aa56fc5650 100644
--- a/llvm/lib/Transforms/Coroutines/CoroFrame.cpp
+++ b/llvm/lib/Transforms/Coroutines/CoroFrame.cpp
@@ -1472,14 +1472,14 @@ static void rewritePHIs(BasicBlock &BB) {
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.
+ // 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.
+ // We will erase the original landing pad at the end of this function
+ // after ehAwareSplitEdge cloned it in the transition blocks.
}
}
|
OCHyams
approved these changes
Jan 27, 2025
| return; | ||
| } | ||
|
|
||
| BasicBlock::iterator Successor = |
There was a problem hiding this comment.
this doesn't break the dyn_cast_or_null below?
There was a problem hiding this comment.
The _or_null part is delt with in the "no subsequent instruction" portion I've added; the dyn_cast will dereference the iterator and examine the instruction automagically.
(The situation is a pain in the neck ._.)
jmorse
added a commit
that referenced
this pull request
Jan 27, 2025
…24288) 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.
|
Merged in 285009f as it was composing with a workaround in an earlier commit. |
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.
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.