Make it illegal for two Functions to point to the same DISubprogram

As recently discussed on llvm-dev [1], this patch makes it illegal for
two Functions to point to the same DISubprogram and updates
FunctionCloner to also clone the debug info of a function to conform
to the new requirement. To simplify the implementation it also factors
out the creation of inlineAt locations from the Inliner into a
general-purpose utility in DILocation.

[1] http://lists.llvm.org/pipermail/llvm-dev/2017-May/112661.html
<rdar://problem/31926379>

Differential Revision: https://reviews.llvm.org/D32975

llvm-svn: 302469

include/llvm/IR/DebugLoc.h

@@ -80,6 +80,22 @@ namespace llvm {
     static DebugLoc get(unsigned Line, unsigned Col, const MDNode *Scope,
                         const MDNode *InlinedAt = nullptr);
 
+    enum { ReplaceLastInlinedAt = true };
+    /// Rebuild the entire inlined-at chain for this instruction so that the top of
+    /// 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;

lib/IR/DebugLoc.cpp

@@ -8,6 +8,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "llvm/IR/DebugLoc.h"
+#include "llvm/IR/IntrinsicInst.h"
 #include "LLVMContextImpl.h"
 #include "llvm/IR/DebugInfo.h"
 using namespace llvm;
@@ -66,6 +67,119 @@ DebugLoc DebugLoc::get(unsigned Line, unsigned Col, const MDNode *Scope,
                          const_cast<MDNode *>(InlinedAt));
 }
 
+DebugLoc DebugLoc::appendInlinedAt(DebugLoc DL, DILocation *InlinedAt,
+                                   LLVMContext &Ctx,
+                                   DenseMap<const MDNode *, MDNode *> &Cache,
+                                   bool ReplaceLast) {
+  SmallVector<DILocation *, 3> InlinedAtLocations;
+  DILocation *Last = InlinedAt;
+  DILocation *CurInlinedAt = DL;
+
+  // Gather all the inlined-at nodes.
+  while (DILocation *IA = CurInlinedAt->getInlinedAt()) {
+    // Skip any we've already built nodes for.
+    if (auto *Found = Cache[IA]) {
+      Last = cast<DILocation>(Found);
+      break;
+    }
+
+    if (ReplaceLast && !IA->getInlinedAt())
+      break;
+    InlinedAtLocations.push_back(IA);
+    CurInlinedAt = IA;
+  }
+
+  // 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 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 || !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::getDistinct(
+        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)

lib/IR/Verifier.cpp

@@ -267,6 +267,9 @@ class Verifier : public InstVisitor<Verifier>, VerifierSupport {
   /// \brief Keep track of the metadata nodes that have been checked already.
   SmallPtrSet<const Metadata *, 32> MDNodes;
 
+  /// Keep track which DISubprogram is attached to which function.
+  DenseMap<const DISubprogram *, const Function *> DISubprogramAttachments;
+
   /// Track all DICompileUnits visited.
   SmallPtrSet<const Metadata *, 2> CUVisited;
 
@@ -386,7 +389,7 @@ public:
     verifyCompileUnits();
 
     verifyDeoptimizeCallingConvs();
-
+    DISubprogramAttachments.clear();
     return !Broken;
   }
 
