[RemoveDIs] Load into new debug info format by default in LLVM

[SLTozer]

This patch enables parsing and creating modules directly into the new debug info format. Prior to this patch, all modules were constructed with the old debug info format by default, and would be converted into the new format just before running LLVM passes. This is an important milestone, in that this means that every tool will now be exposed to debug records, rather than those that run LLVM passes. As far as I've tested, all LLVM tools/projects now either handle debug records, or convert them to the old intrinsic format.

There are a few unit tests that need updating for this patch; these are either cases of tests that previously needed to set the debug info format to function, or tests that depend on the old debug info format in some way. There should be no visible change in the output of any LLVM tool as a result of this patch, although the likelihood of this patch breaking downstream code means an NFC tag might be a little misleading, if not technically incorrect:

This will probably break some downstream tools that don't already handle debug records. If your downstream code breaks as a result of this change, the simplest fix is to convert the module in question to the old debug format before you process it, using Module::convertFromNewDbgValues(). For more information about how to handle debug records or about what has changed, see the migration document:
https://llvm.org/docs/RemoveDIsDebugInfo.html

[llvmbot]

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

@llvm/pr-subscribers-debuginfo

Author: Stephen Tozer (SLTozer)

Changes

This patch enables parsing and creating modules directly into the new debug info format. Prior to this patch, all modules were constructed with the old debug info format by default, and would be converted into the new format just before running LLVM passes. This is an important milestone, in that this means that every tool will now be exposed to debug records, rather than those that run LLVM passes. As far as I've tested, all LLVM tools/projects now either handle debug records, or convert them to the old intrinsic format.

There are a few unit tests that need updating for this patch; these are either cases of tests that previously needed to set the debug info format to function, or tests that depend on the old debug info format in some way. There should be no visible change in the output of any LLVM tool as a result of this patch, although the likelihood of this patch breaking downstream code means an NFC tag might be a little misleading, if not technically incorrect:

This will probably break some downstream tools that don't already handle debug records. If your downstream code breaks as a result of this change, the simplest fix is to convert the module in question to the old debug format before you process it, using Module::convertFromNewDbgValues(). For more information about how to handle debug records or about what has changed, see the migration document:
https://llvm.org/docs/RemoveDIsDebugInfo.html

---

Patch is 25.57 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/89799.diff

15 Files Affected:

• (modified) llvm/include/llvm/AsmParser/LLParser.h (-1)
• (modified) llvm/lib/AsmParser/LLParser.cpp (+16-18)
• (modified) llvm/lib/Bitcode/Reader/BitcodeReader.cpp (+1-1)
• (modified) llvm/lib/IR/BasicBlock.cpp (+1-1)
• (modified) llvm/lib/IR/Function.cpp (+3-1)
• (modified) llvm/lib/IR/Module.cpp (+3-1)
• (modified) llvm/tools/llvm-as/llvm-as.cpp (+3-4)
• (modified) llvm/tools/llvm-dis/llvm-dis.cpp (+1-1)
• (modified) llvm/tools/llvm-link/llvm-link.cpp (+1-7)
• (modified) llvm/unittests/Analysis/IRSimilarityIdentifierTest.cpp (+32)
• (modified) llvm/unittests/IR/BasicBlockDbgInfoTest.cpp (-17)
• (modified) llvm/unittests/IR/DebugInfoTest.cpp (+8-7)
• (modified) llvm/unittests/IR/InstructionsTest.cpp (+5)
• (modified) llvm/unittests/Transforms/Utils/CloningTest.cpp (+3-2)
• (modified) llvm/unittests/Transforms/Utils/LocalTest.cpp (+55)

