Fix cloning of debug info

[yuyichao]

Summary

Export function to clone function debug info and autmatically clone the debug info in CloneModule.

This currently causes a regression in the debug info of the generated code we saves to a file since some of the debug info are not copied. (especially after http://reviews.llvm.org/D14265)

Test done: The code copied into julia fixes the missing debug info on both x64 and aarch64.

[probinson]

While it's nice that you've verified this with julia, it really should have an in-tree test.

include/llvm/Transforms/Utils/Cloning.h

134 ↗

(On Diff #47703)

Please use triple-slash so doxygen will pick this up.

[yuyichao]

I'll fix the comment.

Any pointer to a similar test and/or how/where I should add such test?

[loladiro]

unittests/Transforms/Utils/Cloning.cpp is probably the right place.

[yuyichao]

Fix comment/doc in the header.
Add unit test. Verified that the added test only passes when the CloneDebugInfoMetadata line in CloneModule is added.

[loladiro]

LGTM, but I'd want @pcc to sign off before this gets committed. Also cc to @aprantl since this is DI related.

[pcc]

LGTM

unittests/Transforms/Utils/Cloning.cpp

471 ↗

(On Diff #47744)

Maybe also test that one of the subprogram fields has an expected value?

[tejohnson]

I was looking at this patch in the context of a related debug info cloning patch (D17338). If I understand how this and the CloneDebugInfoMetadata routine works, I think this may have some unintended consequences.

Specifically, CloneDebugInfoMetadata will not only clone the debug info metadata for the old function and place it on the new function, but will also add the cloned subprogram metadata to the list of subprograms hanging off the old module's DICompileUnit via AddOperand. This subprogram list will eventually be cloned to the dest module when named metadata is cloned.

Won't this leave the new cloned subprogram on the old module? Also, not sure what happens when the old subprogram list is mapped into the new module when the named metadata is cloned. Will the new subprogram end up in that list twice (once when mapping the old subprogram and once when mapping the new subprogram which probably uses the identify mapping since it isn't in the value map)?

If my understanding is incorrect, what am I missing?

[pcc]

@tejohnson I think you are correct. We (me and @eugenis) also observed another problem: this change introduces O(n^2) complexity in the module cloner. I have sent out D18583 that should solve both problems.

[tejohnson]

In D17165#386322, @pcc wrote:

@tejohnson I think you are correct. We (me and @eugenis) also observed another problem: this change introduces O(n^2) complexity in the module cloner. I have sent out D18583 that should solve both problems.

Yep, I could imagine that since the cloned debug is continually added to the source module. Thanks for the fix, will take a look at D18583.

llvm/trunk/include/llvm/Transforms/Utils/Cloning.h

 ///
 /// If ModuleLevelChanges is false, VMap contains no non-identity GlobalValue
 /// mappings, and debug info metadata will not be cloned.
 ///
 Function *CloneFunction(const Function *F, ValueToValueMapTy &VMap,
                         bool ModuleLevelChanges,
                         ClonedCodeInfo *CodeInfo = nullptr);
 
+/// Clone the module-level debug info associated with OldFunc. The cloned data
+/// will point to NewFunc instead.
+void CloneDebugInfoMetadata(Function *NewFunc, const Function *OldFunc,
+                            ValueToValueMapTy &VMap);
+
 /// Clone OldFunc into NewFunc, transforming the old arguments into references
 /// to VMap values.  Note that if NewFunc already has basic blocks, the ones
 /// cloned into it will be added to the end of the function.  This function
 /// fills in a list of return instructions, and can optionally remap types
 /// and/or append the specified suffix to all values cloned.
 ///
 /// If ModuleLevelChanges is false, VMap contains no non-identity GlobalValue
 /// mappings.

llvm/trunk/lib/Transforms/Utils/CloneFunction.cpp

static void AddOperand(DICompileUnit *CU, DISubprogramArray SPs,
   for (auto *SP : SPs)
     NewSPs.push_back(SP);
   NewSPs.push_back(NewSP);
   CU->replaceSubprograms(MDTuple::get(CU->getContext(), NewSPs));
 }
 
 // Clone the module-level debug info associated with OldFunc. The cloned data
 // will point to NewFunc instead.
-static void CloneDebugInfoMetadata(Function *NewFunc, const Function *OldFunc,
+void llvm::CloneDebugInfoMetadata(Function *NewFunc, const Function *OldFunc,
                                   ValueToValueMapTy &VMap) {
   DebugInfoFinder Finder;
   Finder.processModule(*OldFunc->getParent());
 
   const DISubprogram *OldSubprogramMDNode = FindSubprogram(OldFunc, Finder);
   if (!OldSubprogramMDNode) return;
 
   auto *NewSubprogram =
       cast<DISubprogram>(MapMetadata(OldSubprogramMDNode, VMap));

llvm/trunk/lib/Transforms/Utils/CloneModule.cpp

for (Module::const_iterator I = M->begin(), E = M->end(); I != E; ++I) {
     if (!I->isDeclaration()) {
       Function::arg_iterator DestI = F->arg_begin();
       for (Function::const_arg_iterator J = I->arg_begin(); J != I->arg_end();
            ++J) {
         DestI->setName(J->getName());
         VMap[&*J] = &*DestI++;
       }
 
+      CloneDebugInfoMetadata(F, &*I, VMap);
       SmallVector<ReturnInst*, 8> Returns;  // Ignore returns cloned.
       CloneFunctionInto(F, &*I, VMap, /*ModuleLevelChanges=*/true, Returns);
     }
 
     if (I->hasPersonalityFn())
       F->setPersonalityFn(MapValue(I->getPersonalityFn(), VMap));
   }
 

llvm/trunk/unittests/Transforms/Utils/Cloning.cpp

void SetUp() override {
     SetupModule();
     CreateOldModule();
     CreateNewModule();
   }
 
   void SetupModule() { OldM = new Module("", C); }
 
   void CreateOldModule() {
+    DIBuilder DBuilder(*OldM);
     IRBuilder<> IBuilder(C);
 
     auto *FuncType = FunctionType::get(Type::getVoidTy(C), false);
     auto *PersFn = Function::Create(FuncType, GlobalValue::ExternalLinkage,
                                     "persfn", OldM);
     auto *F =
         Function::Create(FuncType, GlobalValue::PrivateLinkage, "f", OldM);
     F->setPersonalityFn(PersFn);
+
+    // Create debug info
+    auto *File = DBuilder.createFile("filename.c", "/file/dir/");
+    DITypeRefArray ParamTypes = DBuilder.getOrCreateTypeArray(None);
+    DISubroutineType *DFuncType = DBuilder.createSubroutineType(ParamTypes);
+    auto *CU =
+        DBuilder.createCompileUnit(dwarf::DW_LANG_C99, "filename.c",
+                                   "/file/dir", "CloneModule", false, "", 0);
+    // Function DI
+    auto *Subprogram = DBuilder.createFunction(CU, "f", "f", File, 4, DFuncType,
+                                               true, true, 3, 0, false);
+    F->setSubprogram(Subprogram);
+
     auto *Entry = BasicBlock::Create(C, "", F);
     IBuilder.SetInsertPoint(Entry);
     IBuilder.CreateRetVoid();
+
+    // Finalize the debug info
+    DBuilder.finalize();
   }
 
   void CreateNewModule() { NewM = llvm::CloneModule(OldM).release(); }
 
   LLVMContext C;
   Module *OldM;
   Module *NewM;
 };
 
 TEST_F(CloneModule, Verify) {
   EXPECT_FALSE(verifyModule(*NewM));
 }
 
+TEST_F(CloneModule, Subprogram) {
+  Function *NewF = NewM->getFunction("f");
+  DISubprogram *SP = NewF->getSubprogram();
+  EXPECT_TRUE(SP != nullptr);
+  EXPECT_EQ(SP->getName(), "f");
+  EXPECT_EQ(SP->getFile()->getFilename(), "filename.c");
+  EXPECT_EQ(SP->getLine(), (unsigned)4);
+}
 }