[Cloning] Fix to cloning DISubprograms.

[akhuang]

Summary

When trying to enable -debug-info-kind=constructor there was an assert
that occurs during debug info cloning ("mismatched subprogram between
llvm.dbg.value variable and !dbg attachment").
It appears that during llvm::CloneFunctionInto, a DISubprogram could be
duplicated when MapMetadata is called, and then added to the MD map again
when DIFinder gets a list of subprograms. This results in two different
versions of the DISubprogram.

This patch switches the order so that the DIFinder subprograms are
added before MapMetadata is called.

I'm not sure how to create a small IR testcase for this; there's a long-ish
c++ repro in the bug.

Fixes https://bugs.llvm.org/show_bug.cgi?id=46784

[dblaikie]

There are unit tests for CloneFunction in llvm/unittests/Transforms/Utils/CloningTest.cpp by the looks of it - perhaps this bug/fix can be tested by a new unit test there?

[aprantl]

Thanks! Having a test would be really important to document the expected functionality and prevent future regressions.
Every time we change something here, we tend to find new unexpected behavior in a very complex LTO build a couple months later ;-)

llvm/lib/Transforms/Utils/CloneFunction.cpp

193

Can you add a comment here explaining what this loop does?

[akhuang]

Add unit test and comment.

[dblaikie]

llvm/unittests/Transforms/Utils/CloningTest.cpp

740–744

This debug info metadata looks like it has two cycles (my_operator -> awaitables -> my_operator and test -> w -> test). Are they both necessary? do they both test different cases? Might be worth a few more words in the test comment above about what they're exercising/what aspects of them are relevant/necessary to reproduce the issue?

[akhuang]

llvm/unittests/Transforms/Utils/CloningTest.cpp

740–744

I don't think either is necessary, actually. The important part for reproducing is just that there's a path from test -> my_operator. Maybe I can get rid of some more stuff; this is just what the metadata happened to look like in the repro.

[dblaikie]

llvm/unittests/Transforms/Utils/CloningTest.cpp

740–744

Ah, so the critical path is "test" -> "w" -> anonymous type (!4) -> "my_operator" ?

I guess this could happen in real-world non-coroutine code with templates like this:

template<typename T>
void f1() {
  T t;
}
void f2() {
  struct x { };
  f1<x>();
}

(compiled with optimizations (well, might have to add noinline or optnone to 'f1' to get it through optimizations) that should produce 't' in 'f1's retained types list, etc)

In any case, yeah - documenting that chain & trimming down anything else (I guess removing "awaitables"/removing "my_operator"s retainedNodes list) would be good

[akhuang]

llvm/unittests/Transforms/Utils/CloningTest.cpp

740–744

I guess the test would be clearer if it didn't rely on the verifier check and directly checked the metadata. The only reason "awaitables" exists is to create the situation where its scope doesn't match the DILocation scope.

[dblaikie]

llvm/unittests/Transforms/Utils/CloningTest.cpp

740–744

Ah, yeah - if the output is wrong/problematic even when it's verifier-clean, might be better to test for that directly.

[akhuang]

llvm/unittests/Transforms/Utils/CloningTest.cpp

740–744

well, I guess the thing that causes the failure is that "awaitables" points to one duplicate of "my_operator" and the DILocation points to another duplicate of "my_operator". So this is probably about as simplified as the test can be (as in, I can't remove the retainedNodes = "awaitables" part of the metadata).

[akhuang]

Simplify test and add comments.

[dblaikie]

Sounds good - thanks!

llvm/lib/Transforms/Utils/CloneFunction.cpp

if (SP) {
     MD[SP->getType()].reset(SP->getType());
     MD[SP->getFile()].reset(SP->getFile());
     // If we're not cloning into the same module, no need to clone the
     // subprogram
     if (!MustCloneSP)
       MD[SP].reset(SP);
   }
 
-  SmallVector<std::pair<unsigned, MDNode *>, 1> MDs;
-  OldFunc->getAllMetadata(MDs);
-  for (auto MD : MDs) {
-    NewFunc->addMetadata(
-        MD.first,
-        *MapMetadata(MD.second, VMap,
-                     ModuleLevelChanges ? RF_None : RF_NoModuleLevelChanges,
-                     TypeMapper, Materializer));
-  }
-
   // Everything else beyond this point deals with function instructions,
   // so if we are dealing with a function declaration, we're done.
   if (OldFunc->isDeclaration())
     return;
 
   // When we remap instructions, we want to avoid duplicating inlined
   // DISubprograms, so record all subprograms we find as we duplicate
   // instructions and then freeze them in the MD map.
   // We also record information about dbg.value and dbg.declare to avoid
   // duplicating the types.
   DebugInfoFinder DIFinder;
 
   // Loop over all of the basic blocks in the function, cloning them as
   // appropriate.  Note that we save BE this way in order to handle cloning of
   // recursive functions into themselves.
-  //
   for (Function::const_iterator BI = OldFunc->begin(), BE = OldFunc->end();
        BI != BE; ++BI) {
     const BasicBlock &BB = *BI;
 
     // Create a new basic block and copy instructions into it!
     BasicBlock *CBB = CloneBasicBlock(&BB, VMap, NameSuffix, NewFunc, CodeInfo,
                                       ModuleLevelChanges ? &DIFinder : nullptr);
 
if (ISP != SP)
       VMap.MD()[ISP].reset(ISP);
 
   for (DICompileUnit *CU : DIFinder.compile_units())
     VMap.MD()[CU].reset(CU);
 
   for (DIType *Type : DIFinder.types())
     VMap.MD()[Type].reset(Type);
 
+  // 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,
+                     ModuleLevelChanges ? RF_None : RF_NoModuleLevelChanges,
+                     TypeMapper, Materializer));
+  }
+
   // Loop over all of the instructions in the function, fixing up operand
   // references as we go.  This uses VMap to do all the hard work.
   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)

