Fix for "DICompileUnit not listed in llvm.dbg.cu" verification error after cloning a function from a different module

[OlegPliss]

Summary

Currently when a function with debug info is cloned from a different module, the cloned function may have hanging DICompileUnits, so that the module with the cloned function fails debug info verification. For a simple example, please see the supplied CloneFunctionToDifferentModule test.

The proposed fix inserts all DICompileUnits reachable from the cloned function to "llvm.dbg.cu" metadata operands of the cloned function module. Duplication of operands is carefully avoided.

[efriedma]

Why are the changes to CloneModule.cpp necessary?

Can you provide a testcase showing that we don't copy unnecessary compile units?

[OlegPliss]

In D66510#1638669, @efriedma wrote:

Why are the changes to CloneModule.cpp necessary?

CloneModule calls CloneFunction for all functions. These calls insert all DICompileUnits reachable from the functions to "llvm.dbg.cu" metadata. Without the changes in CloneModule those DICompileUnits are inserted again. Duplicate named metadata operands make output of some tests dependent on number of passes of the optimizer pipeline, and the tests fail.

Can you provide a testcase showing that we don't copy unnecessary compile units?

This property is guaranteed by correctness of DIFinder which is not touched by my changes. Only DIFinder.compile_units() are inserted to "llvm.dbg.cu". DIFinder.compile_units() is a set of compile units reachable from original function, and they are shared with the clone.

[aprantl]

At a first glance, this seems reasonable, I'm just wondering why this hasn't come up before. @rtereshin, any ideas?

[aprantl]

lib/Transforms/Utils/CloneModule.cpp

199

Should this be an insertCU helper function in DebugInfo.h?

[OlegPliss]

lib/Transforms/Utils/CloneModule.cpp

199

If you mean insertCU(DICompileUnit*, SmallPtrSet&), it would only be called twice and would increase number of code lines in the project. The worst, it would not always be called for insertion of DI CUs to "llvm.dbg.cu". Often the code deals with generic operands of generic NMD.

What would really help is proper abstractions for different NMD kinds. The doc says NMD operands is a collection. In general a collection may contain duplicates and is not ordered (and so is not indexable). For "llvm.dbg.cu" and many others a set would be more desired. Here the duplicates just complicate debug info comparison, waste CPU cycles and consume some space in DWARF. Meanwhile CloneModule tries to preserve the operand ordering, and other functions do not.

[OlegPliss]

Check for unnecessary DI compile units is added to the test on Eli's request.

[OlegPliss]

In D66510#1638669, @efriedma wrote:

Can you provide a testcase showing that we don't copy unnecessary compile units?

Done.

[OlegPliss]

In D66510#1638669, @efriedma wrote:

Why are the changes to CloneModule.cpp necessary?

Can you provide a testcase showing that we don't copy unnecessary compile units?

Ping. I need an approval to commit the changes.

[OlegPliss]

In D66510#1639448, @aprantl wrote:

At a first glance, this seems reasonable, I'm just wondering why this hasn't come up before. @rtereshin, any ideas?

Ping. I need an approval to commit the changes.

lib/Transforms/Utils/CloneFunction.cpp

for (Function::iterator BB =
            cast<BasicBlock>(VMap[&OldFunc->front()])->getIterator(),
                           BE = NewFunc->end();
        BB != BE; ++BB)
     // Loop over all instructions, fixing each one as we find it...
     for (Instruction &II : *BB)
       RemapInstruction(&II, VMap,
                        ModuleLevelChanges ? RF_None : RF_NoModuleLevelChanges,
                        TypeMapper, Materializer);
+
+  // Register all DICompileUnits of the old parent module in the new parent module
+  auto* OldModule = OldFunc->getParent();
+  auto* NewModule = NewFunc->getParent();
+  if (OldModule && NewModule && OldModule != NewModule && DIFinder.compile_unit_count()) {
+    auto* NMD = NewModule->getOrInsertNamedMetadata("llvm.dbg.cu");
+    // Avoid multiple insertions of the same DICompileUnit to NMD.
+    SmallPtrSet<const void*, 8> Visited;
+    for (auto* Operand : NMD->operands())
+      Visited.insert(Operand);
+    for (auto* Unit : DIFinder.compile_units())
+      // VMap.MD()[Unit] == Unit
+      if (Visited.insert(Unit).second)
+        NMD->addOperand(Unit);
+  }
 }
 
 /// Return a copy of the specified function and add it to that function's
 /// module.  Also, any references specified in the VMap are changed to refer to
 /// their mapped value instead of the original one.  If any of the arguments to
 /// the function are in the VMap, the arguments are deleted from the resultant
 /// function.  The VMap is updated to include mappings from all of the
 /// instructions and basicblocks in the function from their old to new values.

