[DebugInfo][RemoveDIs] Convert debug-info modes when loading bitcode

[jmorse]

As part of eliminating debug-intrinsics in LLVM, we'll shortly be pushing the conversion from "old" dbg.value mode to "new" DPValue mode out from when the pass manager runs, to when modules are loaded. This patch adds that conversion process and some (temporary) options to llvm-lto{,2} to help test it.

Specifically: now whenever we load a bitcode module, consider a flag of whether to "upgrade" it into the new debug-info mode, and if we're lazily materializing functions then do that lazily too. Doing this exposes an error in the IRLinker/materializer handling of DPValues, where we need to transfer the debug-info format flag correctly, and in ValueMapper we need to remap the Values that DPValues point at.

I've added some test coverage in the modified tests; these will be exercised by our llvm-new-debug-iterators buildbot.

This upgrading of debug-info won't be happening for the llvm18 release, instead we'll turn it on after the branch date, thenbe push the boundary of where "new" debug-info starts and ends down into the existing debug-info upgrade path over the course of the next release.

[llvmbot]

@llvm/pr-subscribers-lto
@llvm/pr-subscribers-llvm-ir
@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-debuginfo

Author: Jeremy Morse (jmorse)

Changes

As part of eliminating debug-intrinsics in LLVM, we'll shortly be pushing the conversion from "old" dbg.value mode to "new" DPValue mode out from when the pass manager runs, to when modules are loaded. This patch adds that conversion process and some (temporary) options to llvm-lto{,2} to help test it.

Specifically: now whenever we load a bitcode module, consider a flag of whether to "upgrade" it into the new debug-info mode, and if we're lazily materializing functions then do that lazily too. Doing this exposes an error in the IRLinker/materializer handling of DPValues, where we need to transfer the debug-info format flag correctly, and in ValueMapper we need to remap the Values that DPValues point at.

I've added some test coverage in the modified tests; these will be exercised by our llvm-new-debug-iterators buildbot.

This upgrading of debug-info won't be happening for the llvm18 release, instead we'll turn it on after the branch date, thenbe push the boundary of where "new" debug-info starts and ends down into the existing debug-info upgrade path over the course of the next release.

---

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

12 Files Affected:

• (modified) llvm/lib/Bitcode/Reader/BitcodeReader.cpp (+23)
• (modified) llvm/lib/IR/LegacyPassManager.cpp (+4-2)
• (modified) llvm/lib/Linker/IRMover.cpp (+1)
• (modified) llvm/lib/Transforms/Utils/ValueMapper.cpp (+9-2)
• (modified) llvm/test/LTO/X86/pr38046.ll (+8)
• (modified) llvm/test/Linker/debug-info-use-before-def.ll (+1)
• (modified) llvm/test/Linker/thinlto_funcimport_debug.ll (+4)
• (modified) llvm/test/ThinLTO/X86/debuginfo-cu-import.ll (+4)
• (modified) llvm/test/ThinLTO/X86/pr35472.ll (+6)
• (modified) llvm/tools/llvm-link/llvm-link.cpp (+18)
• (modified) llvm/tools/llvm-lto/llvm-lto.cpp (+18)
• (modified) llvm/tools/llvm-lto2/llvm-lto2.cpp (+18)

diff --git a/llvm/lib/Bitcode/Reader/BitcodeReader.cpp b/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
index a027d0c21ba0bb..5b233fb365fe27 100644
--- a/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
+++ b/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
@@ -100,6 +100,9 @@ static cl::opt<bool> ExpandConstantExprs(
     cl::desc(
         "Expand constant expressions to instructions for testing purposes"));
 
