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@gmail.com.  Reviewed and edited by
delesley@google.com.

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@192314 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/include/clang/Analysis/Analyses/Consumed.h b/include/clang/Analysis/Analyses/Consumed.h
index cc17a12b0542509d0d707e506117c309f16f9a62..0081a88ae9f78b35cd5cd11276a869da332ffd01 100644 (file)
--- a/include/clang/Analysis/Analyses/Consumed.h
+++ b/include/clang/Analysis/Analyses/Consumed.h
@@ -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.

diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td
index b30fd3fc7211910de0c593ae5d43df5a854d4afe..868a5dd7d60d531ce2d3bfd5640e64dc92bb44f2 100644 (file)
--- a/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/include/clang/Basic/DiagnosticSemaKinds.td
@@ -2213,6 +2213,9 @@ def warn_return_typestate_for_unconsumable_type : Warning<
 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<

diff --git a/lib/Analysis/Consumed.cpp b/lib/Analysis/Consumed.cpp
index ee8dd77ff63779e99ce408ebd9613983fc55b54c..d291f627d0249ef9f2fd702539375c2aeaedfe5d 100644 (file)
--- a/lib/Analysis/Consumed.cpp
+++ b/lib/Analysis/Consumed.cpp
@@ -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 PropagationInfo &PInfo,
   }
 }
 
+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 CFGBlock *Block,
 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 CFGBlock *Block,
   }
 }
 
-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 ConsumedStateMap *Other) {
     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 ConsumedStateMap *Other) {
   }
 }
 
+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 VarDecl *Var) {
   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(AnalysisDeclContext &AC) {
     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(AnalysisDeclContext &AC) {
        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(AnalysisDeclContext &AC) {
     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.

diff --git a/lib/Sema/AnalysisBasedWarnings.cpp b/lib/Sema/AnalysisBasedWarnings.cpp
index a206acd7f2339c8f07094cb600a8b4a641f3faf4..53e53b28f4ca3528ab2bdc156d0b078ca72334a1 100644 (file)
--- a/lib/Sema/AnalysisBasedWarnings.cpp
+++ b/lib/Sema/AnalysisBasedWarnings.cpp
@@ -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(

diff --git a/test/SemaCXX/warn-consumed-analysis.cpp b/test/SemaCXX/warn-consumed-analysis.cpp
index c33e3a197aed983018a8306c897fd7991ef0765f..0618f77b5f4a6b048158a47518e5e22766997ca3 100644 (file)
--- a/test/SemaCXX/warn-consumed-analysis.cpp
+++ b/test/SemaCXX/warn-consumed-analysis.cpp
@@ -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}}
 }