lib/Transforms/Utils/CloneModule.cpp

for (Module::const_alias_iterator I = M.alias_begin(), E = M.alias_end();
     if (!ShouldCloneDefinition(&*I))
       continue;
     GlobalAlias *GA = cast<GlobalAlias>(VMap[&*I]);
     if (const Constant *C = I->getAliasee())
       GA->setAliasee(MapValue(C, VMap));
   }
 
   // And named metadata....
+  const auto* LLVM_DBG_CU = M.getNamedMetadata("llvm.dbg.cu");
   for (Module::const_named_metadata_iterator I = M.named_metadata_begin(),
                                              E = M.named_metadata_end();
        I != E; ++I) {
     const NamedMDNode &NMD = *I;
     NamedMDNode *NewNMD = New->getOrInsertNamedMetadata(NMD.getName());
+    if (&NMD == LLVM_DBG_CU) {
+      // Do not insert duplicate operands.
+      SmallPtrSet<const void*, 8> Visited;
+      for (const auto* Operand : NewNMD->operands())
+        Visited.insert(Operand);
+      for (const auto* Operand : NMD.operands()) {
+        auto* MappedOperand = MapMetadata(Operand, VMap);
+        if (Visited.insert(MappedOperand).second)
+          NewNMD->addOperand(MappedOperand);
+      }
+    } else
       for (unsigned i = 0, e = NMD.getNumOperands(); i != e; ++i)
         NewNMD->addOperand(MapMetadata(NMD.getOperand(i), VMap));
   }
 
   return New;
 }
 
 extern "C" {
 
 LLVMModuleRef LLVMCloneModule(LLVMModuleRef M) {
   return wrap(CloneModule(*unwrap(M)).release());
 }
 
 }

unittests/Transforms/Utils/CloningTest.cpp

if (DbgDeclareInst* OldIntrin = dyn_cast<DbgDeclareInst>(&OldI)) {
       }
     }
 
     ++OldIter;
     ++NewIter;
   }
 }
 
+TEST(CloneFunction, CloneFunctionToDifferentModule) {
+  StringRef ImplAssembly = R"(
+    define void @foo() {
+      ret void, !dbg !5
+    }
+
+    !llvm.module.flags = !{!0}
+    !llvm.dbg.cu = !{!2}
+    !0 = !{i32 1, !"Debug Info Version", i32 3}
+    !1 = distinct !DISubprogram(unit: !2)
+    !2 = distinct !DICompileUnit(language: DW_LANG_C99, file: !3)
+    !3 = !DIFile(filename: "foo.c", directory: "/tmp")
+    !4 = distinct !DISubprogram(unit: !2)
+    !5 = !DILocation(line: 4, scope: !1)
+  )";
+  StringRef DeclAssembly = R"(
+    declare void @foo()
+  )";
+
+  LLVMContext Context;
+  SMDiagnostic Error;
+
+  auto ImplModule = parseAssemblyString(ImplAssembly, Error, Context);
+  auto* ImplFunction = ImplModule->getFunction("foo");
+  EXPECT_TRUE(ImplFunction != nullptr);
+
+  auto DeclModule = parseAssemblyString(DeclAssembly, Error, Context);
+  auto* DeclFunction = DeclModule->getFunction("foo");
+  EXPECT_TRUE(DeclFunction != nullptr);
+
+  ValueToValueMapTy VMap;
+  VMap[ImplFunction] = DeclFunction;
+  // No args to map
+  SmallVector<ReturnInst *, 8> Returns;
+  CloneFunctionInto(DeclFunction, ImplFunction, VMap, true, Returns);
+
+  EXPECT_FALSE(verifyModule(*ImplModule, &errs()));
+  EXPECT_FALSE(verifyModule(*DeclModule, &errs()));
+}
+
 class CloneModule : public ::testing::Test {
 protected:
   void SetUp() override {
     SetupModule();
     CreateOldModule();
     CreateNewModule();
   }