From: David Blaikie Date: Fri, 14 Aug 2015 01:26:19 +0000 (+0000) Subject: unique_ptrify ConsumedBlockInfo analysis to make it move assignable X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=dbe3712f62fc845cf06209f1a488b87b2c9dc96c;p=clang unique_ptrify ConsumedBlockInfo analysis to make it move assignable ConsumedBlockInfo objects were move assigned, but only in a state where the dtor was a no-op anyway. Subtle and easily could've happened in ways that wouldn't've been safe - so this change makes it safe no matter what state the ConsumedBlockInfo object is in. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@244998 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/include/clang/Analysis/Analyses/Consumed.h b/include/clang/Analysis/Analyses/Consumed.h index b511ca41e5..1f5aa12111 100644 --- a/include/clang/Analysis/Analyses/Consumed.h +++ b/include/clang/Analysis/Analyses/Consumed.h @@ -162,8 +162,8 @@ namespace consumed { ConsumedState getState(const CXXBindTemporaryExpr *Tmp) const; /// \brief Merge this state map with another map. - void intersect(const ConsumedStateMap *Other); - + void intersect(const ConsumedStateMap &Other); + void intersectAtLoopHead(const CFGBlock *LoopHead, const CFGBlock *LoopBack, const ConsumedStateMap *LoopBackStates, ConsumedWarningsHandlerBase &WarningsHandler); @@ -196,15 +196,19 @@ namespace consumed { }; class ConsumedBlockInfo { - std::vector StateMapsArray; + std::vector> StateMapsArray; std::vector VisitOrder; public: - ConsumedBlockInfo() { } - ~ConsumedBlockInfo() { llvm::DeleteContainerPointers(StateMapsArray); } + ConsumedBlockInfo() = default; + ConsumedBlockInfo &operator=(ConsumedBlockInfo &&Other) { + StateMapsArray = std::move(Other.StateMapsArray); + VisitOrder = std::move(Other.VisitOrder); + return *this; + } ConsumedBlockInfo(unsigned int NumBlocks, PostOrderCFGView *SortedGraph) - : StateMapsArray(NumBlocks, nullptr), VisitOrder(NumBlocks, 0) { + : StateMapsArray(NumBlocks), VisitOrder(NumBlocks, 0) { unsigned int VisitOrderCounter = 0; for (PostOrderCFGView::iterator BI = SortedGraph->begin(), BE = SortedGraph->end(); BI != BE; ++BI) { @@ -214,17 +218,18 @@ namespace consumed { bool allBackEdgesVisited(const CFGBlock *CurrBlock, const CFGBlock *TargetBlock); - + void addInfo(const CFGBlock *Block, ConsumedStateMap *StateMap, - bool &AlreadyOwned); - void addInfo(const CFGBlock *Block, ConsumedStateMap *StateMap); - + std::unique_ptr &OwnedStateMap); + void addInfo(const CFGBlock *Block, + std::unique_ptr StateMap); + ConsumedStateMap* borrowInfo(const CFGBlock *Block); void discardInfo(const CFGBlock *Block); - - ConsumedStateMap* getInfo(const CFGBlock *Block); - + + std::unique_ptr getInfo(const CFGBlock *Block); + bool isBackEdge(const CFGBlock *From, const CFGBlock *To); bool isBackEdgeTarget(const CFGBlock *Block); }; @@ -233,8 +238,8 @@ namespace consumed { class ConsumedAnalyzer { ConsumedBlockInfo BlockInfo; - ConsumedStateMap *CurrStates; - + std::unique_ptr CurrStates; + ConsumedState ExpectedReturnState; void determineExpectedReturnState(AnalysisDeclContext &AC, diff --git a/lib/Analysis/Consumed.cpp b/lib/Analysis/Consumed.cpp index fa985ee02e..9df23923b0 100644 --- a/lib/Analysis/Consumed.cpp +++ b/lib/Analysis/Consumed.cpp @@ -1038,65 +1038,54 @@ bool ConsumedBlockInfo::allBackEdgesVisited(const CFGBlock *CurrBlock, return true; } -void ConsumedBlockInfo::addInfo(const CFGBlock *Block, - ConsumedStateMap *StateMap, - bool &AlreadyOwned) { - +void ConsumedBlockInfo::addInfo( + const CFGBlock *Block, ConsumedStateMap *StateMap, + std::unique_ptr &OwnedStateMap) { + assert(Block && "Block pointer must not be NULL"); - - ConsumedStateMap *Entry = StateMapsArray[Block->getBlockID()]; - + + auto &Entry = StateMapsArray[Block->getBlockID()]; + if (Entry) { - Entry->intersect(StateMap); - - } else if (AlreadyOwned) { - StateMapsArray[Block->getBlockID()] = new ConsumedStateMap(*StateMap); - - } else { - StateMapsArray[Block->getBlockID()] = StateMap; - AlreadyOwned = true; - } + Entry->intersect(*StateMap); + } else if (OwnedStateMap) + Entry = std::move(OwnedStateMap); + else + Entry = llvm::make_unique(*StateMap); } void ConsumedBlockInfo::addInfo(const CFGBlock *Block, - ConsumedStateMap *StateMap) { + std::unique_ptr StateMap) { assert(Block && "Block pointer must not be NULL"); - ConsumedStateMap *Entry = StateMapsArray[Block->getBlockID()]; - + auto &Entry = StateMapsArray[Block->getBlockID()]; + if (Entry) { - Entry->intersect(StateMap); - delete StateMap; - + Entry->intersect(*StateMap); } else { - StateMapsArray[Block->getBlockID()] = StateMap; + Entry = std::move(StateMap); } } ConsumedStateMap* ConsumedBlockInfo::borrowInfo(const CFGBlock *Block) { assert(Block && "Block pointer must not be NULL"); assert(StateMapsArray[Block->getBlockID()] && "Block has no block info"); - - return StateMapsArray[Block->getBlockID()]; + + return StateMapsArray[Block->getBlockID()].get(); } void ConsumedBlockInfo::discardInfo(const CFGBlock *Block) { - unsigned int BlockID = Block->getBlockID(); - delete StateMapsArray[BlockID]; - StateMapsArray[BlockID] = nullptr; + StateMapsArray[Block->getBlockID()] = nullptr; } -ConsumedStateMap* ConsumedBlockInfo::getInfo(const CFGBlock *Block) { +std::unique_ptr +ConsumedBlockInfo::getInfo(const CFGBlock *Block) { assert(Block && "Block pointer must not be NULL"); - - ConsumedStateMap *StateMap = StateMapsArray[Block->getBlockID()]; - if (isBackEdgeTarget(Block)) { - return new ConsumedStateMap(*StateMap); - } else { - StateMapsArray[Block->getBlockID()] = nullptr; - return StateMap; - } + + auto &Entry = StateMapsArray[Block->getBlockID()]; + return isBackEdgeTarget(Block) ? llvm::make_unique(*Entry) + : std::move(Entry); } bool ConsumedBlockInfo::isBackEdge(const CFGBlock *From, const CFGBlock *To) { @@ -1166,15 +1155,15 @@ ConsumedStateMap::getState(const CXXBindTemporaryExpr *Tmp) const { return CS_None; } -void ConsumedStateMap::intersect(const ConsumedStateMap *Other) { +void ConsumedStateMap::intersect(const ConsumedStateMap &Other) { ConsumedState LocalState; - - if (this->From && this->From == Other->From && !Other->Reachable) { + + if (this->From && this->From == Other.From && !Other.Reachable) { this->markUnreachable(); return; } - - for (const auto &DM : Other->VarMap) { + + for (const auto &DM : Other.VarMap) { LocalState = this->getState(DM.first); if (LocalState == CS_None) @@ -1282,14 +1271,14 @@ bool ConsumedAnalyzer::splitState(const CFGBlock *CurrBlock, if (PInfo.isVarTest()) { CurrStates->setSource(Cond); FalseStates->setSource(Cond); - splitVarStateForIf(IfNode, PInfo.getVarTest(), CurrStates, + splitVarStateForIf(IfNode, PInfo.getVarTest(), CurrStates.get(), FalseStates.get()); - + } else if (PInfo.isBinTest()) { CurrStates->setSource(PInfo.testSourceNode()); FalseStates->setSource(PInfo.testSourceNode()); - splitVarStateForIfBinOp(PInfo, CurrStates, FalseStates.get()); - + splitVarStateForIfBinOp(PInfo, CurrStates.get(), FalseStates.get()); + } else { return false; } @@ -1337,14 +1326,13 @@ bool ConsumedAnalyzer::splitState(const CFGBlock *CurrBlock, CFGBlock::const_succ_iterator SI = CurrBlock->succ_begin(); if (*SI) - BlockInfo.addInfo(*SI, CurrStates); + BlockInfo.addInfo(*SI, std::move(CurrStates)); else - delete CurrStates; - + CurrStates = nullptr; + if (*++SI) - BlockInfo.addInfo(*SI, FalseStates.release()); + BlockInfo.addInfo(*SI, std::move(FalseStates)); - CurrStates = nullptr; return true; } @@ -1363,10 +1351,10 @@ void ConsumedAnalyzer::run(AnalysisDeclContext &AC) { // AC.getCFG()->viewCFG(LangOptions()); BlockInfo = ConsumedBlockInfo(CFGraph->getNumBlockIDs(), SortedGraph); - - CurrStates = new ConsumedStateMap(); - ConsumedStmtVisitor Visitor(AC, *this, CurrStates); - + + CurrStates = llvm::make_unique(); + ConsumedStmtVisitor Visitor(AC, *this, CurrStates.get()); + // Add all trackable parameters to the state map. for (const auto *PI : D->params()) Visitor.VisitParmVarDecl(PI); @@ -1380,13 +1368,12 @@ void ConsumedAnalyzer::run(AnalysisDeclContext &AC) { continue; } else if (!CurrStates->isReachable()) { - delete CurrStates; CurrStates = nullptr; continue; } - - Visitor.reset(CurrStates); - + + Visitor.reset(CurrStates.get()); + // Visit all of the basic block's statements. for (const auto &B : *CurrBlock) { switch (B.getKind()) { @@ -1429,28 +1416,24 @@ void ConsumedAnalyzer::run(AnalysisDeclContext &AC) { if (CurrBlock->succ_size() > 1 || (CurrBlock->succ_size() == 1 && (*CurrBlock->succ_begin())->pred_size() > 1)) { - - bool OwnershipTaken = false; - + + auto *RawState = CurrStates.get(); + for (CFGBlock::const_succ_iterator SI = CurrBlock->succ_begin(), SE = CurrBlock->succ_end(); SI != SE; ++SI) { if (*SI == nullptr) continue; if (BlockInfo.isBackEdge(CurrBlock, *SI)) { - BlockInfo.borrowInfo(*SI)->intersectAtLoopHead(*SI, CurrBlock, - CurrStates, - WarningsHandler); - + BlockInfo.borrowInfo(*SI)->intersectAtLoopHead( + *SI, CurrBlock, RawState, WarningsHandler); + if (BlockInfo.allBackEdgesVisited(CurrBlock, *SI)) BlockInfo.discardInfo(*SI); } else { - BlockInfo.addInfo(*SI, CurrStates, OwnershipTaken); + BlockInfo.addInfo(*SI, RawState, CurrStates); } } - - if (!OwnershipTaken) - delete CurrStates; CurrStates = nullptr; } @@ -1463,8 +1446,8 @@ void ConsumedAnalyzer::run(AnalysisDeclContext &AC) { } // End of block iterator. // Delete the last existing state map. - delete CurrStates; - + CurrStates = nullptr; + WarningsHandler.emitDiagnostics(); } }} // end namespace clang::consumed