r192314 - Consumed analysis: improve loop handling. The prior version of the analysis
DeLesley Hutchins
delesley at google.com
Wed Oct 9 11:30:25 PDT 2013
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);
+ } 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}}
}
More information about the cfe-commits
mailing list