[Cloning] Take another pass at properly cloning debug info

[loladiro]

Summary

In rL302576, DISubprograms gained the constraint that a !dbg attachments to functions must
have a 1:1 mapping to DISubprograms. As part of that change, the function cloning support
was adjusted to attempt to enforce this invariant during cloning. However, there
were several problems with the implementation. Part of these were fixed in rL304079.
However, there was a more fundamental problem with these changes, namely that it
bypasses the matadata value map, causing the cloned metadata to be a mix of metadata
pointing to the new suprogram (where manual code was added to fix those up) and the
old suprogram (where this was not the case). This mismatch could cause a number of
different assertion failures in the DWARF emitter. Some of these are given at
https://github.com/JuliaLang/julia/issues/22069, but some others have been observed
as well. Attempt to rectify this by partially reverting the manual DI metadata fixup,
and instead using the standard value map approach. To retain the desired semantics
of not duplicating the compilation unit and inlined subprograms, explicitly freeze
these in the value map.

[GorNishanov]

Looks good from a coroutine side. Though I am not fluent in DebugInfo to LGTM the whole change.

One comment about "ModuleLevelChanges" parameter.
Is the description of that parameter accurately captures its purpose?

Namely:
/ If ModuleLevelChanges is false, VMap contains no non-identity GlobalValue
/ mappings.

[loladiro]

Well, note really, but that's sort of separate from this revision. A more accurate (though IMO still slightly confusing) description is the one that's on the flag:

/// If this flag is set, the remapper knows that only local values within a
/// function (such as an instruction or argument) are mapped, not global
/// values like functions and global metadata.
RF_NoModuleLevelChanges = 1,

[aprantl]

Thanks Keno, this appears to be a far better approach. The only question that remains is whether / how to update the linkage name of the cloned DISubprogram. But I don't think that this needs to be solved in this patch.

[loladiro]

FYI, reverted due to buildbot failure. Will investigate later today.

include/llvm/IR/DebugLoc.h

public:
     /// the chain now is inlined-at the new call site.
     /// \param   InlinedAt    The new outermost inlined-at in the chain.
     /// \param   ReplaceLast  Replace the last location in the inlined-at chain.
     static DebugLoc appendInlinedAt(DebugLoc DL, DILocation *InlinedAt,
                                     LLVMContext &Ctx,
                                     DenseMap<const MDNode *, MDNode *> &Cache,
                                     bool ReplaceLast = false);
 
-    /// Reparent all debug locations referenced by \c I that belong to \c OrigSP
-    /// to become (possibly indirect) children of \c NewSP.
-    static void reparentDebugInfo(Instruction &I, DISubprogram *OrigSP,
-                                  DISubprogram *NewSP,
-                                  DenseMap<const MDNode *, MDNode *> &Cache);
-
     unsigned getLine() const;
     unsigned getCol() const;
     MDNode *getScope() const;
     DILocation *getInlinedAt() const;
 
     /// \brief Get the fully inlined-at scope for a DebugLoc.
     ///
     /// Gets the inlined-at scope for a DebugLoc.

include/llvm/Transforms/Utils/Cloning.h

 
 namespace llvm {
 
 class AllocaInst;
 class BasicBlock;
 class BlockFrequencyInfo;
 class CallInst;
 class CallGraph;
+class DebugInfoFinder;
 class DominatorTree;
 class Function;
 class Instruction;
 class InvokeInst;
 class Loop;
 class LoopInfo;
 class Module;
 class ProfileSummaryInfo;

 /// function, you can specify it as the optional fourth parameter.
 ///
 /// If you would like to collect additional information about the cloned
 /// function, you can specify a ClonedCodeInfo object with the optional fifth
 /// parameter.
 ///
 BasicBlock *CloneBasicBlock(const BasicBlock *BB, ValueToValueMapTy &VMap,
                             const Twine &NameSuffix = "", Function *F = nullptr,
-                            ClonedCodeInfo *CodeInfo = nullptr);
+                            ClonedCodeInfo *CodeInfo = nullptr,
+                            DebugInfoFinder *DIFinder = nullptr);
 
 /// CloneFunction - 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.  The final argument captures information about the cloned code if

lib/IR/DebugLoc.cpp

