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

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

diff --git a/clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp b/clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
index 0a7015e..ab4e811 100644 (file)
--- 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 4505aed..a9ccc3b 100644 (file)
--- 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 2cd7b34..3c981b2 100644 (file)
--- 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@-1{{FALSE}} expected-warning@-1{{TRUE}}
   if (V.end() != first) {
-      clang_analyzer_eval(clang_analyzer_container_end(V) ==
-                        clang_analyzer_iterator_position(first)); // expected-warning@-1{{FALSE}} expected-warning@-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@-1{{FALSE}}
   }
 }