[clang] 855d21a - [Analyzer] Iterator Checkers: Replace `UnknownVal` in comparison result by a conjured value

[Adam Balogh via cfe-commits]

Author: Adam Balogh
Date: 2019-12-11T15:24:06+01:00
New Revision: 855d21a03ae841b7c6c980e92f67bd5b65287fa6

URL: https://github.com/llvm/llvm-project/commit/855d21a03ae841b7c6c980e92f67bd5b65287fa6
DIFF: https://github.com/llvm/llvm-project/commit/855d21a03ae841b7c6c980e92f67bd5b65287fa6.diff

LOG: [Analyzer] Iterator Checkers: Replace `UnknownVal` in comparison result by a conjured value

Sometimes the return value of a comparison operator call is
`UnkownVal`. Since no assumptions can be made on `UnknownVal`,
this leeds to keeping impossible execution paths in the
exploded graph resulting in poor performance and false
positives. To overcome this we replace unknown results of
iterator comparisons by conjured symbols.

Differential Revision: https://reviews.llvm.org/D70244

Added: 
    

Modified: 
    clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
    clang/test/Analysis/invalidated-iterator.cpp
    clang/test/Analysis/iterator-modelling.cpp

Removed: 
    


################################################################################
diff  --git a/clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp b/clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
index 0a7015e85e93..ab4e8112d677 100644
--- a/clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
@@ -87,7 +87,7 @@ class IteratorModeling
     : public Checker<check::PostCall, check::PostStmt<MaterializeTemporaryExpr>,
                      check::Bind, check::LiveSymbols, check::DeadSymbols> {
 
-  void handleComparison(CheckerContext &C, const Expr *CE, const SVal &RetVal,
+  void handleComparison(CheckerContext &C, const Expr *CE, SVal RetVal,
                         const SVal &LVal, const SVal &RVal,
                         OverloadedOperatorKind Op) const;
   void processComparison(CheckerContext &C, ProgramStateRef State,
@@ -499,9 +499,9 @@ void IteratorModeling::checkDeadSymbols(SymbolReaper &SR,
 }
 
 void IteratorModeling::handleComparison(CheckerContext &C, const Expr *CE,
-                                        const SVal &RetVal, const SVal &LVal,
-                                        const SVal &RVal,
-                                        OverloadedOperatorKind Op) const {
+                                       SVal RetVal, const SVal &LVal,
+                                       const SVal &RVal,
+                                       OverloadedOperatorKind Op) const {
   // Record the operands and the operator of the comparison for the next
   // evalAssume, if the result is a symbolic expression. If it is a concrete
   // value (only one branch is possible), then transfer the state between
@@ -538,6 +538,16 @@ void IteratorModeling::handleComparison(CheckerContext &C, const Expr *CE,
     RPos = getIteratorPosition(State, RVal);
   }
 
+  // We cannot make assumpotions on `UnknownVal`. Let us conjure a symbol
+  // instead.
+  if (RetVal.isUnknown()) {
+    auto &SymMgr = C.getSymbolManager();
+    auto *LCtx = C.getLocationContext();
+    RetVal = nonloc::SymbolVal(SymMgr.conjureSymbol(
+        CE, LCtx, C.getASTContext().BoolTy, C.blockCount()));
+    State = State->BindExpr(CE, LCtx, RetVal);
+  }
+
   processComparison(C, State, LPos->getOffset(), RPos->getOffset(), RetVal, Op);
 }
 
@@ -559,7 +569,7 @@ void IteratorModeling::processComparison(CheckerContext &C,
   const auto ConditionVal = RetVal.getAs<DefinedSVal>();
   if (!ConditionVal)
     return;
-  
+
   if (auto StateTrue = relateSymbols(State, Sym1, Sym2, Op == OO_EqualEqual)) {
     StateTrue = StateTrue->assume(*ConditionVal, true);
     C.addTransition(StateTrue);

diff  --git a/clang/test/Analysis/invalidated-iterator.cpp b/clang/test/Analysis/invalidated-iterator.cpp
index 4505aedd4e36..a9ccc3b75834 100644
--- a/clang/test/Analysis/invalidated-iterator.cpp
+++ b/clang/test/Analysis/invalidated-iterator.cpp
@@ -120,4 +120,3 @@ void assignment(std::vector<int> &V) {
   V.erase(i);
   auto j = V.cbegin(); // no-warning
 }
-

diff  --git a/clang/test/Analysis/iterator-modelling.cpp b/clang/test/Analysis/iterator-modelling.cpp
index 2cd7b349be81..3c981b27bbbf 100644
--- a/clang/test/Analysis/iterator-modelling.cpp
+++ b/clang/test/Analysis/iterator-modelling.cpp
@@ -1969,8 +1969,8 @@ void non_std_find(std::vector<int> &V, int e) {
   clang_analyzer_eval(clang_analyzer_container_end(V) ==
                       clang_analyzer_iterator_position(first)); // expected-warning at -1{{FALSE}} expected-warning at -1{{TRUE}}
   if (V.end() != first) {
-      clang_analyzer_eval(clang_analyzer_container_end(V) ==
-                        clang_analyzer_iterator_position(first)); // expected-warning at -1{{FALSE}} expected-warning at -1 0-1{{TRUE}} FIXME: should only expect FALSE in every case
+    clang_analyzer_eval(clang_analyzer_container_end(V) ==
+                        clang_analyzer_iterator_position(first)); // expected-warning at -1{{FALSE}}
   }
 }