From 282a7d19ac63274e062d7e30a5fd47160390c161 Mon Sep 17 00:00:00 2001 From: Orlando Cazalet-Hyams Date: Thu, 22 Aug 2024 14:52:34 +0100 Subject: [PATCH 1/2] [RemoveDIs] Fix spliceDebugInfo splice-to-end edge case Fix #105571 which demonstrates an end() iterator dereference when performing a non-empty splice to end() from a region that ends at Src::end(). Rather than calling Instruction::adoptDbgRecords from Dest, create a marker (which takes an iterator) and absorbDebugValues onto that. The "absorb" variant doesn't clean up the source marker, which in this case we know is a trailing marker, so we have to do that manually. --- llvm/lib/IR/BasicBlock.cpp | 12 ++++- llvm/unittests/IR/BasicBlockDbgInfoTest.cpp | 54 +++++++++++++++++++++ 2 files changed, 64 insertions(+), 2 deletions(-) diff --git a/llvm/lib/IR/BasicBlock.cpp b/llvm/lib/IR/BasicBlock.cpp index cf05b11c53963..f2407bb3f37cd 100644 --- a/llvm/lib/IR/BasicBlock.cpp +++ b/llvm/lib/IR/BasicBlock.cpp @@ -971,8 +971,16 @@ void BasicBlock::spliceDebugInfoImpl(BasicBlock::iterator Dest, BasicBlock *Src, if (ReadFromTail && Src->getMarker(Last)) { DbgMarker *FromLast = Src->getMarker(Last); if (LastIsEnd) { - Dest->adoptDbgRecords(Src, Last, true); - // adoptDbgRecords will release any trailers. + if (Dest == end()) { + // Abosrb the trailing markers from Src. + assert(FromLast == Src->getTrailingDbgRecords()); + createMarker(Dest)->absorbDebugValues(*FromLast, true); + FromLast->eraseFromParent(); + Src->deleteTrailingDbgRecords(); + } else { + // adoptDbgRecords will release any trailers. + Dest->adoptDbgRecords(Src, Last, true); + } assert(!Src->getTrailingDbgRecords()); } else { // FIXME: can we use adoptDbgRecords here to reduce allocations? diff --git a/llvm/unittests/IR/BasicBlockDbgInfoTest.cpp b/llvm/unittests/IR/BasicBlockDbgInfoTest.cpp index 91a0745a0cc76..fc95f1838c307 100644 --- a/llvm/unittests/IR/BasicBlockDbgInfoTest.cpp +++ b/llvm/unittests/IR/BasicBlockDbgInfoTest.cpp @@ -1525,4 +1525,58 @@ TEST(BasicBlockDbgInfoTest, DbgMoveToEnd) { EXPECT_FALSE(Ret->hasDbgRecords()); } +TEST(BasicBlockDbgInfoTest, DbgKnownSentinelCrash) { + return; + LLVMContext C; + std::unique_ptr M = parseIR(C, R"( + define i16 @foo(i16 %a) !dbg !6 { + entry: + %b = add i16 %a, 0 + #dbg_value(i16 %b, !9, !DIExpression(), !11) + ret i16 0, !dbg !11 + } + + !llvm.dbg.cu = !{!0} + !llvm.module.flags = !{!5} + + !0 = distinct !DICompileUnit(language: DW_LANG_C, file: !1, producer: "debugify", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2) + !1 = !DIFile(filename: "t.ll", directory: "/") + !2 = !{} + !5 = !{i32 2, !"Debug Info Version", i32 3} + !6 = distinct !DISubprogram(name: "foo", linkageName: "foo", scope: null, file: !1, line: 1, type: !7, scopeLine: 1, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !8) + !7 = !DISubroutineType(types: !2) + !8 = !{!9} + !9 = !DILocalVariable(name: "1", scope: !6, file: !1, line: 1, type: !10) + !10 = !DIBasicType(name: "ty16", size: 16, encoding: DW_ATE_unsigned) + !11 = !DILocation(line: 1, column: 1, scope: !6) +)"); + ASSERT_TRUE(M); + + Function *F = M->getFunction("foo"); + BasicBlock &BB = F->getEntryBlock(); + // Start with no trailing records. + ASSERT_FALSE(BB.getTrailingDbgRecords()); + + BasicBlock::iterator Ret = std::prev(BB.end()); + BasicBlock::iterator B = std::prev(Ret); + + // Delete terminator which has debug records: we now get trailing records. + Ret->eraseFromParent(); + EXPECT_TRUE(BB.getTrailingDbgRecords()); + + BasicBlock *NewBB = BasicBlock::Create(C, "NewBB", F); + NewBB->splice(NewBB->end(), &BB, B, BB.end()); + + // The trailing records should've been absorbed into NewBB. + EXPECT_FALSE(BB.getTrailingDbgRecords()); + EXPECT_TRUE(NewBB->getTrailingDbgRecords()); + if (NewBB->getTrailingDbgRecords()) + EXPECT_EQ( + llvm::range_size(NewBB->getTrailingDbgRecords()->getDbgRecordRange()), + 1); + + // Drop the trailing records now, to prevent a cleanup assertion. + NewBB->deleteTrailingDbgRecords(); +} + } // End anonymous namespace. From e0f95a0aa1226e85b42a37b85b7a8d4e29bb156a Mon Sep 17 00:00:00 2001 From: Orlando Cazalet-Hyams Date: Tue, 27 Aug 2024 13:26:24 +0100 Subject: [PATCH 2/2] remove early return, rename unittest & fix warnings --- llvm/unittests/IR/BasicBlockDbgInfoTest.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/llvm/unittests/IR/BasicBlockDbgInfoTest.cpp b/llvm/unittests/IR/BasicBlockDbgInfoTest.cpp index fc95f1838c307..e6f8df2762e81 100644 --- a/llvm/unittests/IR/BasicBlockDbgInfoTest.cpp +++ b/llvm/unittests/IR/BasicBlockDbgInfoTest.cpp @@ -1525,8 +1525,7 @@ TEST(BasicBlockDbgInfoTest, DbgMoveToEnd) { EXPECT_FALSE(Ret->hasDbgRecords()); } -TEST(BasicBlockDbgInfoTest, DbgKnownSentinelCrash) { - return; +TEST(BasicBlockDbgInfoTest, CloneTrailingRecordsToEmptyBlock) { LLVMContext C; std::unique_ptr M = parseIR(C, R"( define i16 @foo(i16 %a) !dbg !6 { @@ -1570,10 +1569,11 @@ TEST(BasicBlockDbgInfoTest, DbgKnownSentinelCrash) { // The trailing records should've been absorbed into NewBB. EXPECT_FALSE(BB.getTrailingDbgRecords()); EXPECT_TRUE(NewBB->getTrailingDbgRecords()); - if (NewBB->getTrailingDbgRecords()) + if (NewBB->getTrailingDbgRecords()) { EXPECT_EQ( llvm::range_size(NewBB->getTrailingDbgRecords()->getDbgRecordRange()), - 1); + 1u); + } // Drop the trailing records now, to prevent a cleanup assertion. NewBB->deleteTrailingDbgRecords();