llvm/unittests/Transforms/Utils/CloningTest.cpp

TEST(CloneFunction, CloneEmptyFunction) {
   auto *ImplFunction = ImplModule->getFunction("foo");
   EXPECT_TRUE(ImplFunction != nullptr);
   auto *DeclFunction = ImplModule->getFunction("bar");
   EXPECT_TRUE(DeclFunction != nullptr);
 
   ValueToValueMapTy VMap;
   SmallVector<ReturnInst *, 8> Returns;
   ClonedCodeInfo CCI;
-  CloneFunctionInto(DeclFunction, ImplFunction, VMap, true, Returns, "", &CCI);
+  CloneFunctionInto(ImplFunction, DeclFunction, VMap, true, Returns, "", &CCI);
 
   EXPECT_FALSE(verifyModule(*ImplModule, &errs()));
   EXPECT_FALSE(CCI.ContainsCalls);
   EXPECT_FALSE(CCI.ContainsDynamicAllocas);
 }
 
 TEST(CloneFunction, CloneFunctionWithInalloca) {
   StringRef ImplAssembly = R"(
TEST(CloneFunction, CloneFunctionWithInalloca) {
   ClonedCodeInfo CCI;
   CloneFunctionInto(DeclFunction, ImplFunction, VMap, true, Returns, "", &CCI);
 
   EXPECT_FALSE(verifyModule(*ImplModule, &errs()));
   EXPECT_TRUE(CCI.ContainsCalls);
   EXPECT_TRUE(CCI.ContainsDynamicAllocas);
 }
 
+TEST(CloneFunction, CloneFunctionWithSubprograms) {
+  // Tests that the debug info is duplicated correctly when a DISubprogram
+  // happens to be one of the operands of the DISubprogram that is being cloned.
+  // In general, operands of "test" that are distinct should be duplicated,
+  // but in this case "my_operator" should not be duplicated. If it is
+  // duplicated, the metadata in the llvm.dbg.declare could end up with
+  // different duplicates.
+  StringRef ImplAssembly = R"(
+    declare void @llvm.dbg.declare(metadata, metadata, metadata)
+
+    define void @test() !dbg !5 {
+      call void @llvm.dbg.declare(metadata i8* undef, metadata !4, metadata !DIExpression()), !dbg !6
+      ret void
+    }
+
+    declare void @cloned()
+
+    !llvm.dbg.cu = !{!0}
+    !llvm.module.flags = !{!2}
+    !0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1)
+    !1 = !DIFile(filename: "test.cpp",  directory: "")
+    !2 = !{i32 1, !"Debug Info Version", i32 3}
+    !3 = distinct !DISubprogram(name: "my_operator", scope: !1, unit: !0, retainedNodes: !{!4})
+    !4 = !DILocalVariable(name: "awaitables", scope: !3)
+    !5 = distinct !DISubprogram(name: "test", scope: !3, unit: !0)
+    !6 = !DILocation(line: 55, column: 15, scope: !3, inlinedAt: !7)
+    !7 = distinct !DILocation(line: 73, column: 14, scope: !5)
+  )";
+
+  LLVMContext Context;
+  SMDiagnostic Error;
+
+  auto ImplModule = parseAssemblyString(ImplAssembly, Error, Context);
+  EXPECT_TRUE(ImplModule != nullptr);
+  auto *OldFunc = ImplModule->getFunction("test");
+  EXPECT_TRUE(OldFunc != nullptr);
+  auto *NewFunc = ImplModule->getFunction("cloned");
+  EXPECT_TRUE(NewFunc != nullptr);
+
+  ValueToValueMapTy VMap;
+  SmallVector<ReturnInst *, 8> Returns;
+  ClonedCodeInfo CCI;
+  CloneFunctionInto(NewFunc, OldFunc, VMap, true, Returns, "", &CCI);
+
+  // This fails if the scopes in the llvm.dbg.declare variable and location
+  // aren't the same.
+  EXPECT_FALSE(verifyModule(*ImplModule, &errs()));
+}
+
 TEST(CloneFunction, CloneFunctionToDifferentModule) {
   StringRef ImplAssembly = R"(
     define void @foo() {
       ret void, !dbg !5
     }
 
     !llvm.module.flags = !{!0}
     !llvm.dbg.cu = !{!2, !6}