release/19.x: [RemoveDIs] Fix spliceDebugInfo splice-to-end edge case (#105671, #106723) #106952

OCHyams · 2024-09-02T09:11:51Z

Please can we backport 43661a1 (and 7ffe67c) into the next dot release.

Replaces #106691 - this one includes a follow-up fix in 7ffe67c (#106723). I couldn't create two separate requests because it creates conflicts.

llvmbot · 2024-09-02T09:12:28Z

@llvm/pr-subscribers-llvm-ir

Author: Orlando Cazalet-Hyams (OCHyams)

Changes

Please can we backport 43661a1 (and 7ffe67c) into the next dot release.

Replaces #106691 - this one includes a follow-up fix in 7ffe67c. I couldn't create two separate requests because it creates conflicts.

Full diff: https://github.com./llvm/llvm-project/pull/106952.diff

2 Files Affected:

(modified) llvm/lib/IR/BasicBlock.cpp (+10-2)
(modified) llvm/unittests/IR/BasicBlockDbgInfoTest.cpp (+52)

diff --git a/llvm/lib/IR/BasicBlock.cpp b/llvm/lib/IR/BasicBlock.cpp
index 0a9498f051cb59..46896d3cdf7d50 100644
--- a/llvm/lib/IR/BasicBlock.cpp
+++ b/llvm/lib/IR/BasicBlock.cpp
@@ -975,8 +975,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 835780e63aaf4f..5ce14d3f6b9cef 100644
--- a/llvm/unittests/IR/BasicBlockDbgInfoTest.cpp
+++ b/llvm/unittests/IR/BasicBlockDbgInfoTest.cpp
@@ -1525,4 +1525,56 @@ TEST(BasicBlockDbgInfoTest, DbgMoveToEnd) {
   EXPECT_FALSE(Ret->hasDbgRecords());
 }
 
+TEST(BasicBlockDbgInfoTest, CloneTrailingRecordsToEmptyBlock) {
+  LLVMContext C;
+  std::unique_ptr<Module> 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 (DbgMarker *Trailing = NewBB->getTrailingDbgRecords()) {
+    EXPECT_EQ(llvm::range_size(Trailing->getDbgRecordRange()), 1u);
+    // Drop the trailing records now, to prevent a cleanup assertion.
+    Trailing->eraseFromParent();
+    NewBB->deleteTrailingDbgRecords();
+  }
+}
+
 } // End anonymous namespace.

jmorse

As with the previous rev, LGTM as a fix for an edge case. This situation has taken many months to expose, so I don't think it urgently needs to go into the imminent release, a point release should be fine.

tru · 2024-09-10T06:17:57Z

Hi, since we are wrapping up LLVM 19.1.0 we are very strict with the fixes we pick at this point. Can you please respond to the following questions to help me understand if this has to be included in the final release or not.

Is this PR a fix for a regression or a critical issue?

What is the risk of accepting this into the release branch?

What is the risk of NOT accepting this into the release branch?

OCHyams

Hi @tru,

Is this PR a fix for a regression or a critical issue?

~~Neither~~ EDIT: This is a regression from LLVM 18.

What is the risk of accepting this into the release branch?

If I've made a mistake in the patch we could get incorrect debug-info in an edge case. I've added some comments inline that explain why this patch won't introduce any new null or end() derefs. If somehow those comments are all wrong, this change would only cause an issue for the same edge case that already runs into one (see below).

What is the risk of NOT accepting this into the release branch?

There's a specific debug info circumstance edge case that results in an end() iterator dereference when this function is called. It took 6 months of usage for the edge case to materialise, so it's not common - the bug was reported in #105571.

Happy to provide more info if needed.

OCHyams · 2024-09-10T07:41:03Z

llvm/lib/IR/BasicBlock.cpp

