r192314 - Consumed analysis: improve loop handling. The prior version of the analysis
Nico Weber
thakis at chromium.org
Fri Apr 25 13:03:11 PDT 2014
(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
More information about the cfe-commits
mailing list