[Cloning] Clone metadata on function declarations

[aeubanks]

Summary

Previously we missed cloning metadata on function declarations because
we don't call CloneFunctionInto() on declarations in CloneModule().

[dexonsmith]

Huh... when did metadata get added to function declarations? I'm not sure what the semantics of those are supposed to be. I seem to remember that been left out when metadata was added for definitions.

[Meinersbur]

In D113812#3129072, @dexonsmith wrote:

Huh... when did metadata get added to function declarations?

Since D21052

[aeubanks]

ping

[dexonsmith]

In D113812#3132357, @Meinersbur wrote:

In D113812#3129072, @dexonsmith wrote:

Huh... when did metadata get added to function declarations?

Since D21052

Thanks, that rings a bell now.

In D113812#3161886, @aeubanks wrote:

ping

LGTM! I left a rambling comment inline but I'm not sure there's anything to be done with it.

llvm/lib/Transforms/Utils/CloneModule.cpp

143–146

I was initially worried about this skipping the DebugInfoFinder logic mentioned in this comment from CloneFunctionInto:

// Duplicate the metadata that is attached to the cloned function.
// Subprograms/CUs/types that were already mapped to themselves won't be
// duplicated.
SmallVector<std::pair<unsigned, MDNode *>, 1> MDs;
OldFunc->getAllMetadata(MDs);
for (auto MD : MDs) {
  NewFunc->addMetadata(MD.first, *MapMetadata(MD.second, VMap, RemapFlag,
                                              TypeMapper, Materializer));
}

Looking at that logic closely, I think it's okay to skip. If the new location is a different module, the DebugInfoFinder is only used for walking the instructions (of which a declaration has none). Only if it's in the same module does it walk the function's attachments.

I thought about trying to put that logic in a function that would be reused here (but be a no-op) in case the logic changes over time. But given that this is a new module, I think it's really unlikely to be useful and would be distracting.

I wonder if there's a comment that should be left somewhere, but I'm not sure what it would say. Maybe it's just obvious that for a new module you don't want special logic like that.

llvm/lib/Transforms/Utils/CloneModule.cpp

if (G.hasInitializer())
       GV->setInitializer(MapValue(G.getInitializer(), VMap));
 
     copyComdat(GV, &G);
   }
 
   // Similarly, copy over function bodies now...
   //
   for (const Function &I : M) {
-    if (I.isDeclaration())
+    Function *F = cast<Function>(VMap[&I]);
+
+    if (I.isDeclaration()) {
+      // Copy over metadata for declarations since we're not doing it below in
+      // CloneFunctionInto().
+      SmallVector<std::pair<unsigned, MDNode *>, 1> MDs;
+      I.getAllMetadata(MDs);
+      for (auto MD : MDs)
+        F->addMetadata(MD.first, *MapMetadata(MD.second, VMap));
       continue;
+    }
 
-    Function *F = cast<Function>(VMap[&I]);
     if (!ShouldCloneDefinition(&I)) {
       // Skip after setting the correct linkage for an external reference.
       F->setLinkage(GlobalValue::ExternalLinkage);
       // Personality function is not valid on a declaration.
       F->setPersonalityFn(nullptr);
       continue;
     }
 

llvm/unittests/Transforms/Utils/CloningTest.cpp

void CreateOldModule() {
     DBuilder.createGlobalVariableExpression(
         Subprogram, "unattached", "unattached", File, 1,
         DBuilder.createNullPtrType(), false, true, Expr);
 
     auto *Entry = BasicBlock::Create(C, "", F);
     IBuilder.SetInsertPoint(Entry);
     IBuilder.CreateRetVoid();
 
+    auto *G =
+        Function::Create(FuncType, GlobalValue::ExternalLinkage, "g", OldM);
+    G->addMetadata(LLVMContext::MD_type, *MDNode::get(C, {}));
+
     // Finalize the debug info
     DBuilder.finalize();
   }
 
   void CreateNewModule() { NewM = llvm::CloneModule(*OldM).release(); }
 
   LLVMContext C;
   Module *OldM;
   Module *NewM;
 };
 
 TEST_F(CloneModule, Verify) {
   // Confirm the old module is (still) valid.
-  EXPECT_FALSE(verifyModule(*OldM));
+  EXPECT_FALSE(verifyModule(*OldM, &errs()));
 
   // Check the new module.
-  EXPECT_FALSE(verifyModule(*NewM));
+  EXPECT_FALSE(verifyModule(*NewM, &errs()));
 }
 
 TEST_F(CloneModule, OldModuleUnchanged) {
   DebugInfoFinder Finder;
   Finder.processModule(*OldM);
   EXPECT_EQ(1U, Finder.subprogram_count());
 }
 
 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);
 }
 
+TEST_F(CloneModule, FunctionDeclarationMetadata) {
+  Function *NewF = NewM->getFunction("g");
+  EXPECT_NE(nullptr, NewF->getMetadata(LLVMContext::MD_type));
+}
+
 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);