From 66fb0d9768edf37c65c20e9d4dafc5d7f96669de Mon Sep 17 00:00:00 2001 From: Hans Wennborg Date: Tue, 9 May 2017 14:44:15 +0000 Subject: [PATCH] Revert r302469 "Make it illegal for two Functions to point to the same DISubprogram" This caused PR32977. Original commit message: > 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 > > > Differential Revision: https://reviews.llvm.org/D32975 llvm-svn: 302533 --- llvm/include/llvm/IR/DebugLoc.h | 16 ---- llvm/lib/IR/DebugLoc.cpp | 114 --------------------------- llvm/lib/IR/Verifier.cpp | 13 +-- llvm/lib/Transforms/Utils/CloneFunction.cpp | 32 ++------ llvm/lib/Transforms/Utils/InlineFunction.cpp | 43 ++++++++-- llvm/test/Verifier/metadata-function-dbg.ll | 16 ++-- llvm/unittests/Transforms/Utils/Cloning.cpp | 65 +++++++-------- 7 files changed, 79 insertions(+), 220 deletions(-) diff --git a/llvm/include/llvm/IR/DebugLoc.h b/llvm/include/llvm/IR/DebugLoc.h index aa74f36..202be3d 100644 --- a/llvm/include/llvm/IR/DebugLoc.h +++ b/llvm/include/llvm/IR/DebugLoc.h @@ -80,22 +80,6 @@ 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 &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 &Cache); - unsigned getLine() const; unsigned getCol() const; MDNode *getScope() const; diff --git a/llvm/lib/IR/DebugLoc.cpp b/llvm/lib/IR/DebugLoc.cpp index 11beb22..f31074a 100644 --- a/llvm/lib/IR/DebugLoc.cpp +++ b/llvm/lib/IR/DebugLoc.cpp @@ -8,7 +8,6 @@ //===----------------------------------------------------------------------===// #include "llvm/IR/DebugLoc.h" -#include "llvm/IR/IntrinsicInst.h" #include "LLVMContextImpl.h" #include "llvm/IR/DebugInfo.h" using namespace llvm; @@ -67,119 +66,6 @@ DebugLoc DebugLoc::get(unsigned Line, unsigned Col, const MDNode *Scope, const_cast(InlinedAt)); } -DebugLoc DebugLoc::appendInlinedAt(DebugLoc DL, DILocation *InlinedAt, - LLVMContext &Ctx, - DenseMap &Cache, - bool ReplaceLast) { - SmallVector 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(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 &Cache) { - SmallVector ScopeChain; - DIScope *Last = NewSP; - DIScope *CurScope = Scope; - do { - if (auto *SP = dyn_cast(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(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(MD)) - Cache[MD] = Last = DILexicalBlock::getDistinct( - Ctx, Last, LB->getFile(), LB->getLine(), LB->getColumn()); - else if (auto *LB = dyn_cast(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 &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( - 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(&I)) { - auto *Var = DbgValue->getVariable(); - I.setOperand(2, MetadataAsValue::get(Ctx, reparentVar(Var))); - } else if (auto *DbgDeclare = dyn_cast(&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) diff --git a/llvm/lib/IR/Verifier.cpp b/llvm/lib/IR/Verifier.cpp index 3b68d63..65e1245 100644 --- a/llvm/lib/IR/Verifier.cpp +++ b/llvm/lib/IR/Verifier.cpp @@ -267,9 +267,6 @@ class Verifier : public InstVisitor, VerifierSupport { /// \brief Keep track of the metadata nodes that have been checked already. SmallPtrSet MDNodes; - /// Keep track which DISubprogram is attached to which function. - DenseMap DISubprogramAttachments; - /// Track all DICompileUnits visited. SmallPtrSet CUVisited; @@ -389,7 +386,7 @@ public: verifyCompileUnits(); verifyDeoptimizeCallingConvs(); - DISubprogramAttachments.clear(); + return !Broken; } @@ -2088,19 +2085,13 @@ 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(I.second), "function !dbg attachment must be a subprogram", &F, I.second); - auto *SP = cast(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, diff --git a/llvm/lib/Transforms/Utils/CloneFunction.cpp b/llvm/lib/Transforms/Utils/CloneFunction.cpp index 4aa26fd..d5124ac 100644 --- a/llvm/lib/Transforms/Utils/CloneFunction.cpp +++ b/llvm/lib/Transforms/Utils/CloneFunction.cpp @@ -41,7 +41,6 @@ BasicBlock *llvm::CloneBasicBlock(const BasicBlock *BB, ValueToValueMapTy &VMap, const Twine &NameSuffix, Function *F, ClonedCodeInfo *CodeInfo) { - DenseMap Cache; BasicBlock *NewBB = BasicBlock::Create(BB->getContext(), "", F); if (BB->hasName()) NewBB->setName(BB->getName()+NameSuffix); @@ -51,9 +50,6 @@ 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); @@ -124,28 +120,12 @@ void llvm::CloneFunctionInto(Function *NewFunc, const Function *OldFunc, SmallVector, 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(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); - } + for (auto MD : MDs) + NewFunc->addMetadata( + MD.first, + *MapMetadata(MD.second, VMap, + ModuleLevelChanges ? RF_None : RF_NoModuleLevelChanges, + TypeMapper, Materializer)); // 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 diff --git a/llvm/lib/Transforms/Utils/InlineFunction.cpp b/llvm/lib/Transforms/Utils/InlineFunction.cpp index bafccaa..6d56e08 100644 --- a/llvm/lib/Transforms/Utils/InlineFunction.cpp +++ b/llvm/lib/Transforms/Utils/InlineFunction.cpp @@ -1302,6 +1302,41 @@ 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 &IANodes) { + SmallVector 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. @@ -1329,16 +1364,14 @@ 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 IANodes; + DenseMap IANodes; for (; FI != Fn->end(); ++FI) { for (BasicBlock::iterator BI = FI->begin(), BE = FI->end(); BI != BE; ++BI) { if (DebugLoc DL = BI->getDebugLoc()) { - auto IA = DebugLoc::appendInlinedAt(DL, InlinedAtNode, BI->getContext(), - IANodes); - auto IDL = DebugLoc::get(DL.getLine(), DL.getCol(), DL.getScope(), IA); - BI->setDebugLoc(IDL); + BI->setDebugLoc( + updateInlinedAtInfo(DL, InlinedAtNode, BI->getContext(), IANodes)); continue; } diff --git a/llvm/test/Verifier/metadata-function-dbg.ll b/llvm/test/Verifier/metadata-function-dbg.ll index 6db4094..24989ed 100644 --- a/llvm/test/Verifier/metadata-function-dbg.ll +++ b/llvm/test/Verifier/metadata-function-dbg.ll @@ -3,18 +3,12 @@ ; CHECK: function declaration may not have a !dbg attachment declare !dbg !4 void @f1() -; CHECK: function must have a single !dbg attachment -define void @f2() !dbg !4 !dbg !4 { +define void @f2() !dbg !4 { unreachable } -; CHECK: DISubprogram attached to more than one function -define void @f3() !dbg !4 { - unreachable -} - -; CHECK: DISubprogram attached to more than one function -define void @f4() !dbg !4 { +; CHECK: function must have a single !dbg attachment +define void @f3() !dbg !4 !dbg !4 { unreachable } @@ -22,7 +16,7 @@ define void @f4() !dbg !4 { ; CHECK: function !dbg attachment must be a subprogram ; CHECK-NEXT: void ()* @bar ; CHECK-NEXT: !{{[0-9]+}} = !{} -define void @bar() !dbg !3 { +define void @bar() !dbg !6 { unreachable } @@ -32,5 +26,5 @@ define void @bar() !dbg !3 { !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 = !{} diff --git a/llvm/unittests/Transforms/Utils/Cloning.cpp b/llvm/unittests/Transforms/Utils/Cloning.cpp index 83f146d..2f4ee86 100644 --- a/llvm/unittests/Transforms/Utils/Cloning.cpp +++ b/llvm/unittests/Transforms/Utils/Cloning.cpp @@ -296,6 +296,7 @@ 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); @@ -305,25 +306,12 @@ protected: auto *DL = DILocation::get(Subprogram->getContext(), 5, 0, Subprogram); DBuilder.insertDeclare(Alloca, Variable, E, DL, Store); DBuilder.insertDbgValueIntrinsic(AllocaContent, 0, Variable, E, DL, - Entry); - // 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. + Terminator); + // 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"), @@ -357,8 +345,15 @@ 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()); - EXPECT_NE(NewFunc->getSubprogram(), OldFunc->getSubprogram()); + + unsigned SubprogramCount = Finder->subprogram_count(); + EXPECT_EQ(1U, SubprogramCount); + + auto Iter = Finder->subprograms().begin(); + auto *Sub = cast(*Iter); + + EXPECT_TRUE(Sub == OldFunc->getSubprogram()); + EXPECT_TRUE(Sub == NewFunc->getSubprogram()); } // Test that instructions in the old function still belong to it in the @@ -385,8 +380,8 @@ TEST_F(CloneFunc, InstructionOwnership) { EXPECT_EQ(OldDL.getCol(), NewDL.getCol()); // But that they belong to different functions - auto *OldSubprogram = cast(OldDL.getInlinedAtScope()); - auto *NewSubprogram = cast(NewDL.getInlinedAtScope()); + auto *OldSubprogram = cast(OldDL.getScope()); + auto *NewSubprogram = cast(NewDL.getScope()); EXPECT_EQ(OldFunc->getSubprogram(), OldSubprogram); EXPECT_EQ(NewFunc->getSubprogram(), NewSubprogram); } @@ -421,26 +416,22 @@ TEST_F(CloneFunc, DebugIntrinsics) { EXPECT_EQ(NewFunc, cast(NewIntrin->getAddress())-> getParent()->getParent()); - if (!OldIntrin->getDebugLoc()->getInlinedAt()) { - // Old variable must belong to the old function. - EXPECT_EQ(OldFunc->getSubprogram(), - cast(OldIntrin->getVariable()->getScope())); - // New variable must belong to the new function. - EXPECT_EQ(NewFunc->getSubprogram(), - cast(NewIntrin->getVariable()->getScope())); - } + // Old variable must belong to the old function + EXPECT_EQ(OldFunc->getSubprogram(), + cast(OldIntrin->getVariable()->getScope())); + // New variable must belong to the New function + EXPECT_EQ(NewFunc->getSubprogram(), + cast(NewIntrin->getVariable()->getScope())); } else if (DbgValueInst* OldIntrin = dyn_cast(&OldI)) { DbgValueInst* NewIntrin = dyn_cast(&NewI); EXPECT_TRUE(NewIntrin); - if (!OldIntrin->getDebugLoc()->getInlinedAt()) { - // Old variable must belong to the old function. - EXPECT_EQ(OldFunc->getSubprogram(), - cast(OldIntrin->getVariable()->getScope())); - // New variable must belong to the new function. - EXPECT_EQ(NewFunc->getSubprogram(), - cast(NewIntrin->getVariable()->getScope())); - } + // Old variable must belong to the old function + EXPECT_EQ(OldFunc->getSubprogram(), + cast(OldIntrin->getVariable()->getScope())); + // New variable must belong to the New function + EXPECT_EQ(NewFunc->getSubprogram(), + cast(NewIntrin->getVariable()->getScope())); } ++OldIter; -- 2.7.4