@@ -2085,13 +2088,19 @@ void Verifier::visitFunction(const Function &F) {
       switch (I.first) {
       default:
         break;
-      case LLVMContext::MD_dbg:
+      case LLVMContext::MD_dbg: {
         ++NumDebugAttachments;
         AssertDI(NumDebugAttachments == 1,
                  "function must have a single !dbg attachment", &F, I.second);
         AssertDI(isa<DISubprogram>(I.second),
                  "function !dbg attachment must be a subprogram", &F, I.second);
+        auto *SP = cast<DISubprogram>(I.second);
+        const Function *&AttachedTo = DISubprogramAttachments[SP];
+        AssertDI(!AttachedTo || AttachedTo == &F,
+                 "DISubprogram attached to more than one function", SP, &F);
+        AttachedTo = &F;
         break;
+      }
       case LLVMContext::MD_prof:
         ++NumProfAttachments;
         Assert(NumProfAttachments == 1,

lib/Transforms/Utils/CloneFunction.cpp

@@ -41,6 +41,7 @@ BasicBlock *llvm::CloneBasicBlock(const BasicBlock *BB,
                                   ValueToValueMapTy &VMap,
                                   const Twine &NameSuffix, Function *F,
                                   ClonedCodeInfo *CodeInfo) {
+  DenseMap<const MDNode *, MDNode *> Cache;
   BasicBlock *NewBB = BasicBlock::Create(BB->getContext(), "", F);
   if (BB->hasName()) NewBB->setName(BB->getName()+NameSuffix);
 
@@ -50,6 +51,9 @@ BasicBlock *llvm::CloneBasicBlock(const BasicBlock *BB,
   for (BasicBlock::const_iterator II = BB->begin(), IE = BB->end();
        II != IE; ++II) {
     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);
@@ -120,12 +124,28 @@ void llvm::CloneFunctionInto(Function *NewFunc, const Function *OldFunc,
 
   SmallVector<std::pair<unsigned, MDNode *>, 1> MDs;
   OldFunc->getAllMetadata(MDs);
-  for (auto MD : MDs)
+  for (auto MD : MDs) {
-    NewFunc->addMetadata(
+    MDNode *NewMD;
-        MD.first,
+    bool MustCloneSP =
-        *MapMetadata(MD.second, VMap,
+        (MD.first == LLVMContext::MD_dbg && OldFunc->getParent() &&
-                     ModuleLevelChanges ? RF_None : RF_NoModuleLevelChanges,
+         OldFunc->getParent() == NewFunc->getParent());
-                     TypeMapper, Materializer));
+    if (MustCloneSP) {
+      auto *SP = cast<DISubprogram>(MD.second);
+      NewMD = DISubprogram::getDistinct(
+          NewFunc->getContext(), SP->getScope(), SP->getName(),
+          NewFunc->getName(), 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);
+  }
 
   // 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

lib/Transforms/Utils/InlineFunction.cpp

@@ -1302,41 +1302,6 @@ static bool hasLifetimeMarkers(AllocaInst *AI) {
   return false;
 }
 
-/// Rebuild the entire inlined-at chain for this instruction so that the top of
-/// the chain now is inlined-at the new call site.
-static DebugLoc
-updateInlinedAtInfo(const DebugLoc &DL, DILocation *InlinedAtNode,
-                    LLVMContext &Ctx,
-                    DenseMap<const DILocation *, DILocation *> &IANodes) {
-  SmallVector<DILocation *, 3> InlinedAtLocations;
-  DILocation *Last = InlinedAtNode;
-  DILocation *CurInlinedAt = DL;
-
-  // Gather all the inlined-at nodes
-  while (DILocation *IA = CurInlinedAt->getInlinedAt()) {
-    // Skip any we've already built nodes for
-    if (DILocation *Found = IANodes[IA]) {
-      Last = Found;
-      break;
-    }
-
-    InlinedAtLocations.push_back(IA);
-    CurInlinedAt = IA;
-  }
-
-  // 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 DILocation *MD : reverse(InlinedAtLocations)) {
-    Last = IANodes[MD] = DILocation::getDistinct(
-        Ctx, MD->getLine(), MD->getColumn(), MD->getScope(), Last);
-  }
-
-  // And finally create the normal location for this instruction, referring to
-  // the new inlined-at chain.
-  return DebugLoc::get(DL.getLine(), DL.getCol(), DL.getScope(), Last);
-}
-
 /// Return the result of AI->isStaticAlloca() if AI were moved to the entry
 /// block. Allocas used in inalloca calls and allocas of dynamic array size
 /// cannot be static.
@@ -1364,14 +1329,16 @@ static void fixupLineNumbers(Function *Fn, Function::iterator FI,
   // Cache the inlined-at nodes as they're built so they are reused, without
   // this every instruction's inlined-at chain would become distinct from each
   // other.
-  DenseMap<const DILocation *, DILocation *> IANodes;
+  DenseMap<const MDNode *, MDNode *> IANodes;
 
   for (; FI != Fn->end(); ++FI) {
     for (BasicBlock::iterator BI = FI->begin(), BE = FI->end();
          BI != BE; ++BI) {
       if (DebugLoc DL = BI->getDebugLoc()) {
-        BI->setDebugLoc(
+        auto IA = DebugLoc::appendInlinedAt(DL, InlinedAtNode, BI->getContext(),
-            updateInlinedAtInfo(DL, InlinedAtNode, BI->getContext(), IANodes));
+                                            IANodes);
+        auto IDL = DebugLoc::get(DL.getLine(), DL.getCol(), DL.getScope(), IA);
+        BI->setDebugLoc(IDL);
         continue;
       }
 

test/Verifier/metadata-function-dbg.ll

@@ -3,12 +3,18 @@
 ; CHECK:      function declaration may not have a !dbg attachment
 declare !dbg !4 void @f1()
 
-define void @f2() !dbg !4 {
+; CHECK:      function must have a single !dbg attachment
+define void @f2() !dbg !4 !dbg !4 {
   unreachable
 }
 
-; CHECK:      function must have a single !dbg attachment
+; CHECK:      DISubprogram attached to more than one function
-define void @f3() !dbg !4 !dbg !4 {
+define void @f3() !dbg !4 {
+  unreachable
+}
+
+; CHECK:      DISubprogram attached to more than one function
+define void @f4() !dbg !4 {
   unreachable
 }
 
@@ -16,7 +22,7 @@ define void @f3() !dbg !4 !dbg !4 {
 ; CHECK:      function !dbg attachment must be a subprogram
 ; CHECK-NEXT: void ()* @bar
 ; CHECK-NEXT: !{{[0-9]+}} = !{}
-define void @bar() !dbg !6 {
+define void @bar() !dbg !3 {
   unreachable
 }
 
@@ -26,5 +32,5 @@ define void @bar() !dbg !6 {
 !llvm.dbg.cu = !{!1}
 !1 = distinct !DICompileUnit(language: DW_LANG_C99, file: !2)
 !2 = !DIFile(filename: "t.c", directory: "/path/to/dir")
+!3 = !{}
 !4 = distinct !DISubprogram(name: "foo", scope: !1, file: !2, unit: !1)
-!6 = !{}

unittests/Transforms/Utils/Cloning.cpp

@@ -296,7 +296,6 @@ protected:
     Value* AllocaContent = IBuilder.getInt32(1);
     Instruction* Store = IBuilder.CreateStore(AllocaContent, Alloca);
     IBuilder.SetCurrentDebugLocation(DebugLoc::get(5, 2, Subprogram));
-    Instruction* Terminator = IBuilder.CreateRetVoid();
 
     // Create a local variable around the alloca
     auto *IntType = DBuilder.createBasicType("int", 32, dwarf::DW_ATE_signed);
@@ -306,12 +305,25 @@ protected:
     auto *DL = DILocation::get(Subprogram->getContext(), 5, 0, Subprogram);
     DBuilder.insertDeclare(Alloca, Variable, E, DL, Store);
     DBuilder.insertDbgValueIntrinsic(AllocaContent, 0, Variable, E, DL,
-                                     Terminator);
+                                     Entry);
-    // Finalize the debug info
+    // Also create an inlined variable.
+    auto *InlinedSP =
+        DBuilder.createFunction(CU, "inlined", "inlined", File, 8, FuncType,
+                                true, true, 9, DINode::FlagZero, false);
+    auto *InlinedVar =
+        DBuilder.createAutoVariable(InlinedSP, "inlined", File, 5, IntType, true);
+    auto *Scope = DBuilder.createLexicalBlock(
+        DBuilder.createLexicalBlockFile(InlinedSP, File), File, 1, 1);
+    auto InlinedDL =
+        DebugLoc::get(9, 4, Scope, DebugLoc::get(5, 2, Subprogram));
+    IBuilder.SetCurrentDebugLocation(InlinedDL);
+    DBuilder.insertDeclare(Alloca, InlinedVar, E, InlinedDL, Store);
+    IBuilder.CreateStore(IBuilder.getInt32(2), Alloca);
+    // Finalize the debug info.
     DBuilder.finalize();
+    IBuilder.CreateRetVoid();
 
-
+    // Create another, empty, compile unit.
-    // Create another, empty, compile unit
     DIBuilder DBuilder2(*M);
     DBuilder2.createCompileUnit(dwarf::DW_LANG_C99,
                                 DBuilder.createFile("extra.c", "/file/dir"),
@@ -345,15 +357,8 @@ TEST_F(CloneFunc, NewFunctionCreated) {
 // function, while the original subprogram still points to the old one.
 TEST_F(CloneFunc, Subprogram) {
   EXPECT_FALSE(verifyModule(*M));
-
+  EXPECT_EQ(3U, Finder->subprogram_count());
-  unsigned SubprogramCount = Finder->subprogram_count();
+  EXPECT_NE(NewFunc->getSubprogram(), OldFunc->getSubprogram());
-  EXPECT_EQ(1U, SubprogramCount);
-
-  auto Iter = Finder->subprograms().begin();
-  auto *Sub = cast<DISubprogram>(*Iter);
-
-  EXPECT_TRUE(Sub == OldFunc->getSubprogram());
-  EXPECT_TRUE(Sub == NewFunc->getSubprogram());
 }
 
 // Test that instructions in the old function still belong to it in the
@@ -380,8 +385,8 @@ TEST_F(CloneFunc, InstructionOwnership) {
       EXPECT_EQ(OldDL.getCol(), NewDL.getCol());
 
       // But that they belong to different functions
-      auto *OldSubprogram = cast<DISubprogram>(OldDL.getScope());
+      auto *OldSubprogram = cast<DISubprogram>(OldDL.getInlinedAtScope());
-      auto *NewSubprogram = cast<DISubprogram>(NewDL.getScope());
+      auto *NewSubprogram = cast<DISubprogram>(NewDL.getInlinedAtScope());
       EXPECT_EQ(OldFunc->getSubprogram(), OldSubprogram);
       EXPECT_EQ(NewFunc->getSubprogram(), NewSubprogram);
     }
@@ -416,22 +421,26 @@ TEST_F(CloneFunc, DebugIntrinsics) {
       EXPECT_EQ(NewFunc, cast<AllocaInst>(NewIntrin->getAddress())->
                          getParent()->getParent());
 
-      // Old variable must belong to the old function
+      if (!OldIntrin->getDebugLoc()->getInlinedAt()) {
-      EXPECT_EQ(OldFunc->getSubprogram(),
+        // Old variable must belong to the old function.
-                cast<DISubprogram>(OldIntrin->getVariable()->getScope()));
+        EXPECT_EQ(OldFunc->getSubprogram(),
-      // New variable must belong to the New function
+                  cast<DISubprogram>(OldIntrin->getVariable()->getScope()));
-      EXPECT_EQ(NewFunc->getSubprogram(),
+        // New variable must belong to the new function.
-                cast<DISubprogram>(NewIntrin->getVariable()->getScope()));
+        EXPECT_EQ(NewFunc->getSubprogram(),
+                  cast<DISubprogram>(NewIntrin->getVariable()->getScope()));
+      }
     } else if (DbgValueInst* OldIntrin = dyn_cast<DbgValueInst>(&OldI)) {
       DbgValueInst* NewIntrin = dyn_cast<DbgValueInst>(&NewI);
       EXPECT_TRUE(NewIntrin);
 
-      // Old variable must belong to the old function
+      if (!OldIntrin->getDebugLoc()->getInlinedAt()) {
-      EXPECT_EQ(OldFunc->getSubprogram(),
+        // Old variable must belong to the old function.
-                cast<DISubprogram>(OldIntrin->getVariable()->getScope()));
+        EXPECT_EQ(OldFunc->getSubprogram(),
-      // New variable must belong to the New function
+                  cast<DISubprogram>(OldIntrin->getVariable()->getScope()));
-      EXPECT_EQ(NewFunc->getSubprogram(),
+        // New variable must belong to the new function.
-                cast<DISubprogram>(NewIntrin->getVariable()->getScope()));
+        EXPECT_EQ(NewFunc->getSubprogram(),
+                  cast<DISubprogram>(NewIntrin->getVariable()->getScope()));
+      }
     }
 
     ++OldIter;