[Cloning] Move distinct GlobalVariable debug info metadata in CloneModule

[EwanCrawford]

Summary

Duplicating the distinct Subprogram and CU metadata nodes seems like the incorrect thing to do in CloneModule for GlobalVariable debug info. As it results in the scope of the GlobalVariable DI no longer being consistent with the rest of the module, and the new CU is absent from llvm.dbg.cu.

Fixed by adding RF_MoveDistinctMDs to MapMetadata flags for GlobalVariables.

Current unit test IR after clone:

@gv = global i32 1, comdat($comdat), !dbg !0, !type !5

define private void @f() comdat($comdat) personality void ()* @persfn !dbg !14 {

!llvm.dbg.cu = !{!10}

!0 = !DIGlobalVariableExpression(var: !1)
!1 = distinct !DIGlobalVariable(name: "gv", linkageName: "gv", scope: !2, file: !3, line: 1, type: !9, isLocal: false, isDefinition: true)
!2 = distinct !DISubprogram(name: "f", linkageName: "f", scope: null, file: !3, line: 4, type: !4, isLocal: true, isDefinition: true, scopeLine: 3, isOptimized: false, unit: !6, variables: !5)
!3 = !DIFile(filename: "filename.c", directory: "/file/dir/")
!4 = !DISubroutineType(types: !5)
!5 = !{}
!6 = distinct !DICompileUnit(language: DW_LANG_C99, file: !7, producer: "CloneModule", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !5, globals: !8)
!7 = !DIFile(filename: "filename.c", directory: "/file/dir")
!8 = !{!0}
!9 = !DIBasicType(tag: DW_TAG_unspecified_type, name: "decltype(nullptr)")
!10 = distinct !DICompileUnit(language: DW_LANG_C99, file: !7, producer: "CloneModule", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !5, globals: !11)
!11 = !{!12}
!12 = !DIGlobalVariableExpression(var: !13)
!13 = distinct !DIGlobalVariable(name: "gv", linkageName: "gv", scope: !14, file: !3, line: 1, type: !9, isLocal: false, isDefinition: true)
!14 = distinct !DISubprogram(name: "f", linkageName: "f", scope: null, file: !3, line: 4, type: !4, isLocal: true, isDefinition: true, scopeLine: 3, isOptimized: false, unit: !10, variables: !5)

Patched IR after clone:

@gv = global i32 1, comdat($comdat), !dbg !0, !type !5

define private void @f() comdat($comdat) personality void ()* @persfn !dbg !2 {

!llvm.dbg.cu = !{!6}

!0 = !DIGlobalVariableExpression(var: !1)
!1 = distinct !DIGlobalVariable(name: "gv", linkageName: "gv", scope: !2, file: !3, line: 1, type: !9, isLocal: false, isDefinition: true)
!2 = distinct !DISubprogram(name: "f", linkageName: "f", scope: null, file: !3, line: 4, type: !4, isLocal: true, isDefinition: true, scopeLine: 3, isOptimized: false, unit: !6, variables: !5)
!3 = !DIFile(filename: "filename.c", directory: "/file/dir/")
!4 = !DISubroutineType(types: !5)
!5 = !{}
!6 = distinct !DICompileUnit(language: DW_LANG_C99, file: !7, producer: "CloneModule", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !5, globals: !8)
!7 = !DIFile(filename: "filename.c", directory: "/file/dir")
!8 = !{!0}
!9 = !DIBasicType(tag: DW_TAG_unspecified_type, name: "decltype(nullptr)")

[aprantl]

IIUC this is because the DIGlobalVariables are currently reachable both through the !dbg attachment on the Global and through the globals: field in the DICompileUnit?
This should be unaffected by you change, but could you, to prevent this from breaking whenever we make changes to the metadata graph in the future, also add a test for an constant global variable that is not attached to any symbol?

!1 = !DIGlobalVariableExpression(var: !3, expr: !2)
!2 = !DIExpression(DW_OP_constu, 42, DW_OP_stack_value)
!3 = !DIGlobalVariable(...)

[aprantl]

to clarify: not attached to any Global, but listed in the globals: list of a DICompileUnit.

[EwanCrawford]

Thanks for taking a look Adrian, I think the DIGlobalVariables being accessible via two routes is indeed the reason for this.

Added that extra test.

[rtereshin]

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

189

I'm hitting a case with an older version of LLVM I can't unfortunately expose as a test: this part of CloneModule while iterating over operands of !llvm.dbg.cu, which are DICompileUnits, sometimes duplicates a DICompileUnit. Not always, strangely enough. The duplicate itself is completely redundant, but it also explicitly breaks the debug info as that CU doesn't get included into llvm.dbg.cu list. Adding RF_MoveDistinctMDs flag to this MapMetadata call solves the problem.

Do you think you could take a look and see if we need RF_MoveDistinctMDs here as well as above?

[dexonsmith]

RF_MoveDistinctMDs is not safe to use in llvm::CloneModule(), since this flag causes the IRLinker to mutate distinct nodes instead of cloning them. That's only sound if the original module is being discarded after linking (e.g., when you want to link some bitcode in, you read it into an ephemeral module, link in what you want, and discard what's left). In llvm::CloneModule(), you'll end up with the original module referencing global variables owned by the cloned module.

I suspect adding RF_MoveDistinctMDs here papered over an underlying bug related to cloning llvm.dbg.cu. See my longer comment in:
https://bugs.llvm.org/show_bug.cgi?id=48841

[dexonsmith]

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

189

Adding RF_MoveDistinctMDs flag to this MapMetadata call solves the problem.

Doing so will only paper over the problem, and invalidate the source module being cloned.

Whatever is happening here may be the key to understanding both bugs. MapMetadata should clone metadata, and NewNMD should reference the clone instead of the original. It sounds like some caller is not going through VMap and is getting a different copy.

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

if (!ShouldCloneDefinition(&*I)) {
       continue;
     }
     if (I->hasInitializer())
       GV->setInitializer(MapValue(I->getInitializer(), VMap));
 
     SmallVector<std::pair<unsigned, MDNode *>, 1> MDs;
     I->getAllMetadata(MDs);
     for (auto MD : MDs)
-      GV->addMetadata(MD.first, *MapMetadata(MD.second, VMap));
+      GV->addMetadata(MD.first,
+                      *MapMetadata(MD.second, VMap, RF_MoveDistinctMDs));
 
     copyComdat(GV, &*I);
   }
 
   // Similarly, copy over function bodies now...
   //
   for (const Function &I : *M) {
     if (I.isDeclaration())
std::unique_ptr<Module> llvm::CloneModule(
   }
 
   // And named metadata....
   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());
     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());
 }
 
 }

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

