r192314 - Consumed analysis: improve loop handling. The prior version of the analysis
Delesley Hutchins
delesley at google.com
Mon Apr 28 13:48:52 PDT 2014
It's deleted on line 1489, "if (!OwnershipTaken) delete CurrStates."
-DeLesley
On Fri, Apr 25, 2014 at 1:03 PM, Nico Weber <thakis at chromium.org> wrote:
> (Below at ^^)
>
> On Wed, Oct 9, 2013 at 11:30 AM, DeLesley Hutchins <delesley at 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.wailes at gmail.com. Reviewed and edited by
>> delesley at 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 at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
--
DeLesley Hutchins | Software Engineer | delesley at google.com | 505-206-0315
More information about the cfe-commits
mailing list