-      // adoptDbgRecords will release any trailers.
+      if (Dest == end()) {
+        // Abosrb the trailing markers from Src.
+        assert(FromLast == Src->getTrailingDbgRecords());


FromLast can't be null because of the outer Src->getMarker(Last) check.

OCHyams · 2024-09-10T07:44:32Z

llvm/lib/IR/BasicBlock.cpp

+        assert(FromLast == Src->getTrailingDbgRecords());
+        createMarker(Dest)->absorbDebugValues(*FromLast, true);
+        FromLast->eraseFromParent();
+        Src->deleteTrailingDbgRecords();


Src is not null (it is dereferenced throughout function without null checks).

Here the trailing marker is deleted (eraseFromParent) and unlinked from its parent (deleteTrailingDbgRecords) - the order doesn't matter because deleteTrailingDbgRecords just removes the ptr from a map.

OCHyams · 2024-09-10T07:44:44Z

llvm/lib/IR/BasicBlock.cpp

+        Src->deleteTrailingDbgRecords();
+      } else {
+        // adoptDbgRecords will release any trailers.
+        Dest->adoptDbgRecords(Src, Last, true);


Same behaviour as before the patch in the else branch.

OCHyams · 2024-09-10T07:47:11Z

llvm/lib/IR/BasicBlock.cpp

+      if (Dest == end()) {
+        // Abosrb the trailing markers from Src.
+        assert(FromLast == Src->getTrailingDbgRecords());
+        createMarker(Dest)->absorbDebugValues(*FromLast, true);


createMarker gets an existing marker if it exists or creates a new one otherwise, the return value is never null. end() is a valid and expected input to this function.

OCHyams · 2024-09-10T09:05:30Z

@tru just a note, I got confused about the release timelines and I mistakenly said this wasn't a regression (it is a regression from LLVM 18 as it is in in LLVM 19 that we turned on the feature that triggers the issue this patch fixes). I've updated my initial reply.

tru · 2024-09-10T09:29:34Z

Thanks for your detailed answer! I think this can go in 19.1.0

Fix llvm#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. (cherry picked from commit 43661a1)

github-actions · 2024-09-10T14:40:23Z

@OCHyams (or anyone else). If you would like to add a note about this fix in the release notes (completely optional). Please reply to this comment with a one or two sentence description of the fix. When you are done, please add the release:note label to this PR.

OCHyams requested review from jmorse, SLTozer and tru September 2, 2024 09:11

llvmbot added the llvm:ir label Sep 2, 2024

OCHyams mentioned this pull request Sep 2, 2024

release/19.x: [RemoveDIs] Fix spliceDebugInfo splice-to-end edge case (#105671) #106691

Closed

jmorse approved these changes Sep 3, 2024

View reviewed changes

nikic added this to the LLVM 19.X Release milestone Sep 4, 2024

OCHyams commented Sep 10, 2024

View reviewed changes

tru force-pushed the backport-43661a121435 branch from a0dadeb to d930276 Compare September 10, 2024 14:38

tru force-pushed the backport-43661a121435 branch from d930276 to 11e2a15 Compare September 10, 2024 14:39

tru merged commit 11e2a15 into llvm:release/19.x Sep 10, 2024
9 of 10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

release/19.x: [RemoveDIs] Fix spliceDebugInfo splice-to-end edge case (#105671, #106723) #106952

release/19.x: [RemoveDIs] Fix spliceDebugInfo splice-to-end edge case (#105671, #106723) #106952

OCHyams commented Sep 2, 2024 •

edited

Loading

llvmbot commented Sep 2, 2024

jmorse left a comment

tru commented Sep 10, 2024

OCHyams left a comment •

edited

Loading

OCHyams Sep 10, 2024

OCHyams Sep 10, 2024

OCHyams Sep 10, 2024

OCHyams Sep 10, 2024

OCHyams commented Sep 10, 2024

tru commented Sep 10, 2024

github-actions bot commented Sep 10, 2024

release/19.x: [RemoveDIs] Fix spliceDebugInfo splice-to-end edge case (#105671, #106723) #106952

release/19.x: [RemoveDIs] Fix spliceDebugInfo splice-to-end edge case (#105671, #106723) #106952

Conversation

OCHyams commented Sep 2, 2024 • edited Loading

llvmbot commented Sep 2, 2024

jmorse left a comment

Choose a reason for hiding this comment

tru commented Sep 10, 2024

OCHyams left a comment • edited Loading

Choose a reason for hiding this comment

OCHyams Sep 10, 2024

Choose a reason for hiding this comment

OCHyams Sep 10, 2024

Choose a reason for hiding this comment

OCHyams Sep 10, 2024

Choose a reason for hiding this comment

OCHyams Sep 10, 2024

Choose a reason for hiding this comment

OCHyams commented Sep 10, 2024

tru commented Sep 10, 2024

github-actions bot commented Sep 10, 2024

OCHyams commented Sep 2, 2024 •

edited

Loading

OCHyams left a comment •

edited

Loading