DebugLoc DebugLoc::appendInlinedAt(DebugLoc DL, DILocation *InlinedAt,
   // map of already-constructed inlined-at nodes.
   for (const DILocation *MD : reverse(InlinedAtLocations))
     Cache[MD] = Last = DILocation::getDistinct(
         Ctx, MD->getLine(), MD->getColumn(), MD->getScope(), Last);
 
   return Last;
 }
 
-/// Reparent \c Scope from \c OrigSP to \c NewSP.
-static DIScope *reparentScope(LLVMContext &Ctx, DIScope *Scope,
-                              DISubprogram *OrigSP, DISubprogram *NewSP,
-                              DenseMap<const MDNode *, MDNode *> &Cache) {
-  SmallVector<DIScope *, 3> ScopeChain;
-  DIScope *Last = NewSP;
-  DIScope *CurScope = Scope;
-  do {
-    if (auto *SP = dyn_cast<DISubprogram>(CurScope)) {
-      // Don't rewrite this scope chain if it doesn't lead to the replaced SP.
-      if (SP != OrigSP)
-        return Scope;
-      Cache.insert({OrigSP, NewSP});
-      break;
-    }
-    if (auto *Found = Cache[CurScope]) {
-      Last = cast<DIScope>(Found);
-      break;
-    }
-    ScopeChain.push_back(CurScope);
-  } while ((CurScope = CurScope->getScope().resolve()));
-
-  // Starting from the top, rebuild the nodes to point to the new inlined-at
-  // location (then rebuilding the rest of the chain behind it) and update the
-  // map of already-constructed inlined-at nodes.
-  for (const DIScope *MD : reverse(ScopeChain)) {
-    if (auto *LB = dyn_cast<DILexicalBlock>(MD))
-      Cache[MD] = Last = DILexicalBlock::getDistinct(
-          Ctx, Last, LB->getFile(), LB->getLine(), LB->getColumn());
-    else if (auto *LB = dyn_cast<DILexicalBlockFile>(MD))
-      Cache[MD] = Last = DILexicalBlockFile::getDistinct(
-          Ctx, Last, LB->getFile(), LB->getDiscriminator());
-    else
-      llvm_unreachable("illegal parent scope");
-  }
-  return Last;
-}
-
-void DebugLoc::reparentDebugInfo(Instruction &I, DISubprogram *OrigSP,
-                                 DISubprogram *NewSP,
-                                 DenseMap<const MDNode *, MDNode *> &Cache) {
-  auto DL = I.getDebugLoc();
-  if (!OrigSP || !NewSP || OrigSP == NewSP || !DL)
-    return;
-
-  // Reparent the debug location.
-  auto &Ctx = I.getContext();
-  DILocation *InlinedAt = DL->getInlinedAt();
-  if (InlinedAt) {
-    while (auto *IA = InlinedAt->getInlinedAt())
-      InlinedAt = IA;
-    auto NewScope =
-        reparentScope(Ctx, InlinedAt->getScope(), OrigSP, NewSP, Cache);
-    InlinedAt =
-        DebugLoc::get(InlinedAt->getLine(), InlinedAt->getColumn(), NewScope);
-  }
-  I.setDebugLoc(
-      DebugLoc::get(DL.getLine(), DL.getCol(),
-                    reparentScope(Ctx, DL->getScope(), OrigSP, NewSP, Cache),
-                    DebugLoc::appendInlinedAt(DL, InlinedAt, Ctx, Cache,
-                                              ReplaceLastInlinedAt)));
-
-  // Fix up debug variables to point to NewSP.
-  auto reparentVar = [&](DILocalVariable *Var) {
-    return DILocalVariable::get(
-        Ctx,
-        cast<DILocalScope>(
-            reparentScope(Ctx, Var->getScope(), OrigSP, NewSP, Cache)),
-        Var->getName(), Var->getFile(), Var->getLine(), Var->getType(),
-        Var->getArg(), Var->getFlags(), Var->getAlignInBits());
-  };
-  if (auto *DbgValue = dyn_cast<DbgValueInst>(&I)) {
-    auto *Var = DbgValue->getVariable();
-    I.setOperand(2, MetadataAsValue::get(Ctx, reparentVar(Var)));
-  } else if (auto *DbgDeclare = dyn_cast<DbgDeclareInst>(&I)) {
-    auto *Var = DbgDeclare->getVariable();
-    I.setOperand(1, MetadataAsValue::get(Ctx, reparentVar(Var)));
-  }
-}
-
-
 #if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
 LLVM_DUMP_METHOD void DebugLoc::dump() const {
   if (!Loc)
     return;
 
   dbgs() << getLine();
   if (getCol() != 0)
     dbgs() << ',' << getCol();

lib/Transforms/Coroutines/CoroSplit.cpp

static Function *createClone(Function &F, Twine Suffix, coro::Shape &Shape,
   // Replace all args with undefs. The buildCoroutineFrame algorithm already
   // rewritten access to the args that occurs after suspend points with loads
   // and stores to/from the coroutine frame.
   for (Argument &A : F.args())
     VMap[&A] = UndefValue::get(A.getType());
 
   SmallVector<ReturnInst *, 4> Returns;
 
-  CloneFunctionInto(NewF, &F, VMap, /*ModuleLevelChanges=*/false, Returns);
+  CloneFunctionInto(NewF, &F, VMap, /*ModuleLevelChanges=*/true, Returns);
 
   // Remove old returns.
   for (ReturnInst *Return : Returns)
     changeToUnreachable(Return, /*UseLLVMTrap=*/false);
 
   // Remove old return attributes.
   NewF->removeAttributes(
       AttributeList::ReturnIndex,

lib/Transforms/Utils/CloneFunction.cpp

 #include "llvm/IR/Module.h"
 #include "llvm/Transforms/Utils/BasicBlockUtils.h"
 #include "llvm/Transforms/Utils/Local.h"
 #include "llvm/Transforms/Utils/ValueMapper.h"
 #include <map>
 using namespace llvm;
 
 /// See comments in Cloning.h.
-BasicBlock *llvm::CloneBasicBlock(const BasicBlock *BB,
-                                  ValueToValueMapTy &VMap,
+BasicBlock *llvm::CloneBasicBlock(const BasicBlock *BB, ValueToValueMapTy &VMap,
                                   const Twine &NameSuffix, Function *F,
-                                  ClonedCodeInfo *CodeInfo) {
+                                  ClonedCodeInfo *CodeInfo,
+                                  DebugInfoFinder *DIFinder) {
   DenseMap<const MDNode *, MDNode *> Cache;
   BasicBlock *NewBB = BasicBlock::Create(BB->getContext(), "", F);
   if (BB->hasName()) NewBB->setName(BB->getName()+NameSuffix);
 
   bool hasCalls = false, hasDynamicAllocas = false, hasStaticAllocas = false;
   
   // Loop over all instructions, and copy them over.
   for (BasicBlock::const_iterator II = BB->begin(), IE = BB->end();
        II != IE; ++II) {
+
+    if (DIFinder && F->getParent() && II->getDebugLoc())
+      DIFinder->processLocation(*F->getParent(), II->getDebugLoc().get());
+
     Instruction *NewInst = II->clone();
-    if (F && F->getSubprogram())
-      DebugLoc::reparentDebugInfo(*NewInst, BB->getParent()->getSubprogram(),
-                                  F->getSubprogram(), Cache);
     if (II->hasName())
       NewInst->setName(II->getName()+NameSuffix);
     NewBB->getInstList().push_back(NewInst);
     VMap[&*II] = NewInst; // Add instruction map to value.
 
     hasCalls |= (isa<CallInst>(II) && !isa<DbgInfoIntrinsic>(II));
     if (const AllocaInst *AI = dyn_cast<AllocaInst>(II)) {
       if (isa<ConstantInt>(AI->getArraySize()))
if (Argument *NewArg = dyn_cast<Argument>(VMap[&OldArg])) {
           OldAttrs.getParamAttributes(OldArg.getArgNo());
     }
   }
 
   NewFunc->setAttributes(
       AttributeList::get(NewFunc->getContext(), OldAttrs.getFnAttributes(),
                          OldAttrs.getRetAttributes(), NewArgAttrs));
 
+  bool MustCloneSP =
+      OldFunc->getParent() && OldFunc->getParent() == NewFunc->getParent();
+  DISubprogram *SP = OldFunc->getSubprogram();
+  if (SP) {
+    assert(!MustCloneSP || ModuleLevelChanges);
+    // Add mappings for some DebugInfo nodes that we don't want duplicated
+    // even if they're distinct.
+    auto &MD = VMap.MD();
+    MD[SP->getUnit()].reset(SP->getUnit());
+    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) {
-    MDNode *NewMD;
-    bool MustCloneSP =
-        (MD.first == LLVMContext::MD_dbg && OldFunc->getParent() &&
-         OldFunc->getParent() == NewFunc->getParent());
-    if (MustCloneSP) {
-      auto *SP = cast<DISubprogram>(MD.second);
-      NewMD = DISubprogram::getDistinct(
-          NewFunc->getContext(), SP->getScope(), SP->getName(),
-          SP->getLinkageName(), SP->getFile(), SP->getLine(), SP->getType(),
-          SP->isLocalToUnit(), SP->isDefinition(), SP->getScopeLine(),
-          SP->getContainingType(), SP->getVirtuality(), SP->getVirtualIndex(),
-          SP->getThisAdjustment(), SP->getFlags(), SP->isOptimized(),
-          SP->getUnit(), SP->getTemplateParams(), SP->getDeclaration(),
-          SP->getVariables(), SP->getThrownTypes());
-    } else
-      NewMD =
-          MapMetadata(MD.second, VMap,
-                      ModuleLevelChanges ? RF_None : RF_NoModuleLevelChanges,
-                      TypeMapper, Materializer);
-    NewFunc->addMetadata(MD.first, *NewMD);
+    NewFunc->addMetadata(
+        MD.first,
+        *MapMetadata(MD.second, VMap,
+                     TypeMapper, Materializer));
   }
 
+  // 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.
+  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);
+    BasicBlock *CBB = CloneBasicBlock(&BB, VMap, NameSuffix, NewFunc, CodeInfo,
+                                      SP ? &DIFinder : nullptr);
 
     // Add basic block mapping.
     VMap[&BB] = CBB;
 
     // It is only legal to clone a function if a block address within that
     // function is never referenced outside of the function.  Given that, we
     // want to map block addresses from the old function to block addresses in
     // the clone. (This is different from the generic ValueMapper
     // implementation, which generates an invalid blockaddress when
     // cloning a function.)
     if (BB.hasAddressTaken()) {
       Constant *OldBBAddr = BlockAddress::get(const_cast<Function*>(OldFunc),
                                               const_cast<BasicBlock*>(&BB));
       VMap[OldBBAddr] = BlockAddress::get(NewFunc, CBB);
     }
 
     // Note return instructions for the caller.
     if (ReturnInst *RI = dyn_cast<ReturnInst>(CBB->getTerminator()))
       Returns.push_back(RI);
   }
 
+  for (DISubprogram *ISP : DIFinder.subprograms()) {
+    if (ISP != SP) {
+      VMap.MD()[ISP].reset(ISP);
+    }
+  }
+
   // 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)
Function *llvm::CloneFunction(Function *F, ValueToValueMapTy &VMap,
   Function::arg_iterator DestI = NewF->arg_begin();
   for (const Argument & I : F->args())
     if (VMap.count(&I) == 0) {     // Is this argument preserved?
       DestI->setName(I.getName()); // Copy the name over...
       VMap[&I] = &*DestI++;        // Add mapping to VMap
     }
 
   SmallVector<ReturnInst*, 8> Returns;  // Ignore returns cloned.
-  CloneFunctionInto(NewF, F, VMap, /*ModuleLevelChanges=*/false, Returns, "",
+  CloneFunctionInto(NewF, F, VMap, F->getSubprogram() != nullptr, Returns, "",
                     CodeInfo);
 
   return NewF;
 }
 
 
 
 namespace {

unittests/Transforms/Utils/Cloning.cpp

 // Test that a new, distinct function was created.
 TEST_F(CloneFunc, NewFunctionCreated) {
   EXPECT_NE(OldFunc, NewFunc);
 }
 
 // Test that a new subprogram entry was added and is pointing to the new
 // function, while the original subprogram still points to the old one.
 TEST_F(CloneFunc, Subprogram) {
-  EXPECT_FALSE(verifyModule(*M));
+  EXPECT_FALSE(verifyModule(*M, &errs()));
   EXPECT_EQ(3U, Finder->subprogram_count());
   EXPECT_NE(NewFunc->getSubprogram(), OldFunc->getSubprogram());
 }
 
 // Test that instructions in the old function still belong to it in the
 // metadata, while instruction in the new function belong to the new one.
 TEST_F(CloneFunc, InstructionOwnership) {
   EXPECT_FALSE(verifyModule(*M));