Skip to content

Commit 285009f

Browse files
committed
[NFC][DebugInfo] Rewrite more call-sites to insert with iterators (#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.
1 parent a85b2dc commit 285009f

File tree

4 files changed

+53
-32
lines changed

4 files changed

+53
-32
lines changed

clang/lib/CodeGen/CGObjCRuntime.cpp

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -230,11 +230,14 @@ void CGObjCRuntime::EmitTryCatchStmt(CodeGenFunction &CGF,
230230
CodeGenFunction::LexicalScope Cleanups(CGF, Handler.Body->getSourceRange());
231231
SaveAndRestore RevertAfterScope(CGF.CurrentFuncletPad);
232232
if (useFunclets) {
233-
llvm::Instruction *CPICandidate = Handler.Block->getFirstNonPHI();
234-
if (auto *CPI = dyn_cast_or_null<llvm::CatchPadInst>(CPICandidate)) {
235-
CGF.CurrentFuncletPad = CPI;
236-
CPI->setOperand(2, CGF.getExceptionSlot().emitRawPointer(CGF));
237-
CGF.EHStack.pushCleanup<CatchRetScope>(NormalCleanup, CPI);
233+
llvm::BasicBlock::iterator CPICandidate =
234+
Handler.Block->getFirstNonPHIIt();
235+
if (CPICandidate != Handler.Block->end()) {
236+
if (auto *CPI = dyn_cast_or_null<llvm::CatchPadInst>(CPICandidate)) {
237+
CGF.CurrentFuncletPad = CPI;
238+
CPI->setOperand(2, CGF.getExceptionSlot().emitRawPointer(CGF));
239+
CGF.EHStack.pushCleanup<CatchRetScope>(NormalCleanup, CPI);
240+
}
238241
}
239242
}
240243

llvm/lib/IR/Verifier.cpp

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6521,8 +6521,10 @@ void Verifier::visitIntrinsicCall(Intrinsic::ID ID, CallBase &Call) {
65216521
const ColorVector &CV = BlockEHFuncletColors.find(CallBB)->second;
65226522
assert(CV.size() > 0 && "Uncolored block");
65236523
for (BasicBlock *ColorFirstBB : CV)
6524-
if (dyn_cast_or_null<FuncletPadInst>(ColorFirstBB->getFirstNonPHI()))
6525-
InEHFunclet = true;
6524+
if (auto It = ColorFirstBB->getFirstNonPHIIt();
6525+
It != ColorFirstBB->end())
6526+
if (dyn_cast_or_null<FuncletPadInst>(&*It))
6527+
InEHFunclet = true;
65266528

65276529
// Check for funclet operand bundle
65286530
bool HasToken = false;

llvm/lib/Transforms/Coroutines/CoroFrame.cpp

Lines changed: 27 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1448,34 +1448,39 @@ static void rewritePHIs(BasicBlock &BB) {
14481448

14491449
// Special case for CleanupPad: all EH blocks must have the same unwind edge
14501450
// so we need to create an additional "dispatcher" block.
1451-
if (auto *CleanupPad =
1452-
dyn_cast_or_null<CleanupPadInst>(BB.getFirstNonPHI())) {
1453-
SmallVector<BasicBlock *, 8> Preds(predecessors(&BB));
1454-
for (BasicBlock *Pred : Preds) {
1455-
if (CatchSwitchInst *CS =
1456-
dyn_cast<CatchSwitchInst>(Pred->getTerminator())) {
1457-
// CleanupPad with a CatchSwitch predecessor: therefore this is an
1458-
// unwind destination that needs to be handle specially.
1459-
assert(CS->getUnwindDest() == &BB);
1460-
(void)CS;
1461-
rewritePHIsForCleanupPad(&BB, CleanupPad);
1462-
return;
1451+
if (!BB.empty()) {
1452+
if (auto *CleanupPad =
1453+
dyn_cast_or_null<CleanupPadInst>(BB.getFirstNonPHIIt())) {
1454+
SmallVector<BasicBlock *, 8> Preds(predecessors(&BB));
1455+
for (BasicBlock *Pred : Preds) {
1456+
if (CatchSwitchInst *CS =
1457+
dyn_cast<CatchSwitchInst>(Pred->getTerminator())) {
1458+
// CleanupPad with a CatchSwitch predecessor: therefore this is an
1459+
// unwind destination that needs to be handle specially.
1460+
assert(CS->getUnwindDest() == &BB);
1461+
(void)CS;
1462+
rewritePHIsForCleanupPad(&BB, CleanupPad);
1463+
return;
1464+
}
14631465
}
14641466
}
14651467
}
14661468

14671469
LandingPadInst *LandingPad = nullptr;
14681470
PHINode *ReplPHI = nullptr;
1469-
if ((LandingPad = dyn_cast_or_null<LandingPadInst>(BB.getFirstNonPHI()))) {
1470-
// ehAwareSplitEdge will clone the LandingPad in all the edge blocks.
1471-
// We replace the original landing pad with a PHINode that will collect the
1472-
// results from all of them.
1473-
ReplPHI = PHINode::Create(LandingPad->getType(), 1, "");
1474-
ReplPHI->insertBefore(LandingPad->getIterator());
1475-
ReplPHI->takeName(LandingPad);
1476-
LandingPad->replaceAllUsesWith(ReplPHI);
1477-
// We will erase the original landing pad at the end of this function after
1478-
// ehAwareSplitEdge cloned it in the transition blocks.
1471+
if (!BB.empty()) {
1472+
if ((LandingPad =
1473+
dyn_cast_or_null<LandingPadInst>(BB.getFirstNonPHIIt()))) {
1474+
// ehAwareSplitEdge will clone the LandingPad in all the edge blocks.
1475+
// We replace the original landing pad with a PHINode that will collect the
1476+
// results from all of them.
1477+
ReplPHI = PHINode::Create(LandingPad->getType(), 1, "");
1478+
ReplPHI->insertBefore(LandingPad->getIterator());
1479+
ReplPHI->takeName(LandingPad);
1480+
LandingPad->replaceAllUsesWith(ReplPHI);
1481+
// We will erase the original landing pad at the end of this function after
1482+
// ehAwareSplitEdge cloned it in the transition blocks.
1483+
}
14791484
}
14801485

14811486
SmallVector<BasicBlock *, 8> Preds(predecessors(&BB));

llvm/lib/Transforms/Coroutines/CoroSplit.cpp

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -824,16 +824,27 @@ static void updateScopeLine(Instruction *ActiveSuspend,
824824
if (!ActiveSuspend)
825825
return;
826826

827-
auto *Successor = ActiveSuspend->getNextNonDebugInstruction();
827+
// No subsequent instruction -> fallback to the location of ActiveSuspend.
828+
if (!ActiveSuspend->getNextNonDebugInstruction()) {
829+
if (auto DL = ActiveSuspend->getDebugLoc())
830+
if (SPToUpdate.getFile() == DL->getFile())
831+
SPToUpdate.setScopeLine(DL->getLine());
832+
return;
833+
}
834+
835+
BasicBlock::iterator Successor =
836+
ActiveSuspend->getNextNonDebugInstruction()->getIterator();
828837
// Corosplit splits the BB around ActiveSuspend, so the meaningful
829838
// instructions are not in the same BB.
830839
if (auto *Branch = dyn_cast_or_null<BranchInst>(Successor);
831840
Branch && Branch->isUnconditional())
832-
Successor = &*Branch->getSuccessor(0)->getFirstNonPHIOrDbg();
841+
Successor = Branch->getSuccessor(0)->getFirstNonPHIOrDbg();
833842

834843
// Find the first successor of ActiveSuspend with a non-zero line location.
835844
// If that matches the file of ActiveSuspend, use it.
836-
for (; Successor; Successor = Successor->getNextNonDebugInstruction()) {
845+
BasicBlock *PBB = Successor->getParent();
846+
for (; Successor != PBB->end(); Successor = std::next(Successor)) {
847+
Successor = skipDebugIntrinsics(Successor);
837848
auto DL = Successor->getDebugLoc();
838849
if (!DL || DL.getLine() == 0)
839850
continue;

0 commit comments

Comments
 (0)