+// Declare external flag for whether we're using the new debug-info format.
+extern llvm::cl::opt<bool> UseNewDbgInfoFormat;
+
 namespace {
 
 enum {
@@ -6629,6 +6632,9 @@ Error BitcodeReader::materialize(GlobalValue *GV) {
   if (Error Err = materializeMetadata())
     return Err;
 
+  bool NewDebugInfoRequested = F->IsNewDbgInfoFormat;
+  F->IsNewDbgInfoFormat = false;
+
   // Move the bit stream to the saved position of the deferred function body.
   if (Error JumpFailed = Stream.JumpToBit(DFII->second))
     return JumpFailed;
@@ -6704,6 +6710,14 @@ Error BitcodeReader::materialize(GlobalValue *GV) {
   // Look for functions that rely on old function attribute behavior.
   UpgradeFunctionAttributes(*F);
 
+  // If we've materialized a function set up in "new" debug-info mode, the
+  // contents just loaded will still be in dbg.value mode. Switch to the new
+  // mode now. NB: we can add more complicated logic here in the future to
+  // correctly identify when we do and don't need to autoupgrade.
+  if (NewDebugInfoRequested) {
+    F->convertToNewDbgValues();
+  }
+
   // Bring in any functions that this function forward-referenced via
   // blockaddresses.
   return materializeForwardReferencedFunctions();
@@ -8027,6 +8041,15 @@ BitcodeModule::getModuleImpl(LLVMContext &Context, bool MaterializeAll,
     if (Error Err = R->materializeForwardReferencedFunctions())
       return std::move(Err);
   }
+
+  // If we are operating in a "new debug-info" context, upgrade the debug-info
+  // in the loaded module. This is a transitional approach as we enable "new"
+  // debug-info in LLVM, which will eventually be pushed down into the
+  // autoupgrade path once the bitcode-encoding is finalised. Non-materialised
+  // functions will be upgraded in the materialize method.
+  if (UseNewDbgInfoFormat && !M->IsNewDbgInfoFormat)
+    M->convertToNewDbgValues();
+
   return std::move(M);
 }
 
diff --git a/llvm/lib/IR/LegacyPassManager.cpp b/llvm/lib/IR/LegacyPassManager.cpp
index dac4fbce17e40d..8945c6dbc8d848 100644
--- a/llvm/lib/IR/LegacyPassManager.cpp
+++ b/llvm/lib/IR/LegacyPassManager.cpp
@@ -530,7 +530,8 @@ bool PassManagerImpl::run(Module &M) {
 
   // RemoveDIs: if a command line flag is given, convert to the DPValue
   // representation of debug-info for the duration of these passes.
-  if (UseNewDbgInfoFormat)
+  bool shouldConvertDbgInfo = UseNewDbgInfoFormat && !M.IsNewDbgInfoFormat;
+  if (shouldConvertDbgInfo)
     M.convertToNewDbgValues();
 
   for (ImmutablePass *ImPass : getImmutablePasses())
@@ -545,7 +546,8 @@ bool PassManagerImpl::run(Module &M) {
   for (ImmutablePass *ImPass : getImmutablePasses())
     Changed |= ImPass->doFinalization(M);
 
-  M.convertFromNewDbgValues();
+  if (shouldConvertDbgInfo)
+    M.convertFromNewDbgValues();
 
   return Changed;
 }
diff --git a/llvm/lib/Linker/IRMover.cpp b/llvm/lib/Linker/IRMover.cpp
index 1bd562d1e8ae2b..8cc0f7fb909916 100644
--- a/llvm/lib/Linker/IRMover.cpp
+++ b/llvm/lib/Linker/IRMover.cpp
@@ -694,6 +694,7 @@ Function *IRLinker::copyFunctionProto(const Function *SF) {
                              SF->getAddressSpace(), SF->getName(), &DstM);
   F->copyAttributesFrom(SF);
   F->setAttributes(mapAttributeTypes(F->getContext(), F->getAttributes()));
+  F->IsNewDbgInfoFormat = SF->IsNewDbgInfoFormat;
   return F;
 }
 
diff --git a/llvm/lib/Transforms/Utils/ValueMapper.cpp b/llvm/lib/Transforms/Utils/ValueMapper.cpp
index 71d0f09e47713b..5cbae0e27c1408 100644
--- a/llvm/lib/Transforms/Utils/ValueMapper.cpp
+++ b/llvm/lib/Transforms/Utils/ValueMapper.cpp
@@ -1048,9 +1048,16 @@ void Mapper::remapFunction(Function &F) {
       A.mutateType(TypeMapper->remapType(A.getType()));
 
   // Remap the instructions.
-  for (BasicBlock &BB : F)
-    for (Instruction &I : BB)
+  for (BasicBlock &BB : F) {
+    for (Instruction &I : BB) {
       remapInstruction(&I);
+      if (I.DbgMarker) {
+        for (DPValue &DPV : I.DbgMarker->getDbgValueRange()) {
+          remapDPValue(DPV);
+        }
+      }
+    }
+  }
 }
 
 void Mapper::mapAppendingVariable(GlobalVariable &GV, Constant *InitPrefix,
diff --git a/llvm/test/LTO/X86/pr38046.ll b/llvm/test/LTO/X86/pr38046.ll
index 8f04190f0c83f6..ddcc0f42cf231b 100644
--- a/llvm/test/LTO/X86/pr38046.ll
+++ b/llvm/test/LTO/X86/pr38046.ll
@@ -5,6 +5,14 @@
 ; RUN: llvm-dis %t.lto.o.0.2.internalize.bc >/dev/null 2>%t.dis.stderr || true
 ; RUN: FileCheck -allow-empty %s < %t.dis.stderr
 
+;; Re-run with "new" debug-info mode to ensure the variable location information
+;; is handled gracefully.
+; RUN: llvm-lto2 run -save-temps -o %t.lto.o %t.o \
+; RUN:   -r=%t.o,foo,plx \
+; RUN:   -r=%t.o,get,pl --try-experimental-debuginfo-iterators
+; RUN: llvm-dis %t.lto.o.0.2.internalize.bc >/dev/null 2>%t.dis.stderr || true
+; RUN: FileCheck -allow-empty %s < %t.dis.stderr
+
 ; CHECK-NOT: Global is external, but doesn't have external or weak linkage
 
 target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
diff --git a/llvm/test/Linker/debug-info-use-before-def.ll b/llvm/test/Linker/debug-info-use-before-def.ll
index ac5f3148f83c7e..d32dd97f4ffafe 100644
--- a/llvm/test/Linker/debug-info-use-before-def.ll
+++ b/llvm/test/Linker/debug-info-use-before-def.ll
@@ -1,4 +1,5 @@
 ; RUN: llvm-link -S %s | FileCheck %s
+; RUN: llvm-link -S %s --try-experimental-debuginfo-iterators | FileCheck %s
 
 ; Test that when a debug metadata use-before-def is run through llvm-link, the
 ; value reference is preserved. Tests both singular uses and DIArgList uses of
diff --git a/llvm/test/Linker/thinlto_funcimport_debug.ll b/llvm/test/Linker/thinlto_funcimport_debug.ll
index 294b3a773ef512..5a69fc6492b244 100644
--- a/llvm/test/Linker/thinlto_funcimport_debug.ll
+++ b/llvm/test/Linker/thinlto_funcimport_debug.ll
@@ -6,6 +6,10 @@
 ; If we import func1 and not func2 we should only link DISubprogram for func1
 ; RUN: llvm-link %t2.bc -summary-index=%t3.thinlto.bc -import=func1:%t.bc -S | FileCheck %s
 
+;; Repeat runlines using "new" debuginfo iterators mode.
+; RUN: llvm-lto -thinlto -o %t3 %t.bc %t2.bc --try-experimental-debuginfo-iterators
+; RUN: llvm-link %t2.bc -summary-index=%t3.thinlto.bc -import=func1:%t.bc -S --try-experimental-debuginfo-iterators | FileCheck %s
+
 ; CHECK: declare i32 @func2
 ; CHECK: define available_externally i32 @func1
 
diff --git a/llvm/test/ThinLTO/X86/debuginfo-cu-import.ll b/llvm/test/ThinLTO/X86/debuginfo-cu-import.ll
index 850ef09b1c2f0d..56269f8f84854b 100644
--- a/llvm/test/ThinLTO/X86/debuginfo-cu-import.ll
+++ b/llvm/test/ThinLTO/X86/debuginfo-cu-import.ll
@@ -13,6 +13,10 @@
 ; CHECK-NOT: DICompileUnit{{.*}} globals:
 ; CHECK-NOT: DICompileUnit{{.*}} imports:
 
+;; Repeat test in RemoveDIs debug-info mode to check that bitcode is loaded and
+;; converted correctly.
+; RUN: llvm-lto -thinlto-action=import %t2.bc -thinlto-index=%t.index.bc -o - --try-experimental-debuginfo-iterators | llvm-dis -o - | FileCheck %s
+
 ; ModuleID = 'debuginfo-cu-import.c'
 source_filename = "debuginfo-cu-import.c"
 target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
diff --git a/llvm/test/ThinLTO/X86/pr35472.ll b/llvm/test/ThinLTO/X86/pr35472.ll
index f7b19e8ba79690..f214c607f3d286 100644
--- a/llvm/test/ThinLTO/X86/pr35472.ll
+++ b/llvm/test/ThinLTO/X86/pr35472.ll
@@ -9,6 +9,12 @@
 ; RUN: llvm-nm %t1.bc.thinlto.o | FileCheck %s -check-prefix=ThinLTOa
 ; RUN: llvm-nm %t2.bc.thinlto.o | FileCheck %s -check-prefix=ThinLTOb
 
+;; Re-run with "new" debug-info mode, checking that we load / convert / emit
+;; the dbg.declares below correctly.
+; RUN: llvm-lto -thinlto-action=run %t1.bc %t2.bc -exported-symbol=_Z5Alphav --try-experimental-debuginfo-iterators
+; RUN: llvm-nm %t1.bc.thinlto.o | FileCheck %s -check-prefix=ThinLTOa
+; RUN: llvm-nm %t2.bc.thinlto.o | FileCheck %s -check-prefix=ThinLTOb
+
 ; ThinLTOa-DAG: T _Z5Bravov
 ; ThinLTOa-DAG: W _ZN4EchoD2Ev
 ; ThinLTOb-DAG: T _Z5Alphav
diff --git a/llvm/tools/llvm-link/llvm-link.cpp b/llvm/tools/llvm-link/llvm-link.cpp
index a476b50a1ed90b..a63ccb42e6e990 100644
--- a/llvm/tools/llvm-link/llvm-link.cpp
+++ b/llvm/tools/llvm-link/llvm-link.cpp
@@ -129,6 +129,13 @@ static cl::opt<bool> IgnoreNonBitcode(
     cl::desc("Do not report an error for non-bitcode files in archives"),
     cl::Hidden);
 
+static cl::opt<bool> TryUseNewDbgInfoFormat(
+      "try-experimental-debuginfo-iterators",
+      cl::desc("Enable debuginfo iterator positions, if they're built in"),
+      cl::init(false));
+
+extern cl::opt<bool> UseNewDbgInfoFormat;
+
 static ExitOnError ExitOnErr;
 
 // Read the specified bitcode file in and return it. This routine searches the
@@ -465,6 +472,17 @@ int main(int argc, char **argv) {
   cl::HideUnrelatedOptions({&LinkCategory, &getColorCategory()});
   cl::ParseCommandLineOptions(argc, argv, "llvm linker\n");
 
+  // RemoveDIs debug-info transition: tests may request that we /try/ to use the
+  // new debug-info format, if it's built in.
+#ifdef EXPERIMENTAL_DEBUGINFO_ITERATORS
+  if (TryUseNewDbgInfoFormat) {
+    // If LLVM was built with support for this, turn the new debug-info format
+    // on.
+    UseNewDbgInfoFormat = true;
+  }
+#endif
+  (void)TryUseNewDbgInfoFormat;
+
   LLVMContext Context;
   Context.setDiagnosticHandler(std::make_unique<LLVMLinkDiagnosticHandler>(),
                                true);
diff --git a/llvm/tools/llvm-lto/llvm-lto.cpp b/llvm/tools/llvm-lto/llvm-lto.cpp
index 735b3763f5b2fa..5bec24c8c8f189 100644
--- a/llvm/tools/llvm-lto/llvm-lto.cpp
+++ b/llvm/tools/llvm-lto/llvm-lto.cpp
@@ -264,6 +264,13 @@ static cl::opt<bool>
     LTOSaveBeforeOpt("lto-save-before-opt", cl::init(false),
                      cl::desc("Save the IR before running optimizations"));
 
+static cl::opt<bool> TryUseNewDbgInfoFormat(
+      "try-experimental-debuginfo-iterators",
+      cl::desc("Enable debuginfo iterator positions, if they're built in"),
+      cl::init(false), cl::Hidden);
+
+extern cl::opt<bool> UseNewDbgInfoFormat;
+
 namespace {
 
 struct ModuleInfo {
@@ -937,6 +944,17 @@ int main(int argc, char **argv) {
   cl::HideUnrelatedOptions({&LTOCategory, &getColorCategory()});
   cl::ParseCommandLineOptions(argc, argv, "llvm LTO linker\n");
 
+  // RemoveDIs debug-info transition: tests may request that we /try/ to use the
+  // new debug-info format, if it's built in.
+#ifdef EXPERIMENTAL_DEBUGINFO_ITERATORS
+  if (TryUseNewDbgInfoFormat) {
+    // If LLVM was built with support for this, turn the new debug-info format
+    // on.
+    UseNewDbgInfoFormat = true;
+  }
+#endif
+  (void)TryUseNewDbgInfoFormat;
+
   if (OptLevel < '0' || OptLevel > '3')
     error("optimization level must be between 0 and 3");
 
diff --git a/llvm/tools/llvm-lto2/llvm-lto2.cpp b/llvm/tools/llvm-lto2/llvm-lto2.cpp
index 81c97a994038d9..3301fb87b22f99 100644
--- a/llvm/tools/llvm-lto2/llvm-lto2.cpp
+++ b/llvm/tools/llvm-lto2/llvm-lto2.cpp
@@ -187,6 +187,13 @@ static cl::opt<bool> EnableFreestanding(
     cl::desc("Enable Freestanding (disable builtins / TLI) during LTO"),
     cl::Hidden);
 
+static cl::opt<bool> TryUseNewDbgInfoFormat(
+      "try-experimental-debuginfo-iterators",
+      cl::desc("Enable debuginfo iterator positions, if they're built in"),
+      cl::init(false), cl::Hidden);
+
+extern cl::opt<bool> UseNewDbgInfoFormat;
+
 static void check(Error E, std::string Msg) {
   if (!E)
     return;
@@ -222,6 +229,17 @@ static int usage() {
 static int run(int argc, char **argv) {
   cl::ParseCommandLineOptions(argc, argv, "Resolution-based LTO test harness");
 
+  // RemoveDIs debug-info transition: tests may request that we /try/ to use the
+  // new debug-info format, if it's built in.
+#ifdef EXPERIMENTAL_DEBUGINFO_ITERATORS
+  if (TryUseNewDbgInfoFormat) {
+    // If LLVM was built with support for this, turn the new debug-info format
+    // on.
+    UseNewDbgInfoFormat = true;
+  }
+#endif
+  (void)TryUseNewDbgInfoFormat;
+
   // FIXME: Workaround PR30396 which means that a symbol can appear
   // more than once if it is defined in module-level assembly and
   // has a GV declaration. We allow (file, symbol) pairs to have multiple

[github-actions]

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

[SLTozer]

LGTM, with one small suggestion. Ideally we'll soon put a similar conversion step in the IR parser, so that we don't have to do any conversion in the pass managers.

[SLTozer]

Suggested change

	if (I.DbgMarker) {
	for (DPValue &DPV : I.DbgMarker->getDbgValueRange()) {
	remapDPValue(DPV);
	}
	}
	for (DPValue &DPV : I.getDbgValueRange())
	remapDPValue(DPV);

[jplehr]

Hi @jmorse I believe this patch broke the AMDGPU OpenMP Offload buildbots
https://lab.llvm.org/buildbot/#/builders/193/builds/45566
https://lab.llvm.org/staging/#/builders/185/builds/843

If you need help in reproducing, I'm happy to try and help.

[jmorse]

Curses, looks like I killed most of the buildbots. Reverted in c3f7fb1

[nico]

Also broke check-clang when the AMDGPU target was enabled fwiw: http://45.33.8.238/linux/129084/step_7.txt

Thanks for the revert!

[jmorse]

/me squints -- alright, this was because I was using I.DbgMarker->getDbgValueRange() rather than I.getDbgValueRange(), and I wasn't testing with a clean tree. I'd reapply with the obvious fix, but I'm also out of the figurative office for the rest of the day, so can't revert if it kills all the buildbots again.