auto *CU = DBuilder.createCompileUnit(dwarf::DW_LANG_C99,
                                                               "/file/dir"),
                                           "CloneModule", false, "", 0);
     // Function DI
     auto *Subprogram =
         DBuilder.createFunction(CU, "f", "f", File, 4, DFuncType, true, true, 3,
                                 DINode::FlagZero, false);
     F->setSubprogram(Subprogram);
 
+    // Create and assign DIGlobalVariableExpression to gv
+    auto GVExpression = DBuilder.createGlobalVariableExpression(
+        Subprogram, "gv", "gv", File, 1, DBuilder.createNullPtrType(), false);
+    GV->addDebugInfo(GVExpression);
+
+    // DIGlobalVariableExpression not attached to any global variable
+    auto Expr = DBuilder.createExpression(
+        ArrayRef<uint64_t>{dwarf::DW_OP_constu, 42U, dwarf::DW_OP_stack_value});
+
+    DBuilder.createGlobalVariableExpression(
+        Subprogram, "unattached", "unattached", File, 1,
+        DBuilder.createNullPtrType(), false, Expr);
+
     auto *Entry = BasicBlock::Create(C, "", F);
     IBuilder.SetInsertPoint(Entry);
     IBuilder.CreateRetVoid();
 
     // Finalize the debug info
     DBuilder.finalize();
   }
 
TEST_F(CloneModule, Subprogram) {
   EXPECT_EQ(SP->getLine(), (unsigned)4);
 }
 
 TEST_F(CloneModule, GlobalMetadata) {
   GlobalVariable *NewGV = NewM->getGlobalVariable("gv");
   EXPECT_NE(nullptr, NewGV->getMetadata(LLVMContext::MD_type));
 }
 
+TEST_F(CloneModule, GlobalDebugInfo) {
+  GlobalVariable *NewGV = NewM->getGlobalVariable("gv");
+  EXPECT_TRUE(NewGV != nullptr);
+
+  // Find debug info expression assigned to global
+  SmallVector<DIGlobalVariableExpression *, 1> GVs;
+  NewGV->getDebugInfo(GVs);
+  EXPECT_EQ(GVs.size(), 1U);
+
+  DIGlobalVariableExpression *GVExpr = GVs[0];
+  DIGlobalVariable *GV = GVExpr->getVariable();
+  EXPECT_TRUE(GV != nullptr);
+
+  EXPECT_EQ(GV->getName(), "gv");
+  EXPECT_EQ(GV->getLine(), 1U);
+
+  // Assert that the scope of the debug info attached to
+  // global variable matches the cloned function.
+  DISubprogram *SP = NewM->getFunction("f")->getSubprogram();
+  EXPECT_TRUE(SP != nullptr);
+  EXPECT_EQ(GV->getScope(), SP);
+}
+
+TEST_F(CloneModule, CompileUnit) {
+  // Find DICompileUnit listed in llvm.dbg.cu
+  auto *NMD = NewM->getNamedMetadata("llvm.dbg.cu");
+  EXPECT_TRUE(NMD != nullptr);
+  EXPECT_EQ(NMD->getNumOperands(), 1U);
+
+  DICompileUnit *CU = dyn_cast<llvm::DICompileUnit>(NMD->getOperand(0));
+  EXPECT_TRUE(CU != nullptr);
+
+  // Assert this CU is consistent with the cloned function debug info
+  DISubprogram *SP = NewM->getFunction("f")->getSubprogram();
+  EXPECT_TRUE(SP != nullptr);
+  EXPECT_EQ(SP->getUnit(), CU);
+
+  // Check globals listed in CU have the correct scope
+  DIGlobalVariableExpressionArray GlobalArray = CU->getGlobalVariables();
+  EXPECT_EQ(GlobalArray.size(), 2U);
+  for (DIGlobalVariableExpression *GVExpr : GlobalArray) {
+    DIGlobalVariable *GV = GVExpr->getVariable();
+    EXPECT_EQ(GV->getScope(), SP);
+  }
+}
+
 TEST_F(CloneModule, Comdat) {
   GlobalVariable *NewGV = NewM->getGlobalVariable("gv");
   auto *CD = NewGV->getComdat();
   ASSERT_NE(nullptr, CD);
   EXPECT_EQ("comdat", CD->getName());
   EXPECT_EQ(Comdat::ExactMatch, CD->getSelectionKind());
 
   Function *NewF = NewM->getFunction("f");
   EXPECT_EQ(CD, NewF->getComdat());
 }
 }