diff --git a/llvm/include/llvm/AsmParser/LLParser.h b/llvm/include/llvm/AsmParser/LLParser.h
index b2dcdfad0a04b4..e687254f6c4c70 100644
--- a/llvm/include/llvm/AsmParser/LLParser.h
+++ b/llvm/include/llvm/AsmParser/LLParser.h
@@ -337,7 +337,6 @@ namespace llvm {
 
     // Top-Level Entities
     bool parseTopLevelEntities();
-    bool finalizeDebugInfoFormat(Module *M);
     void dropUnknownMetadataReferences();
     bool validateEndOfModule(bool UpgradeDebugInfo);
     bool validateEndOfIndex();
diff --git a/llvm/lib/AsmParser/LLParser.cpp b/llvm/lib/AsmParser/LLParser.cpp
index 2902bd9fe17c48..34053a5ca9c8e8 100644
--- a/llvm/lib/AsmParser/LLParser.cpp
+++ b/llvm/lib/AsmParser/LLParser.cpp
@@ -74,23 +74,6 @@ static std::string getTypeString(Type *T) {
   return Tmp.str();
 }
 
-// Whatever debug info format we parsed, we should convert to the expected debug
-// info format immediately afterwards.
-bool LLParser::finalizeDebugInfoFormat(Module *M) {
-  // We should have already returned an error if we observed both intrinsics and
-  // records in this IR.
-  assert(!(SeenNewDbgInfoFormat && SeenOldDbgInfoFormat) &&
-         "Mixed debug intrinsics/records seen without a parsing error?");
-  if (PreserveInputDbgFormat == cl::boolOrDefault::BOU_TRUE) {
-    UseNewDbgInfoFormat = SeenNewDbgInfoFormat;
-    WriteNewDbgInfoFormatToBitcode = SeenNewDbgInfoFormat;
-    WriteNewDbgInfoFormat = SeenNewDbgInfoFormat;
-  } else if (M) {
-    M->setIsNewDbgInfoFormat(false);
-  }
-  return false;
-}
-
 /// Run: module ::= toplevelentity*
 bool LLParser::Run(bool UpgradeDebugInfo,
                    DataLayoutCallbackTy DataLayoutCallback) {
@@ -108,7 +91,7 @@ bool LLParser::Run(bool UpgradeDebugInfo,
   }
 
   return parseTopLevelEntities() || validateEndOfModule(UpgradeDebugInfo) ||
-         validateEndOfIndex() || finalizeDebugInfoFormat(M);
+         validateEndOfIndex();
 }
 
 bool LLParser::parseStandaloneConstantValue(Constant *&C,
@@ -207,6 +190,18 @@ void LLParser::dropUnknownMetadataReferences() {
 bool LLParser::validateEndOfModule(bool UpgradeDebugInfo) {
   if (!M)
     return false;
+
+  // We should have already returned an error if we observed both intrinsics and
+  // records in this IR.
+  assert(!(SeenNewDbgInfoFormat && SeenOldDbgInfoFormat) &&
+         "Mixed debug intrinsics/records seen without a parsing error?");
+  if (PreserveInputDbgFormat == cl::boolOrDefault::BOU_TRUE) {
+    UseNewDbgInfoFormat = SeenNewDbgInfoFormat;
+    WriteNewDbgInfoFormatToBitcode = SeenNewDbgInfoFormat;
+    WriteNewDbgInfoFormat = SeenNewDbgInfoFormat;
+    M->setNewDbgInfoFormatFlag(SeenNewDbgInfoFormat);
+  }
+
   // Handle any function attribute group forward references.
   for (const auto &RAG : ForwardRefAttrGroups) {
     Value *V = RAG.first;
@@ -439,6 +434,9 @@ bool LLParser::validateEndOfModule(bool UpgradeDebugInfo) {
   UpgradeModuleFlags(*M);
   UpgradeSectionAttributes(*M);
 
+  if (PreserveInputDbgFormat != cl::boolOrDefault::BOU_TRUE)
+    M->setIsNewDbgInfoFormat(UseNewDbgInfoFormat);
+
   if (!Slots)
     return false;
   // Initialize the slot mapping.
diff --git a/llvm/lib/Bitcode/Reader/BitcodeReader.cpp b/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
index 0b7fcd88418894..73fe63b5b8f6f7 100644
--- a/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
+++ b/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
@@ -4319,7 +4319,7 @@ Error BitcodeReader::parseModule(uint64_t ResumeBit,
   if (PreserveInputDbgFormat != cl::boolOrDefault::BOU_TRUE) {
     TheModule->IsNewDbgInfoFormat =
         UseNewDbgInfoFormat &&
-        LoadBitcodeIntoNewDbgInfoFormat == cl::boolOrDefault::BOU_TRUE;
+        LoadBitcodeIntoNewDbgInfoFormat != cl::boolOrDefault::BOU_FALSE;
   }
 
   this->ValueTypeCallback = std::move(Callbacks.ValueType);
diff --git a/llvm/lib/IR/BasicBlock.cpp b/llvm/lib/IR/BasicBlock.cpp
index 6e62767c99e2a2..73595dbe280be8 100644
--- a/llvm/lib/IR/BasicBlock.cpp
+++ b/llvm/lib/IR/BasicBlock.cpp
@@ -181,7 +181,7 @@ template class llvm::SymbolTableListTraits<Instruction,
 BasicBlock::BasicBlock(LLVMContext &C, const Twine &Name, Function *NewParent,
                        BasicBlock *InsertBefore)
     : Value(Type::getLabelTy(C), Value::BasicBlockVal),
-      IsNewDbgInfoFormat(false), Parent(nullptr) {
+      IsNewDbgInfoFormat(UseNewDbgInfoFormat), Parent(nullptr) {
 
   if (NewParent)
     insertInto(NewParent, InsertBefore);
diff --git a/llvm/lib/IR/Function.cpp b/llvm/lib/IR/Function.cpp
index e66fe73425e863..823e4a70dd9b7e 100644
--- a/llvm/lib/IR/Function.cpp
+++ b/llvm/lib/IR/Function.cpp
@@ -83,6 +83,8 @@ static cl::opt<unsigned> NonGlobalValueMaxNameSize(
     "non-global-value-max-name-size", cl::Hidden, cl::init(1024),
     cl::desc("Maximum size for the name of non-global values."));
 
+extern cl::opt<bool> UseNewDbgInfoFormat;
+
 void Function::convertToNewDbgValues() {
   IsNewDbgInfoFormat = true;
   for (auto &BB : *this) {
@@ -438,7 +440,7 @@ Function::Function(FunctionType *Ty, LinkageTypes Linkage, unsigned AddrSpace,
     : GlobalObject(Ty, Value::FunctionVal,
                    OperandTraits<Function>::op_begin(this), 0, Linkage, name,
                    computeAddrSpace(AddrSpace, ParentModule)),
-      NumArgs(Ty->getNumParams()), IsNewDbgInfoFormat(false) {
+      NumArgs(Ty->getNumParams()), IsNewDbgInfoFormat(UseNewDbgInfoFormat) {
   assert(FunctionType::isValidReturnType(getReturnType()) &&
          "invalid return type");
   setGlobalObjectSubClassData(0);
diff --git a/llvm/lib/IR/Module.cpp b/llvm/lib/IR/Module.cpp
index a8696ed9e3ce5d..915fa5097383cc 100644
--- a/llvm/lib/IR/Module.cpp
+++ b/llvm/lib/IR/Module.cpp
@@ -54,6 +54,8 @@
 
 using namespace llvm;
 
+extern cl::opt<bool> UseNewDbgInfoFormat;
+
 //===----------------------------------------------------------------------===//
 // Methods to implement the globals and functions lists.
 //
@@ -72,7 +74,7 @@ template class llvm::SymbolTableListTraits<GlobalIFunc>;
 Module::Module(StringRef MID, LLVMContext &C)
     : Context(C), ValSymTab(std::make_unique<ValueSymbolTable>(-1)),
       ModuleID(std::string(MID)), SourceFileName(std::string(MID)), DL(""),
-      IsNewDbgInfoFormat(false) {
+      IsNewDbgInfoFormat(UseNewDbgInfoFormat) {
   Context.addModule(this);
 }
 
diff --git a/llvm/tools/llvm-as/llvm-as.cpp b/llvm/tools/llvm-as/llvm-as.cpp
index e48e3f4d22c123..0958e16c2197ac 100644
--- a/llvm/tools/llvm-as/llvm-as.cpp
+++ b/llvm/tools/llvm-as/llvm-as.cpp
@@ -142,11 +142,10 @@ int main(int argc, char **argv) {
   }
 
   // Convert to new debug format if requested.
-  assert(!M->IsNewDbgInfoFormat && "Unexpectedly in new debug mode");
-  if (UseNewDbgInfoFormat && WriteNewDbgInfoFormatToBitcode) {
-    M->convertToNewDbgValues();
+  M->setIsNewDbgInfoFormat(UseNewDbgInfoFormat &&
+                           WriteNewDbgInfoFormatToBitcode);
+  if (M->IsNewDbgInfoFormat)
     M->removeDebugIntrinsicDeclarations();
-  }
 
   std::unique_ptr<ModuleSummaryIndex> Index = std::move(ModuleAndIndex.Index);
 
diff --git a/llvm/tools/llvm-dis/llvm-dis.cpp b/llvm/tools/llvm-dis/llvm-dis.cpp
index fbbb5506e43e05..d28af85bc739eb 100644
--- a/llvm/tools/llvm-dis/llvm-dis.cpp
+++ b/llvm/tools/llvm-dis/llvm-dis.cpp
@@ -258,7 +258,7 @@ int main(int argc, char **argv) {
       // All that llvm-dis does is write the assembly to a file.
       if (!DontPrint) {
         if (M) {
-          ScopedDbgInfoFormatSetter FormatSetter(*M, WriteNewDbgInfoFormat);
+          M->setIsNewDbgInfoFormat(WriteNewDbgInfoFormat);
           if (WriteNewDbgInfoFormat)
             M->removeDebugIntrinsicDeclarations();
           M->print(Out->os(), Annotator.get(), PreserveAssemblyUseListOrder);
diff --git a/llvm/tools/llvm-link/llvm-link.cpp b/llvm/tools/llvm-link/llvm-link.cpp
index 7794f2d81ed064..b84469d1c757f8 100644
--- a/llvm/tools/llvm-link/llvm-link.cpp
+++ b/llvm/tools/llvm-link/llvm-link.cpp
@@ -489,12 +489,6 @@ int main(int argc, char **argv) {
   if (LoadBitcodeIntoNewDbgInfoFormat == cl::boolOrDefault::BOU_UNSET)
     LoadBitcodeIntoNewDbgInfoFormat = cl::boolOrDefault::BOU_TRUE;
 
-  // RemoveDIs debug-info transition: tests may request that we /try/ to use the
-  // new debug-info format.
-  if (TryUseNewDbgInfoFormat) {
-    // Turn the new debug-info format on.
-    UseNewDbgInfoFormat = true;
-  }
   // Since llvm-link collects multiple IR modules together, for simplicity's
   // sake we disable the "PreserveInputDbgFormat" flag to enforce a single
   // debug info format.
@@ -556,7 +550,7 @@ int main(int argc, char **argv) {
     SetFormat(WriteNewDbgInfoFormat);
     Composite->print(Out.os(), nullptr, PreserveAssemblyUseListOrder);
   } else if (Force || !CheckBitcodeOutputToConsole(Out.os())) {
-    SetFormat(WriteNewDbgInfoFormatToBitcode);
+    SetFormat(UseNewDbgInfoFormat && WriteNewDbgInfoFormatToBitcode);
     WriteBitcodeToFile(*Composite, Out.os(), PreserveBitcodeUseListOrder);
   }
 
diff --git a/llvm/unittests/Analysis/IRSimilarityIdentifierTest.cpp b/llvm/unittests/Analysis/IRSimilarityIdentifierTest.cpp
index f6a053792f8529..69ea590945668e 100644
--- a/llvm/unittests/Analysis/IRSimilarityIdentifierTest.cpp
+++ b/llvm/unittests/Analysis/IRSimilarityIdentifierTest.cpp
@@ -12,6 +12,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Analysis/IRSimilarityIdentifier.h"
+#include "llvm/ADT/ScopeExit.h"
 #include "llvm/AsmParser/Parser.h"
 #include "llvm/IR/LLVMContext.h"
 #include "llvm/IR/Module.h"
@@ -22,6 +23,27 @@
 using namespace llvm;
 using namespace IRSimilarity;
 
+extern llvm::cl::opt<bool> UseNewDbgInfoFormat;
+extern cl::opt<cl::boolOrDefault> PreserveInputDbgFormat;
+extern bool WriteNewDbgInfoFormatToBitcode;
+extern cl::opt<bool> WriteNewDbgInfoFormat;
+
+// Backup all of the existing settings that may be modified when
+// PreserveInputDbgFormat=true, so that when the test is finished we return them
+// (and the "preserve" setting) to their original values.
+auto TempSettingChange() {
+  return make_scope_exit(
+      [OldPreserveInputDbgFormat = PreserveInputDbgFormat.getValue(),
+       OldUseNewDbgInfoFormat = UseNewDbgInfoFormat.getValue(),
+       OldWriteNewDbgInfoFormatToBitcode = WriteNewDbgInfoFormatToBitcode,
+       OldWriteNewDbgInfoFormat = WriteNewDbgInfoFormat.getValue()] {
+        PreserveInputDbgFormat = OldPreserveInputDbgFormat;
+        UseNewDbgInfoFormat = OldUseNewDbgInfoFormat;
+        WriteNewDbgInfoFormatToBitcode = OldWriteNewDbgInfoFormatToBitcode;
+        WriteNewDbgInfoFormat = OldWriteNewDbgInfoFormat;
+      });
+}
+
 static std::unique_ptr<Module> makeLLVMModule(LLVMContext &Context,
                                               StringRef ModuleStr) {
   SMDiagnostic Err;
@@ -1308,6 +1330,9 @@ TEST(IRInstructionMapper, CallBrInstIllegal) {
 
 // Checks that an debuginfo intrinsics are mapped to be invisible.  Since they
 // do not semantically change the program, they can be recognized as similar.
+// FIXME: PreserveInputDbgFormat is set to true because this test contains
+// malformed debug info that cannot be converted to the new debug info format;
+// this test should be updated later to use valid debug info.
 TEST(IRInstructionMapper, DebugInfoInvisible) {
   StringRef ModuleString = R"(
                           define i32 @f(i32 %a, i32 %b) {
@@ -1320,6 +1345,8 @@ TEST(IRInstructionMapper, DebugInfoInvisible) {
 
                           declare void @llvm.dbg.value(metadata)
                           !0 = distinct !{!"test\00", i32 10})";
+  auto SettingGuard = TempSettingChange();
+  PreserveInputDbgFormat = cl::boolOrDefault::BOU_TRUE;
   LLVMContext Context;
   std::unique_ptr<Module> M = makeLLVMModule(Context, ModuleString);
 
@@ -1916,6 +1943,9 @@ TEST(IRSimilarityCandidate, CheckRegionsDifferentTypes) {
 
 // Check that debug instructions do not impact similarity. They are marked as
 // invisible.
+// FIXME: PreserveInputDbgFormat is set to true because this test contains
+// malformed debug info that cannot be converted to the new debug info format;
+// this test should be updated later to use valid debug info.
 TEST(IRSimilarityCandidate, IdenticalWithDebug) {
   StringRef ModuleString = R"(
                           define i32 @f(i32 %a, i32 %b) {
@@ -1938,6 +1968,8 @@ TEST(IRSimilarityCandidate, IdenticalWithDebug) {
                           declare void @llvm.dbg.value(metadata)
                           !0 = distinct !{!"test\00", i32 10}
                           !1 = distinct !{!"test\00", i32 11})";
+  auto SettingGuard = TempSettingChange();
+  PreserveInputDbgFormat = cl::boolOrDefault::BOU_TRUE;
   LLVMContext Context;
   std::unique_ptr<Module> M = makeLLVMModule(Context, ModuleString);
 
diff --git a/llvm/unittests/IR/BasicBlockDbgInfoTest.cpp b/llvm/unittests/IR/BasicBlockDbgInfoTest.cpp
index 905928819dda80..cea0e6fd852a39 100644
--- a/llvm/unittests/IR/BasicBlockDbgInfoTest.cpp
+++ b/llvm/unittests/IR/BasicBlockDbgInfoTest.cpp
@@ -72,8 +72,6 @@ TEST(BasicBlockDbgInfoTest, InsertAfterSelf) {
     !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();
 
@@ -104,8 +102,6 @@ TEST(BasicBlockDbgInfoTest, InsertAfterSelf) {
   auto Range2 = RetInst->getDbgRecordRange();
   EXPECT_EQ(std::distance(Range2.begin(), Range2.end()), 1u);
 
-  M->convertFromNewDbgValues();
-
   UseNewDbgInfoFormat = false;
 }
 
@@ -140,8 +136,6 @@ TEST(BasicBlockDbgInfoTest, MarkerOperations) {
 
   // Fetch the entry block,
   BasicBlock &BB = M->getFunction("f")->getEntryBlock();
-  // Convert the module to "new" form debug-info.
-  M->convertToNewDbgValues();
   EXPECT_EQ(BB.size(), 2u);
 
   // Fetch out our two markers,
@@ -276,8 +270,6 @@ TEST(BasicBlockDbgInfoTest, HeadBitOperations) {
   // Test that the movement of debug-data when using moveBefore etc and
   // insertBefore etc are governed by the "head" bit of iterators.
   BasicBlock &BB = M->getFunction("f")->getEntryBlock();
-  // Convert the module to "new" form debug-info.
-  M->convertToNewDbgValues();
 
   // Test that the head bit behaves as expected: it should be set when the
   // code wants the _start_ of the block, but not otherwise.
@@ -385,8 +377,6 @@ TEST(BasicBlockDbgInfoTest, InstrDbgAccess) {
   // Check that DbgVariableRecords can be accessed from Instructions without
   // digging into the depths of DbgMarkers.
   BasicBlock &BB = M->getFunction("f")->getEntryBlock();
-  // Convert the module to "new" form debug-info.
-  M->convertToNewDbgValues();
 
   Instruction *BInst = &*BB.begin();
   Instruction *CInst = BInst->getNextNode();
@@ -523,7 +513,6 @@ class DbgSpliceTest : public ::testing::Test {
   void SetUp() override {
     UseNewDbgInfoFormat = true;
     M = parseIR(C, SpliceTestIR.c_str());
-    M->convertToNewDbgValues();
 
     BBEntry = &M->getFunction("f")->getEntryBlock();
     BBExit = BBEntry->getNextNode();
@@ -1163,7 +1152,6 @@ TEST(BasicBlockDbgInfoTest, DbgSpliceTrailing) {
 
   BasicBlock &Entry = M->getFunction("f")->getEntryBlock();
   BasicBlock &Exit = *Entry.getNextNode();
-  M->convertToNewDbgValues();
 
   // Begin by forcing entry block to have dangling DbgVariableRecord.
   Entry.getTerminator()->eraseFromParent();
@@ -1217,7 +1205,6 @@ TEST(BasicBlockDbgInfoTest, RemoveInstAndReinsert) {
 )");
 
   BasicBlock &Entry = M->getFunction("f")->getEntryBlock();
-  M->convertToNewDbgValues();
 
   // Fetch the relevant instructions from the converted function.
   Instruction *SubInst = &*Entry.begin();
@@ -1296,7 +1283,6 @@ TEST(BasicBlockDbgInfoTest, RemoveInstAndReinsertForOneDbgVariableRecord) {
 )");
 
   BasicBlock &Entry = M->getFunction("f")->getEntryBlock();
-  M->convertToNewDbgValues();
 
   // Fetch the relevant instructions from the converted function.
   Instruction *SubInst = &*Entry.begin();
@@ -1380,7 +1366,6 @@ TEST(BasicBlockDbgInfoTest, DbgSpliceToEmpty1) {
   Function &F = *M->getFunction("f");
   BasicBlock &Entry = F.getEntryBlock();
   BasicBlock &Exit = *Entry.getNextNode();
-  M->convertToNewDbgValues();
 
   // Begin by forcing entry block to have dangling DbgVariableRecord.
   Entry.getTerminator()->eraseFromParent();
@@ -1450,7 +1435,6 @@ TEST(BasicBlockDbgInfoTest, DbgSpliceToEmpty2) {
   Function &F = *M->getFunction("f");
   BasicBlock &Entry = F.getEntryBlock();
   BasicBlock &Exit = *Entry.getNextNode();
-  M->convertToNewDbgValues();
 
   // Begin by forcing entry block to have dangling DbgVariableRecord.
   Entry.getTerminator()->eraseFromParent();
@@ -1520,7 +1504,6 @@ TEST(BasicBlockDbgInfoTest, DbgMoveToEnd) {
   Function &F = *M->getFunction("f");
   BasicBlock &Entry = F.getEntryBlock();
   BasicBlock &Exit = *Entry.getNextNode();
-  M->convertToNewDbgValues();
 
   // Move the return to the end of the entry block.
   Instruction *Br = Entry.getTerminator();
diff --git a/llvm/unittests/IR/DebugInfoTest.cpp b/llvm/unittests/IR/DebugInfoTest.cpp
index d06b979bf4a1c4..2e75384c67b063 100644
--- a/llvm/unittests/IR/DebugInfoTest.cpp
+++ b/llvm/unittests/IR/DebugInfoTest.cpp
@@ -237,6 +237,9 @@ TEST(DbgVariableIntrinsic, EmptyMDIsKillLocation) {
 // Duplicate of above test, but in DbgVariableRecord representation.
 TEST(MetadataTest, DeleteInstUsedByDbgVariableRecord) {
   LLVMContext C;
+  bool OldDbgValueMode = UseNewDbgInfoFormat;
+  UseNewDbgInfoFormat = true;
+
   std::unique_ptr<Module> M = parseIR(C, R"(
     define i16 @f(i16 %a) !dbg !6 {
       %b = add i16 %a, 1, !dbg !11
@@ -262,10 +265,7 @@ TEST(MetadataTest, DeleteInstUsedByDbgVariableRecord) {
     !11 = !DILocation(line: 1, column: 1, scope: !6)
 )");
 
-  bool OldDbgValueMode = UseNewDbgInfoFormat;
-  UseNewDbgInfoFormat = true;
   Instruction &I = *M->getFunction("f")->getEntryBlock().getFirstNonPHI();
-  M->convertToNewDbgValues();
 
   // Find the DbgVariableRecords using %b.
   SmallVector<DbgValueInst *, 2> DVIs;
@@ -1044,10 +1044,6 @@ TEST(MetadataTest, ConvertDbgToDbgVariableRecord) {
 TEST(MetadataTest, DbgVariableRecordConversionRoutines) {
   LLVMContext C;
 
-  // For the purpose of this test, set and un-set the command line option
-  // corresponding to UseNewDbgInfoFormat.
-  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
@@ -1077,6 +1073,11 @@ TEST(MetadataTest, DbgVariableRecordConversionRoutines) {
     !11 = !DILocation(line: 1, column: 1, scope: !6)
 )");
 
+  // For the purpose of this test, set and un-set the command line option
+  // corresponding to UseNewDbgInfoFormat, but only after parsing, to ensure
+  // that the IR starts off in the old format.
+  UseNewDbgInfoFormat = true;
+
   // Check that the conversion routines and utilities between dbg.value
   // debug-info format and DbgVariableRecords works.
   Function *F = M->getFunction("f");
diff --git a/llvm/unittests/IR/InstructionsTest.cpp b/llvm/unittests/IR/InstructionsTest.cpp
index b47c73f0b329ae..6c4debf4dbec8c 100644
--- a/llvm/unittests/IR/InstructionsTest.cpp
+++ b/llvm/unittests/IR/InstructionsTest.cpp
@@ -31,6 +31,8 @@
 #include "gtest/gtest.h"
 #include <memory>
 
+extern llvm::cl::opt<llvm::cl::boolOrDefault> PreserveInputDbgFormat;
+
 namespace llvm {
 namespace {
 
@@ -1460,6 +1462,8 @@ TEST(InstructionsTest, GetSplat) {
 
 TEST(InstructionsTest, SkipDebug) {
   LLVMContext C;
+  cl::boolOrDefault OldDbgFormat = PreserveInputDbgFormat;
+  PreserveInputDbgFormat = cl::boolOrDefault::BOU_TRUE;
   std::unique_ptr<Module> M = parseIR(C,
                                       R"(
       declare void @llvm.dbg.value(metadata, metadata, metadata)
@@ -1495,6 +1499,7 @@ TEST(InstructionsTest, SkipDebug) {
 
   // After the terminator, there are no non-debug instructions.
   EXPECT_EQ(nullptr, Term->getNextNonDebugInstruction());
+  PreserveInputDbgFormat = OldDbgFormat;
 }
 
 TEST(InstructionsTest, PhiMightNotBeFPMathOperator) {
diff --git a/llvm/unittests/Transforms/Utils/CloningTest.cpp b/llvm/unittests/Transforms/Utils/CloningTest.cpp
index 025771f07ce5d4..6f4e860d604680 100644
--- a/llvm/unittests/Transforms/Utils/CloningTest.cpp
+++ b/llvm/unittests/Transforms/Utils/CloningTest.cpp
@@ -844,8 +844,9 @@ TEST(CloneFunction, CloneFunctionWithInlinedSubprograms) {
   EXPECT_FA...
[truncated]

[OCHyams]

LGTM-ish. Overall looks good, and is an important and exciting milestone indeed!

There's not many of them, but I'm worried about the loss of test coverage between this landing and the tests with FIXMEs getting fixed up. Fixing them up will cause some coverage loss for debug intrinsics, but IMO the primary / default mode should have the better coverage.

I don't think changing these few test alone is enough to suggest that debug intrinsic support is at all unsupported yet, but I think it brings us closer to the point where we need to consider telling people that debug intrinsic support is winding down. We should probably look at the opaque pointer transition - I may be misremembering but IIRC there was a release where typed pointers existed but they had limited testing / support. Ah yep,

LLVM 16: Opaque pointers are enabled by default. Typed pointers are supported on a best-effort basis only and not tested.

https://llvm.org/docs/OpaquePointers.html

Do you have a plan for those FIXME tests? I'm happy to take a look at fixing those up as a patch to land immediately after this one.

[OCHyams]

nit: static?
double bit: Can we give this a more specific name, e.g. "SaveDbgInfoFormat" or something similar?

[OCHyams]

I wonder if this should be shared somewhere? Maybe not necessary for this - depends if there's any common unit test header you can use? Then you could use it in the other tests too rather then manually saving and restoring
PreserveInputDbgFormat etc. YMMV. Please apply static and the rename mentioned at the other instance of this function either way.

EDIT: This doesn't matter much if we're going to fix up the tests immediately.

[OCHyams]

Ignore the above, the usage in this file gets removed in #90476 anyway.

[OCHyams]

I'd argue this test doesn't make sense for DbgRecords at all, and the comment should say something along the lines of "TODO: Remove when debug intrinsics are fully removed".

[OCHyams]

I think there's probably value in leaving this test as-is until debug intrinscs are completely removed, as not only does it test that DbgVariableRecord RAUW works, but it gives us the additional coverage that it works after converting from debug intrinsics, which is worth something IMO. Could you adjust this comment to reflect that if you agree?

[SLTozer]

I think that with the approach described above, where we only support intrinsics on a best-effort basis, maintaining the convert-from-intrinsic component seems unnecessary to me - but I don't mind leaving this test as-is until intrinsics are fully removed. I don't see the specific argument for maintaining the conversion coverage here (rather than testing conversion on its own), but if you think it'd be good then I'm happy to leave it in without further justification, since it's essentially 0 cost to maintain.

[jryans]

Should this change (or another one happening soon) mention the new debug info format in the release notes?

[OCHyams]

Once my inline comments are addressed I'm happy to approve this - I've converted the FIXME-tests that can/should be upgraded in #90476, which can land after this does.

[OCHyams]

Ignore the above, the usage in this file gets removed in #90476 anyway.

[dwblaikie]

Should this change (or another one happening soon) mention the new debug info format in the release notes?

+1 to this

[SLTozer]

Updated some of the tests according to the review comments, except for the comments that are resolved in #90476. Also added some release notes - I added them to the "debug info" section, but it wasn't clear to me whether that refers to debug info in LLVM's IR (superseding the "LLVM IR" category for debug-info-specific topics), or just debug info produced in objects built by LLVM.

[OCHyams]

Code / test changes LGTM

I'll leave it to @dwblaikie or @jryans to Accept if you're happy with the release notes situation.

[OCHyams]

Do we need to mention that testing for debug intrinsics will be best-effort going forward (/soon)?

[SLTozer]

That should probably be in the migration docs as well, but it makes sense to put it here.

[jryans]

Looks good to me, thanks for working on this! 😄

[SLTozer]

Following the failed merge, there's a small functional change and a number of trivial unit test updates needed to fix the patch - since I can't reopen the review, the commit containing the changes is here: SLTozer@a8fed21

Anyone able to eyeball it and give reapproval?

[jryans]

Additional commit looks good to me. 🙂

[OCHyams]

Additional commit looks good to me. 🙂

+1

I'd consider splitting up the return condition in DbgVariableRecord::isKillLocation into multiple returns, but ymmv, fine either way.

[danilaml]

Another drive-by: in my local build I was able to reliably reproduce unittests failures with this commit for debug build with Assertion !NodePtr->isKnownSentinel()' failed. assertion.

[SLTozer]

Another drive-by: in my local build I was able to reliably reproduce unittests failures with this commit for debug build with Assertion !NodePtr->isKnownSentinel()' failed. assertion.

Which unit test(s), and what platform+build configuration were you using?

[SLTozer]

I am sorry but I am going to revert this patch. The stage-2 IR verifier error is from a ThinLTO+SamplePGO build.

I've got stage-2 ThinLTO+SPGO build of clang using the flags you included but can't reproduce the error - do you have any more information that might be relevant to reproducing the error? Otherwise I'll continue to dig at the rest of the info you've provided to see if I can see an obvious cause of failure, and see if we can find some way to verify a fix without having to repeatedly land and revert until the fix is confirmed.

[danilaml]

@SLTozer my issue went away after some time and I wasn't able to reproduce it since. I'm guessing it's some sort of merge fluke.

[MaskRay]

I am sorry but I am going to revert this patch. The stage-2 IR verifier error is from a ThinLTO+SamplePGO build.

I've got stage-2 ThinLTO+SPGO build of clang using the flags you included but can't reproduce the error - do you have any more information that might be relevant to reproducing the error? Otherwise I'll continue to dig at the rest of the info you've provided to see if I can see an obvious cause of failure, and see if we can find some way to verify a fix without having to repeatedly land and revert until the fix is confirmed.

I am able to reproduce the assertion failure using an internal sample profile in a ThinLTO+SamplePGO+ -g build at current HEAD.
Disabling any of these three features (ThinLTO, SamplePGO, or -g) resolves the assertion failure.
(Both -g and -g -gsplit-dwarf can reproduce the assertion failure.)

// during ThinLTO backend compilation for clang/lib/StaticAnalyzer/Checkers/NumberObjectConversionChecker.cpp
llvm-project/llvm/lib/Linker/IRMover.cpp:260 in Type *(anonymous namespace)::TypeMapTy::get(Type *, SmallPtrSet<StructType *, 8>
&): !(Pair.first != Ty && Pair.second == Ty) && "mapping to a source type"

The sample profile file matters. I've tried a simpler profile using llvm-test-suite sqlite3.i (below), but that doesn't reproduce the crash.

• reland [RemoveDIs] Load into new debug info format by default in LLVM #89799
• create a stage 1 build
• Create a stage 2 ThinLTO+SamplePGO+split DWARF build: configure-llvm s2-custom-fdo -DLLVM_TARGETS_TO_BUILD=host -DLLVM_ENABLE_PROJECTS=clang -DCMAKE_{C,CXX}_FLAGS='-g -fdebug-info-for-profiling -funique-internal-linkage-names' -DCMAKE_EXE_LINKER_FLAGS=-Wl,--build-id -DLLVM_USE_SPLIT_DWARF=on -DLLVM_ENABLE_LTO=Thin
• Preprocess llvm-test-suite sqlite.c to sqlite.i: clang -DNDEBUG -w -DSTDC_HEADERS=1 -DHAVE_SYS_TYPES_H=1 -DHAVE_SYS_STAT_H=1 -DHAVE_STDLIB_H=1 -DHAVE_STRING_H=1 -DHAVE_MEMORY_H=1 -DHAVE_STRINGS_H=1 -DHAVE_INTTYPES_H=1 -DHAVE_STDINT_H=1 -DHAVE_UNISTD_H=1 -DSQLITE_OMIT_LOAD_EXTENSION=1 -DSQLITE_THREADSAFE=0 -I. -E llvm-test-suite/MultiSource/Applications/sqlite3/sqlite3.c -o sqlite3.i
• Collect perf.data using sqlite3.i on an machine with an Intel CPU: perf record -b -e BR_INST_RETIRED.NEAR_TAKEN:uppp -- /tmp/out/s2-custom-fdo/bin/clang -c -O1 sqlite3.i
• create_llvm_prof --binary=/tmp/out/s2-custom-fdo/bin/clang --profile=perf.data --out=clang.prof
• configure-llvm s2-custom-use -DLLVM_TARGETS_TO_BUILD=host -DLLVM_ENABLE_PROJECTS=clang -DCMAKE_{C,CXX}_FLAGS='-g -fdebug-info-for-profiling -funique-internal-linkage-names -fprofile-sample-use=/t/clang.prof' -DLLVM_USE_SPLIT_DWARF=on -DLLVM_ENABLE_LTO=Thin

---

I've also tried an instrumentation PGO with a simple profile and the -fprofile-use= build does not reproduce the assertion failure.

I know that the task is still on our side to figure out the issue, as this seems really difficult on your side.

[SLTozer]

I know that the task is still on our side to figure out the issue, as this seems really difficult on your side.

Thanks for this, and also for the additional info. I was beginning to suspect that a specific sample would be needed; part of the preconditions for the problem that you gave in your previous post are that the problem function is reachable from an instruction metadata attachment, and I've found exactly one instruction (in my non-reproducing build) prior to function importing that has the requisite attachment; that instruction exists in NumberObjectConversionChecker, and I can't see any way (in my build) for it to be imported into PointerIterationChecker, so it's not clear how/why the ValueMapper would touch it, though it could happen in the reverse case (importing NumberObjectConversionChecker into PointerIterationChecker).

Based on where this error first appears, I suspect that the sample profile is only needed to produce the bitcode modules for the thin link; the error happens during importing, which I think (I'm not very familiar with ThinLTO or PGO) happens before llvm-lto2 performs any post-link optimizations that would use the sample profile; if that's correct, then it should be possible to reproduce the error with just the pre-LTO bitcode modules (which could then be reduced further if needed by reducing the objects passed to the link) and the linker flags. If that is the case, and you can get a reproducer that contains no internal data, I should be able to handle the investigation.

[SLTozer]

Ping @MaskRay - any luck so far? And if not, is what I suggested in the post above (submitting just the pre-link bc modules without the sample profile) a possibility? If neither of those, I'd still be interested to know any further details that have turned up - the details so far have narrowed things down a bit, but still haven't gotten me close to figuring out what state the IR would need to be in to trigger this error.

[MaskRay]

Ping @MaskRay - any luck so far? And if not, is what I suggested in the post above (submitting just the pre-link bc modules without the sample profile) a possibility? If neither of those, I'd still be interested to know any further details that have turned up - the details so far have narrowed things down a bit, but still haven't gotten me close to figuring out what state the IR would need to be in to trigger this error.

Good news. With this rebased reapply commit (MaskRay@18eaa15), I can no longer reproduce the crash as our AutoFDO profile updated.

Perhaps just reland this...

[SLTozer]

This certainly is an elusive bug! Hopefully if it shows up again after relanding we'll have better luck identifying the issue.

[jeanPerier]

I think this patch is causing a buildbot failure with a flang test in the llvm-test-suite using -g and -flto.: https://lab.llvm.org/buildbot/#/builders/198/builds/10772

An assert is hit in llvm::Module::removeDebugIntrinsicDeclarations.

Reproducer:

file.f90:

subroutine sub()
 real :: x
end subroutine

flang-new -c -g -flto file.f90

flang-new: llvm/lib/IR/Module.cpp:94: void llvm::Module::removeDebugIntrinsicDeclarations(): Assertion `(!isMaterialized() || DeclareIntrinsicFn->hasZeroLiveUses()) && "Debug declare intrinsic should have had uses removed."' failed.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
Stack dump:
0.	Program arguments: bin/flang-new -fc1 -triple x86_64-unknown-linux-gnu -emit-llvm-bc -fcolor-diagnostics -flto=full -mrelocation-model pic -pic-level 2 -pic-is-pie -target-cpu x86-64 -debug-info-kind=standalone -resource-dir bin/.. -mframe-pointer=all -o - -x f95-cpp-input rep_crash.f90
 #0 0x00005635907c86c8 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (bin/flang-new+0x2ea86c8)
 #1 0x00005635907c636e llvm::sys::RunSignalHandlers() (bin/flang-new+0x2ea636e)
 #2 0x00005635907c9038 SignalHandler(int) Signals.cpp:0:0
 #3 0x00007fcbada42520 (/lib/x86_64-linux-gnu/libc.so.6+0x42520)
 #4 0x00007fcbada969fc __pthread_kill_implementation ./nptl/pthread_kill.c:44:76
 #5 0x00007fcbada969fc __pthread_kill_internal ./nptl/pthread_kill.c:78:10
 #6 0x00007fcbada969fc pthread_kill ./nptl/pthread_kill.c:89:10
 #7 0x00007fcbada42476 gsignal ./signal/../sysdeps/posix/raise.c:27:6
 #8 0x00007fcbada287f3 abort ./stdlib/abort.c:81:7
 #9 0x00007fcbada2871b _nl_load_domain ./intl/loadmsgcat.c:1177:9
#10 0x00007fcbada39e96 (/lib/x86_64-linux-gnu/libc.so.6+0x39e96)
#11 0x0000563594ec414a llvm::Module::removeDebugIntrinsicDeclarations() (bin/flang-new+0x75a414a)
#12 0x0000563593e6ae9b llvm::BitcodeWriterPass::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) (bin/flang-new+0x654ae9b)
#13 0x00005635908f523d llvm::detail::PassModel<llvm::Module, llvm::BitcodeWriterPass, llvm::AnalysisManager<llvm::Module>>::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) FrontendActions.cpp:0:0
#14 0x0000563594ed6356 llvm::PassManager<llvm::Module, llvm::AnalysisManager<llvm::Module>>::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) (bin/flang-new+0x75b6356)
#15 0x000056359081048d Fortran::frontend::CodeGenAction::runOptimizationPipeline(llvm::raw_pwrite_stream&) (bin/flang-new+0x2ef048d)
#16 0x0000563590812a32 Fortran::frontend::CodeGenAction::executeAction() (bin/flang-new+0x2ef2a32)
#17 0x000056359080349c Fortran::frontend::FrontendAction::execute() (bin/flang-new+0x2ee349c)
#18 0x00005635907df712 Fortran::frontend::CompilerInstance::executeAction(Fortran::frontend::FrontendAction&) (bin/flang-new+0x2ebf712)
#19 0x00005635908077ef Fortran::frontend::executeCompilerInvocation(Fortran::frontend::CompilerInstance*) (bin/flang-new+0x2ee77ef)
#20 0x00005635903837d6 fc1_main(llvm::ArrayRef<char const*>, char const*) (bin/flang-new+0x2a637d6)
#21 0x0000563590381e4b main (bin/flang-new+0x2a61e4b)
#22 0x00007fcbada29d90 __libc_start_call_main ./csu/../sysdeps/nptl/libc_start_call_main.h:58:16
#23 0x00007fcbada29e40 call_init ./csu/../csu/libc-start.c:128:20
#24 0x00007fcbada29e40 __libc_start_main ./csu/../csu/libc-start.c:379:5
#25 0x0000563590381865 _start (bin/flang-new+0x2a61865)

I was unable to reproduce when dumping llvm IR and starting back from that, but one can see the llvm-ir with debug info generated for that Fortran program with flang-new -fc1 -emit-llvm -debug-info-kind=standalone.

FYI @abidh.

[SLTozer]

I'll take a look - if it's not a quick fix I'll revert and resubmit with the flang stuff working. To clarify the nature of the error though, LLVM is moving to use a non-intrinsic-based representation of debug info, and this error has occurred because even though the module is set to use debug records, it contains debug intrinsics, which is an error. Since this didn't show up the last time this patch landed, there's presumably some new code added to flang since then that inserts debug intrinsics in some way other than the DIBuilder, so it might be very straightforward to update the code in question.

[jeanPerier]

Thanks!

Since this didn't show up the last time this patch landed, there's presumably some new code added to flang since then that inserts debug intrinsics in some way other than the DIBuilder, so it might be very straightforward to update the code in question.

That is very likely, @abidh is currently making patch to generate debug info in flang.

Note that flang is generating MLIR LLVM dialect. It generates mlir::LLVM::DbgDeclareOp here. These MLIR ops are then later translated into LLVM IR (likely in mlir/lib/Target/LLVMIR/ModuleTranslation.cpp or mlir/lib/Target/LLVMIR/DebugTranslation.cpp but I am not sure here).

It is weird though that the flto flag matters here. Is it possible that the -flto pipeline would incorrectly mark the module as using debug records?

[SLTozer]

Yep, I see that - MLIR currently doesn't support debug records; before translating IR to MLIR we convert to intrinsic-form, so there probably needs to be a conversion layer inserted that converts from intrinsic-form back to record-form in the reverse direction.

I'm also confused as to why the flto flag matters; iirc, the LLVM test suite configs with debug info are just -O0 -g or -O3 -flto -g, so it might just be that any optimization with -g causes the error, and -flto is the only such config being run. Though it's also possible that the pipeline creates the IR module in a different way or at a different point that exposes the error.

[SLTozer]

Alright, I think I have the fix. Although it's trivial, it's not NFC, and so it should probably be reviewed rather than pushed without review. I'll revert this for now, and push it up when the other commit lands.