[DebugInfo][RemoveDIs] Set new-dbg-info flag from Modules correctly

[jmorse]

It turns out there's a pathway for Functions to be inserted into modules without having the "New" debug-info flag set correctly, which this patch fixes. Sadly there isn't a Module::insert method to instrument out there, everyone touches the list directly.

This fix exposes a path where such functions are produced in the outliner in the wrong mode; requiring a fix there to correctly drop RemoveDIs-mode debug-info. This is covered by test/DebugInfo/AArch64/ir-outliner.ll

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-llvm-ir

Author: Jeremy Morse (jmorse)

Changes

It turns out there's a pathway for Functions to be inserted into modules without having the "New" debug-info flag set correctly, which this patch fixes. Sadly there isn't a Module::insert method to instrument out there, everyone touches the list directly.

This fix exposes a path where such functions are produced in the outliner in the wrong mode; requiring a fix there to correctly drop RemoveDIs-mode debug-info. This is covered by test/DebugInfo/AArch64/ir-outliner.ll

---

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

2 Files Affected:

• (modified) llvm/lib/IR/Function.cpp (+3-1)
• (modified) llvm/lib/Transforms/IPO/IROutliner.cpp (+7-4)

diff --git a/llvm/lib/IR/Function.cpp b/llvm/lib/IR/Function.cpp
index fceffbc3cea6d7..056e4f31981a72 100644
--- a/llvm/lib/IR/Function.cpp
+++ b/llvm/lib/IR/Function.cpp
@@ -437,8 +437,10 @@ Function::Function(FunctionType *Ty, LinkageTypes Linkage, unsigned AddrSpace,
   if (Ty->getNumParams())
     setValueSubclassData(1);   // Set the "has lazy arguments" bit.
 
-  if (ParentModule)
+  if (ParentModule) {
     ParentModule->getFunctionList().push_back(this);
+    IsNewDbgInfoFormat = ParentModule->IsNewDbgInfoFormat;
+  }
 
   HasLLVMReservedName = getName().starts_with("llvm.");
   // Ensure intrinsics have the right parameter attributes.
diff --git a/llvm/lib/Transforms/IPO/IROutliner.cpp b/llvm/lib/Transforms/IPO/IROutliner.cpp
index 8e6d0e814372d6..48470bc71ff38a 100644
--- a/llvm/lib/Transforms/IPO/IROutliner.cpp
+++ b/llvm/lib/Transforms/IPO/IROutliner.cpp
@@ -721,6 +721,12 @@ static void moveFunctionData(Function &Old, Function &New,
     std::vector<Instruction *> DebugInsts;
 
     for (Instruction &Val : CurrBB) {
+      // Since debug-info originates from many different locations in the
+      // program, it will cause incorrect reporting from a debugger if we keep
+      // the same debug instructions. Drop non-intrinsic DPValues here,
+      // collect intrinsics for removal later.
+      Val.dropDbgValues();
+
       // We must handle the scoping of called functions differently than
       // other outlined instructions.
       if (!isa<CallInst>(&Val)) {
@@ -744,10 +750,7 @@ static void moveFunctionData(Function &Old, Function &New,
       // From this point we are only handling call instructions.
       CallInst *CI = cast<CallInst>(&Val);
 
-      // We add any debug statements here, to be removed after.  Since the
-      // instructions originate from many different locations in the program,
-      // it will cause incorrect reporting from a debugger if we keep the
-      // same debug instructions.
+      // Collect debug intrinsics for later removal.
       if (isa<DbgInfoIntrinsic>(CI)) {
         DebugInsts.push_back(&Val);
         continue;

[jmorse]

(Possibly to state the obvious: while the flag for what flavour of debug-info we're carrying has saved our necks in the past, the fact that it's masking coverage here is unhelpful, and cause for getting rid of it earlier).

[OCHyams]

LGTM👍

[jayfoad]

Thanks for the patch!

It turns out there's a pathway for Functions to be inserted into modules without having the "New" debug-info flag set correctly

Is there any other path that does set it correctly?

[jmorse]

Is there any other path that does set it correctly?

Sniffing around this, I get the impression that we failed to spot what was going on here and instead scattered explicit setting of the flag everywhere. Probably not our finest hour ._. , but something we can clean-up now.

EDIT: (because every time I copy+paste something and hit enter, github submits it...)