[DebugInfo][RemoveDIs] Cope with instructions moving after themselves

[jmorse]

We occasionally move instructions to their own positions, which is not an error, and has no effect on the program. However, if dbg.value intrinsics are present then they can effectively be moved without any other effect on the program.

This is a problem if we're using non-instruction debug-info: a moveAfter that appears to be a no-op in RemoveDIs mode can jump in front of intrinsics in dbg.value mode. Thus: if an instruction is moved to itself and the head bit is set, force attached debug-info to shift down one instruction, which replicates the dbg.value behaviour.

[llvmbot]

@llvm/pr-subscribers-llvm-ir

@llvm/pr-subscribers-debuginfo

Author: Jeremy Morse (jmorse)

Changes

We occasionally move instructions to their own positions, which is not an error, and has no effect on the program. However, if dbg.value intrinsics are present then they can effectively be moved without any other effect on the program.

This is a problem if we're using non-instruction debug-info: a moveAfter that appears to be a no-op in RemoveDIs mode can jump in front of intrinsics in dbg.value mode. Thus: if an instruction is moved to itself and the head bit is set, force attached debug-info to shift down one instruction, which replicates the dbg.value behaviour.

---

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

2 Files Affected:

• (modified) llvm/lib/IR/Instruction.cpp (+3-2)
• (modified) llvm/unittests/IR/BasicBlockDbgInfoTest.cpp (+73)

diff --git a/llvm/lib/IR/Instruction.cpp b/llvm/lib/IR/Instruction.cpp
index 4b5349856b8d7bf..7144924c34570be 100644
--- a/llvm/lib/IR/Instruction.cpp
+++ b/llvm/lib/IR/Instruction.cpp
@@ -200,8 +200,9 @@ void Instruction::moveBeforeImpl(BasicBlock &BB, InstListType::iterator I,
   // If we've been given the "Preserve" flag, then just move the DPValues with
   // the instruction, no more special handling needed.
   if (BB.IsNewDbgInfoFormat && DbgMarker && !Preserve) {
-    if (I != this->getIterator()) {
-      // "this" is definitely moving; detach any existing DPValues.
+    if (I != this->getIterator() || InsertAtHead) {
+      // "this" is definitely moving in the list, or it's moving ahead of it's
+      // attached DPValues. Detach any existing DPValues.
       handleMarkerRemoval();
     }
   }
diff --git a/llvm/unittests/IR/BasicBlockDbgInfoTest.cpp b/llvm/unittests/IR/BasicBlockDbgInfoTest.cpp
index a8ca706cc95b9b5..c914a5b6f239416 100644
--- a/llvm/unittests/IR/BasicBlockDbgInfoTest.cpp
+++ b/llvm/unittests/IR/BasicBlockDbgInfoTest.cpp
@@ -42,6 +42,79 @@ static std::unique_ptr<Module> parseIR(LLVMContext &C, const char *IR) {
 
 namespace {
 
+// We can occasionally moveAfter an instruction so that it moves to the
+// position that it already resides at. This is fine -- but gets complicated
+// with dbg.value intrinsics. By moving an instruction, we can end up changing
+// nothing but the location of debug-info intrinsics. That has to be modelled
+// by DPValues, the dbg.value replacement.
+TEST(BasicBlockDbgInfoTest, InsertAfterSelf) {
+  LLVMContext C;
+  UseNewDbgInfoFormat = true;
+
+  std::unique_ptr<Module> M = parseIR(C, R"(
+    define i16 @f(i16 %a) !dbg !6 {
+      call void @llvm.dbg.value(metadata i16 %a, metadata !9, metadata !DIExpression()), !dbg !11
+      %b = add i16 %a, 1, !dbg !11
+      call void @llvm.dbg.value(metadata i16 %b, metadata !9, metadata !DIExpression()), !dbg !11
+      %c = add i16 %b, 1, !dbg !11
+      ret i16 0, !dbg !11
+    }
+    declare void @llvm.dbg.value(metadata, metadata, metadata) #0
+    attributes #0 = { nounwind readnone speculatable willreturn }
+
+    !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)
+)");
+
+  // Convert the module to "new" form debug-info.
+  M->convertToNewDbgValues();
+  // Fetch the entry block,
+  BasicBlock &BB = M->getFunction("f")->getEntryBlock();
+
+  Instruction *Inst1 = &*BB.begin();
+  Instruction *Inst2 = &*std::next(BB.begin());
+  Instruction *RetInst = &*std::next(Inst2->getIterator());
+  EXPECT_TRUE(Inst1->hasDbgValues());
+  EXPECT_TRUE(Inst2->hasDbgValues());
+  EXPECT_FALSE(RetInst->hasDbgValues());
+
+  // If we move Inst2 to be after Inst1, then it comes _immediately_ after. Were
+  // we in dbg.value form we would then have:
+  //    dbg.value
+  //    %b = add
+  //    %c = add
+  //    dbg.value
+  // Check that this is replicated by DPValues.
+  Inst2->moveAfter(Inst1);
+
+  // Inst1 should only have one DPValue on it.
+  EXPECT_TRUE(Inst1->hasDbgValues());
+  auto Range1 = Inst1->getDbgValueRange();
+  EXPECT_EQ(std::distance(Range1.begin(), Range1.end()), 1u);
+  // Inst2 should have none.
+  EXPECT_FALSE(Inst2->hasDbgValues());
+  // While the return inst should now have one on it.
+  EXPECT_TRUE(RetInst->hasDbgValues());
+  auto Range2 = RetInst->getDbgValueRange();
+  EXPECT_EQ(std::distance(Range2.begin(), Range2.end()), 1u);
+
+  M->convertFromNewDbgValues();
+
+  UseNewDbgInfoFormat = false;
+}
+
+
 TEST(BasicBlockDbgInfoTest, MarkerOperations) {
   LLVMContext C;
   UseNewDbgInfoFormat = true;

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[OCHyams]

LGTM, just two nits

[OCHyams]

nit "...head of its attached..."

Is this something we want to change in the future, or is there a legitimate use case for this debug shuffling behaviour? If this is purely to match existing weird behaviour then I think it deserves a FIXME.

[jmorse]

Eeeshh, that's a big question, as it leads into the greater question of "does the movement of instructions like this actually need to affect debug-info in any way". If an instruction is moving like this, it probably shouldn't have a line number attached, and so there should be no visible effect to the developer of these debug records moving or not. That then naturally moves onto the question of... shouldn't the debug records be linked in some way to the line numbers that they implicitly cover?

Something we can answer in the future, for now, I think it's not-a-bug to precisely match what happens in dbg.value mode.

[OCHyams]

nit: dangling ,