It's deleted on line 1489, "if (!OwnershipTaken) delete CurrStates."
-DeLesley On Fri, Apr 25, 2014 at 1:03 PM, Nico Weber <tha...@chromium.org> wrote: > (Below at ^^) > > On Wed, Oct 9, 2013 at 11:30 AM, DeLesley Hutchins <deles...@google.com> > wrote: >> Author: delesley >> Date: Wed Oct 9 13:30:24 2013 >> New Revision: 192314 >> >> URL: http://llvm.org/viewvc/llvm-project?rev=192314&view=rev >> Log: >> Consumed analysis: improve loop handling. The prior version of the analysis >> marked all variables as "unknown" at the start of a loop. The new version >> keeps the initial state of variables unchanged, but issues a warning if the >> state at the end of the loop is different from the state at the beginning. >> This patch will eventually be replaced with a more precise analysis. >> >> Initial patch by chris.wai...@gmail.com. Reviewed and edited by >> deles...@google.com. >> >> Modified: >> cfe/trunk/include/clang/Analysis/Analyses/Consumed.h >> cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td >> cfe/trunk/lib/Analysis/Consumed.cpp >> cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp >> cfe/trunk/test/SemaCXX/warn-consumed-analysis.cpp >> >> Modified: cfe/trunk/include/clang/Analysis/Analyses/Consumed.h >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Analysis/Analyses/Consumed.h?rev=192314&r1=192313&r2=192314&view=diff >> ============================================================================== >> --- cfe/trunk/include/clang/Analysis/Analyses/Consumed.h (original) >> +++ cfe/trunk/include/clang/Analysis/Analyses/Consumed.h Wed Oct 9 13:30:24 >> 2013 >> @@ -49,6 +49,16 @@ namespace consumed { >> /// \brief Emit the warnings and notes left by the analysis. >> virtual void emitDiagnostics() {} >> >> + /// \brief Warn that a variable's state doesn't match at the entry and >> exit >> + /// of a loop. >> + /// >> + /// \param Loc -- The location of the end of the loop. >> + /// >> + /// \param VariableName -- The name of the variable that has a >> mismatched >> + /// state. >> + virtual void warnLoopStateMismatch(SourceLocation Loc, >> + StringRef VariableName) {} >> + >> // FIXME: This can be removed when the attr propagation fix for >> templated >> // classes lands. >> /// \brief Warn about return typestates set for unconsumable types. >> @@ -120,17 +130,18 @@ namespace consumed { >> : Reachable(Other.Reachable), From(Other.From), Map(Other.Map) {} >> >> /// \brief Get the consumed state of a given variable. >> - ConsumedState getState(const VarDecl *Var); >> + ConsumedState getState(const VarDecl *Var) const; >> >> /// \brief Merge this state map with another map. >> void intersect(const ConsumedStateMap *Other); >> >> + void intersectAtLoopHead(const CFGBlock *LoopHead, const CFGBlock >> *LoopBack, >> + const ConsumedStateMap *LoopBackStates, >> + ConsumedWarningsHandlerBase &WarningsHandler); >> + >> /// \brief Return true if this block is reachable. >> bool isReachable() const { return Reachable; } >> >> - /// \brief Mark all variables as unknown. >> - void makeUnknown(); >> - >> /// \brief Mark the block as unreachable. >> void markUnreachable(); >> >> @@ -144,28 +155,45 @@ namespace consumed { >> >> /// \brief Remove the variable from our state map. >> void remove(const VarDecl *Var); >> + >> + /// \brief Tests to see if there is a mismatch in the states stored in >> two >> + /// maps. >> + /// >> + /// \param Other -- The second map to compare against. >> + bool operator!=(const ConsumedStateMap *Other) const; >> }; >> >> class ConsumedBlockInfo { >> - >> - ConsumedStateMap **StateMapsArray; >> - PostOrderCFGView::CFGBlockSet VisitedBlocks; >> + std::vector<ConsumedStateMap*> StateMapsArray; >> + std::vector<int> VisitOrder; >> >> public: >> - >> ConsumedBlockInfo() : StateMapsArray(NULL) {} >> >> - ConsumedBlockInfo(const CFG *CFGraph) >> - : StateMapsArray(new ConsumedStateMap*[CFGraph->getNumBlockIDs()]()), >> - VisitedBlocks(CFGraph) {} >> + ConsumedBlockInfo(unsigned int NumBlocks, PostOrderCFGView *SortedGraph) >> + : StateMapsArray(NumBlocks, 0), VisitOrder(NumBlocks, 0) { >> + unsigned int VisitOrderCounter = 0; >> + for (PostOrderCFGView::iterator BI = SortedGraph->begin(), >> + BE = SortedGraph->end(); BI != BE; ++BI) { >> + VisitOrder[(*BI)->getBlockID()] = VisitOrderCounter++; >> + } >> + } >> + >> + bool allBackEdgesVisited(const CFGBlock *CurrBlock, >> + const CFGBlock *TargetBlock); >> >> void addInfo(const CFGBlock *Block, ConsumedStateMap *StateMap, >> bool &AlreadyOwned); >> void addInfo(const CFGBlock *Block, ConsumedStateMap *StateMap); >> >> + ConsumedStateMap* borrowInfo(const CFGBlock *Block); >> + >> + void discardInfo(const CFGBlock *Block); >> + >> ConsumedStateMap* getInfo(const CFGBlock *Block); >> >> - void markVisited(const CFGBlock *Block); >> + bool isBackEdge(const CFGBlock *From, const CFGBlock *To); >> + bool isBackEdgeTarget(const CFGBlock *Block); >> }; >> >> /// A class that handles the analysis of uniqueness violations. >> >> Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=192314&r1=192313&r2=192314&view=diff >> ============================================================================== >> --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original) >> +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Wed Oct 9 13:30:24 >> 2013 >> @@ -2213,6 +2213,9 @@ def warn_return_typestate_for_unconsumab >> def warn_return_typestate_mismatch : Warning< >> "return value not in expected state; expected '%0', observed '%1'">, >> InGroup<Consumed>, DefaultIgnore; >> +def warn_loop_state_mismatch : Warning< >> + "state of variable '%0' must match at the entry and exit of loop">, >> + InGroup<Consumed>, DefaultIgnore; >> >> // ConsumedStrict warnings >> def warn_unnecessary_test : Warning< >> >> Modified: cfe/trunk/lib/Analysis/Consumed.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/Consumed.cpp?rev=192314&r1=192313&r2=192314&view=diff >> ============================================================================== >> --- cfe/trunk/lib/Analysis/Consumed.cpp (original) >> +++ cfe/trunk/lib/Analysis/Consumed.cpp Wed Oct 9 13:30:24 2013 >> @@ -31,18 +31,15 @@ >> #include "llvm/Support/Compiler.h" >> #include "llvm/Support/raw_ostream.h" >> >> +// TODO: Use information from tests in while-loop conditional. >> // TODO: Add notes about the actual and expected state for >> // TODO: Correctly identify unreachable blocks when chaining boolean >> operators. >> // TODO: Adjust the parser and AttributesList class to support lists of >> // identifiers. >> // TODO: Warn about unreachable code. >> // TODO: Switch to using a bitmap to track unreachable blocks. >> -// TODO: Mark variables as Unknown going into while- or for-loops only if >> they >> -// are referenced inside that block. (Deferred) >> // TODO: Handle variable definitions, e.g. bool valid = x.isValid(); >> // if (valid) ...; (Deferred) >> -// TODO: Add a method(s) to identify which method calls perform what state >> -// transitions. (Deferred) >> // TODO: Take notes on state transitions to provide better warning messages. >> // (Deferred) >> // TODO: Test nested conditionals: A) Checking the same value multiple >> times, >> @@ -54,6 +51,27 @@ using namespace consumed; >> // Key method definition >> ConsumedWarningsHandlerBase::~ConsumedWarningsHandlerBase() {} >> >> +static SourceLocation getWarningLocForLoopExit(const CFGBlock *ExitBlock) { >> + // Find the source location of the last statement in the block, if the >> block >> + // is not empty. >> + if (const Stmt *StmtNode = ExitBlock->getTerminator()) { >> + return StmtNode->getLocStart(); >> + } else { >> + for (CFGBlock::const_reverse_iterator BI = ExitBlock->rbegin(), >> + BE = ExitBlock->rend(); BI != BE; ++BI) { >> + // FIXME: Handle other CFGElement kinds. >> + if (Optional<CFGStmt> CS = BI->getAs<CFGStmt>()) >> + return CS->getStmt()->getLocStart(); >> + } >> + } >> + >> + // The block is empty, and has a single predecessor. Use its exit >> location. >> + assert(ExitBlock->pred_size() == 1 && *ExitBlock->pred_begin() && >> + ExitBlock->succ_size() != 0); >> + >> + return getWarningLocForLoopExit(*ExitBlock->pred_begin()); >> +} >> + >> static ConsumedState invertConsumedUnconsumed(ConsumedState State) { >> switch (State) { >> case CS_Unconsumed: >> @@ -897,11 +915,26 @@ void splitVarStateForIfBinOp(const Propa >> } >> } >> >> +bool ConsumedBlockInfo::allBackEdgesVisited(const CFGBlock *CurrBlock, >> + const CFGBlock *TargetBlock) { >> + >> + assert(CurrBlock && "Block pointer must not be NULL"); >> + assert(TargetBlock && "TargetBlock pointer must not be NULL"); >> + >> + unsigned int CurrBlockOrder = VisitOrder[CurrBlock->getBlockID()]; >> + for (CFGBlock::const_pred_iterator PI = TargetBlock->pred_begin(), >> + PE = TargetBlock->pred_end(); PI != PE; ++PI) { >> + if (*PI && CurrBlockOrder < VisitOrder[(*PI)->getBlockID()] ) >> + return false; >> + } >> + return true; >> +} >> + >> void ConsumedBlockInfo::addInfo(const CFGBlock *Block, >> ConsumedStateMap *StateMap, >> bool &AlreadyOwned) { >> >> - if (VisitedBlocks.alreadySet(Block)) return; >> + assert(Block && "Block pointer must not be NULL"); >> >> ConsumedStateMap *Entry = StateMapsArray[Block->getBlockID()]; >> >> @@ -920,10 +953,7 @@ void ConsumedBlockInfo::addInfo(const CF >> void ConsumedBlockInfo::addInfo(const CFGBlock *Block, >> ConsumedStateMap *StateMap) { >> >> - if (VisitedBlocks.alreadySet(Block)) { >> - delete StateMap; >> - return; >> - } >> + assert(Block != NULL && "Block pointer must not be NULL"); >> >> ConsumedStateMap *Entry = StateMapsArray[Block->getBlockID()]; >> >> @@ -936,15 +966,56 @@ void ConsumedBlockInfo::addInfo(const CF >> } >> } >> >> -ConsumedStateMap* ConsumedBlockInfo::getInfo(const CFGBlock *Block) { >> +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()]; >> } >> >> -void ConsumedBlockInfo::markVisited(const CFGBlock *Block) { >> - VisitedBlocks.insert(Block); >> +void ConsumedBlockInfo::discardInfo(const CFGBlock *Block) { >> + unsigned int BlockID = Block->getBlockID(); >> + delete StateMapsArray[BlockID]; >> + StateMapsArray[BlockID] = NULL; >> +} >> + >> +ConsumedStateMap* 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); > > ^^ Who is supposed to delete this? > >> + } else { >> + StateMapsArray[Block->getBlockID()] = NULL; >> + return StateMap; >> + } >> +} >> + >> +bool ConsumedBlockInfo::isBackEdge(const CFGBlock *From, const CFGBlock >> *To) { >> + assert(From && "From block must not be NULL"); >> + assert(To && "From block must not be NULL"); >> + >> + return VisitOrder[From->getBlockID()] > VisitOrder[To->getBlockID()]; >> +} >> + >> +bool ConsumedBlockInfo::isBackEdgeTarget(const CFGBlock *Block) { >> + assert(Block != NULL && "Block pointer must not be NULL"); >> + >> + // Anything with less than two predecessors can't be the target of a back >> + // edge. >> + if (Block->pred_size() < 2) >> + return false; >> + >> + unsigned int BlockVisitOrder = VisitOrder[Block->getBlockID()]; >> + for (CFGBlock::const_pred_iterator PI = Block->pred_begin(), >> + PE = Block->pred_end(); PI != PE; ++PI) { >> + if (*PI && BlockVisitOrder < VisitOrder[(*PI)->getBlockID()]) >> + return true; >> + } >> + return false; >> } >> >> -ConsumedState ConsumedStateMap::getState(const VarDecl *Var) { >> +ConsumedState ConsumedStateMap::getState(const VarDecl *Var) const { >> MapType::const_iterator Entry = Map.find(Var); >> >> if (Entry != Map.end()) { >> @@ -963,8 +1034,8 @@ void ConsumedStateMap::intersect(const C >> return; >> } >> >> - for (MapType::const_iterator DMI = Other->Map.begin(), >> - DME = Other->Map.end(); DMI != DME; ++DMI) { >> + for (MapType::const_iterator DMI = Other->Map.begin(), DME = >> Other->Map.end(); >> + DMI != DME; ++DMI) { >> >> LocalState = this->getState(DMI->first); >> >> @@ -976,19 +1047,34 @@ void ConsumedStateMap::intersect(const C >> } >> } >> >> +void ConsumedStateMap::intersectAtLoopHead(const CFGBlock *LoopHead, >> + const CFGBlock *LoopBack, const ConsumedStateMap *LoopBackStates, >> + ConsumedWarningsHandlerBase &WarningsHandler) { >> + >> + ConsumedState LocalState; >> + SourceLocation BlameLoc = getWarningLocForLoopExit(LoopBack); >> + >> + for (MapType::const_iterator DMI = LoopBackStates->Map.begin(), >> + DME = LoopBackStates->Map.end(); DMI != DME; ++DMI) { >> + >> + LocalState = this->getState(DMI->first); >> + >> + if (LocalState == CS_None) >> + continue; >> + >> + if (LocalState != DMI->second) { >> + Map[DMI->first] = CS_Unknown; >> + WarningsHandler.warnLoopStateMismatch( >> + BlameLoc, DMI->first->getNameAsString()); >> + } >> + } >> +} >> + >> void ConsumedStateMap::markUnreachable() { >> this->Reachable = false; >> Map.clear(); >> } >> >> -void ConsumedStateMap::makeUnknown() { >> - for (MapType::const_iterator DMI = Map.begin(), DME = Map.end(); DMI != >> DME; >> - ++DMI) { >> - >> - Map[DMI->first] = CS_Unknown; >> - } >> -} >> - >> void ConsumedStateMap::setState(const VarDecl *Var, ConsumedState State) { >> Map[Var] = State; >> } >> @@ -997,6 +1083,17 @@ void ConsumedStateMap::remove(const VarD >> Map.erase(Var); >> } >> >> +bool ConsumedStateMap::operator!=(const ConsumedStateMap *Other) const { >> + for (MapType::const_iterator DMI = Other->Map.begin(), DME = >> Other->Map.end(); >> + DMI != DME; ++DMI) { >> + >> + if (this->getState(DMI->first) != DMI->second) >> + return true; >> + } >> + >> + return false; >> +} >> + >> void ConsumedAnalyzer::determineExpectedReturnState(AnalysisDeclContext &AC, >> const FunctionDecl *D) { >> QualType ReturnType; >> @@ -1126,10 +1223,11 @@ void ConsumedAnalyzer::run(AnalysisDeclC >> return; >> >> determineExpectedReturnState(AC, D); >> - >> - BlockInfo = ConsumedBlockInfo(CFGraph); >> - >> + >> PostOrderCFGView *SortedGraph = AC.getAnalysis<PostOrderCFGView>(); >> + // AC.getCFG()->viewCFG(LangOptions()); >> + >> + BlockInfo = ConsumedBlockInfo(CFGraph->getNumBlockIDs(), SortedGraph); >> >> CurrStates = new ConsumedStateMap(); >> ConsumedStmtVisitor Visitor(AC, *this, CurrStates); >> @@ -1145,7 +1243,6 @@ void ConsumedAnalyzer::run(AnalysisDeclC >> E = SortedGraph->end(); I != E; ++I) { >> >> const CFGBlock *CurrBlock = *I; >> - BlockInfo.markVisited(CurrBlock); >> >> if (CurrStates == NULL) >> CurrStates = BlockInfo.getInfo(CurrBlock); >> @@ -1181,27 +1278,33 @@ void ConsumedAnalyzer::run(AnalysisDeclC >> if (!splitState(CurrBlock, Visitor)) { >> CurrStates->setSource(NULL); >> >> - if (CurrBlock->succ_size() > 1) { >> - CurrStates->makeUnknown(); >> + if (CurrBlock->succ_size() > 1 || >> + (CurrBlock->succ_size() == 1 && >> + (*CurrBlock->succ_begin())->pred_size() > 1)) { >> >> bool OwnershipTaken = false; >> >> for (CFGBlock::const_succ_iterator SI = CurrBlock->succ_begin(), >> SE = CurrBlock->succ_end(); SI != SE; ++SI) { >> >> - if (*SI) BlockInfo.addInfo(*SI, CurrStates, OwnershipTaken); >> + if (*SI == NULL) continue; >> + >> + if (BlockInfo.isBackEdge(CurrBlock, *SI)) { >> + BlockInfo.borrowInfo(*SI)->intersectAtLoopHead(*SI, CurrBlock, >> + CurrStates, >> + WarningsHandler); >> + >> + if (BlockInfo.allBackEdgesVisited(*SI, CurrBlock)) >> + BlockInfo.discardInfo(*SI); >> + } else { >> + BlockInfo.addInfo(*SI, CurrStates, OwnershipTaken); >> + } >> } >> >> if (!OwnershipTaken) >> delete CurrStates; >> >> CurrStates = NULL; >> - >> - } else if (CurrBlock->succ_size() == 1 && >> - (*CurrBlock->succ_begin())->pred_size() > 1) { >> - >> - BlockInfo.addInfo(*CurrBlock->succ_begin(), CurrStates); >> - CurrStates = NULL; >> } >> } >> } // End of block iterator. >> >> Modified: cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp?rev=192314&r1=192313&r2=192314&view=diff >> ============================================================================== >> --- cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp (original) >> +++ cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp Wed Oct 9 13:30:24 2013 >> @@ -1477,6 +1477,13 @@ public: >> } >> } >> >> + void warnLoopStateMismatch(SourceLocation Loc, StringRef VariableName) { >> + PartialDiagnosticAt Warning(Loc, >> S.PDiag(diag::warn_loop_state_mismatch) << >> + VariableName); >> + >> + Warnings.push_back(DelayedDiag(Warning, OptionalNotes())); >> + } >> + >> void warnReturnTypestateForUnconsumableType(SourceLocation Loc, >> StringRef TypeName) { >> PartialDiagnosticAt Warning(Loc, S.PDiag( >> >> Modified: cfe/trunk/test/SemaCXX/warn-consumed-analysis.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/warn-consumed-analysis.cpp?rev=192314&r1=192313&r2=192314&view=diff >> ============================================================================== >> --- cfe/trunk/test/SemaCXX/warn-consumed-analysis.cpp (original) >> +++ cfe/trunk/test/SemaCXX/warn-consumed-analysis.cpp Wed Oct 9 13:30:24 >> 2013 >> @@ -494,25 +494,34 @@ void testUnreachableBlock() { >> *var; >> } >> >> -void testSimpleForLoop() { >> - ConsumableClass<int> var; >> + >> +void testForLoop1() { >> + ConsumableClass<int> var0, var1(42); >> >> - for (int i = 0; i < 10; ++i) { >> - *var; // expected-warning {{invalid invocation of method 'operator*' on >> object 'var' while it is in the 'unknown' state}} >> + for (int i = 0; i < 10; ++i) { // expected-warning {{state of variable >> 'var1' must match at the entry and exit of loop}} >> + *var0; // expected-warning {{invalid invocation of method 'operator*' >> on object 'var0' while it is in the 'consumed' state}} >> + >> + *var1; >> + var1.consume(); >> + *var1; // expected-warning {{invalid invocation of method 'operator*' >> on object 'var1' while it is in the 'consumed' state}} >> } >> >> - *var; // expected-warning {{invalid invocation of method 'operator*' on >> object 'var' while it is in the 'unknown' state}} >> + *var0; // expected-warning {{invalid invocation of method 'operator*' on >> object 'var0' while it is in the 'consumed' state}} >> } >> >> -void testSimpleWhileLoop() { >> - int i = 0; >> +void testWhileLoop1() { >> + int i = 10; >> >> - ConsumableClass<int> var; >> + ConsumableClass<int> var0, var1(42); >> >> - while (i < 10) { >> - *var; // expected-warning {{invalid invocation of method 'operator*' on >> object 'var' while it is in the 'unknown' state}} >> - ++i; >> + while (i-- > 0) { >> + *var0; // expected-warning {{invalid invocation of method 'operator*' >> on object 'var0' while it is in the 'consumed' state}} >> + >> + *var1; >> + var1.consume(); >> + *var1; // expected-warning {{invalid invocation of method 'operator*' >> on object 'var1' while it is in the 'consumed' state}} \ >> + // expected-warning {{state of variable 'var1' must match at the >> entry and exit of loop}} >> } >> >> - *var; // expected-warning {{invalid invocation of method 'operator*' on >> object 'var' while it is in the 'unknown' state}} >> + *var0; // expected-warning {{invalid invocation of method 'operator*' on >> object 'var0' while it is in the 'consumed' state}} >> } >> >> >> _______________________________________________ >> cfe-commits mailing list >> cfe-commits@cs.uiuc.edu >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits -- DeLesley Hutchins | Software Engineer | deles...@google.com | 505-206-0315 _______________________________________________ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
