[clang] 770ad9f - [Analyzer] Fix for iterator modeling and checkers: handle negative numbers correctly

[Adam Balogh via cfe-commits]

Author: Adam Balogh
Date: 2020-02-25T14:57:34+01:00
New Revision: 770ad9f55e660e0ec89f61d5579dfafad17ab5f5

URL: https://github.com/llvm/llvm-project/commit/770ad9f55e660e0ec89f61d5579dfafad17ab5f5
DIFF: https://github.com/llvm/llvm-project/commit/770ad9f55e660e0ec89f61d5579dfafad17ab5f5.diff

LOG: [Analyzer] Fix for iterator modeling and checkers: handle negative numbers correctly

Currently, using negative numbers in iterator operations (additions and
subractions) results in advancements with huge positive numbers due to
an error. This patch fixes it.

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

Added: 
    

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

Removed: 
    


################################################################################
diff  --git a/clang/lib/StaticAnalyzer/Checkers/Iterator.cpp b/clang/lib/StaticAnalyzer/Checkers/Iterator.cpp
index 64daf358fbe5..e80d8bc32dec 100644
--- a/clang/lib/StaticAnalyzer/Checkers/Iterator.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/Iterator.cpp
@@ -200,22 +200,29 @@ ProgramStateRef advancePosition(ProgramStateRef State, const SVal &Iter,
 
   auto &SymMgr = State->getStateManager().getSymbolManager();
   auto &SVB = State->getStateManager().getSValBuilder();
+  auto &BVF = State->getStateManager().getBasicVals();
 
   assert ((Op == OO_Plus || Op == OO_PlusEqual ||
            Op == OO_Minus || Op == OO_MinusEqual) &&
           "Advance operator must be one of +, -, += and -=.");
   auto BinOp = (Op == OO_Plus || Op == OO_PlusEqual) ? BO_Add : BO_Sub;
-  if (const auto IntDist = Distance.getAs<nonloc::ConcreteInt>()) {
-    // For concrete integers we can calculate the new position
-    const auto NewPos =
-      Pos->setTo(SVB.evalBinOp(State, BinOp,
-                               nonloc::SymbolVal(Pos->getOffset()),
-                               *IntDist, SymMgr.getType(Pos->getOffset()))
-                 .getAsSymbol());
-    return setIteratorPosition(State, Iter, NewPos);
-  }
+  const auto IntDistOp = Distance.getAs<nonloc::ConcreteInt>();
+  if (!IntDistOp)
+    return nullptr;
 
-  return nullptr;
+  // For concrete integers we can calculate the new position
+  nonloc::ConcreteInt IntDist = *IntDistOp;
+
+  if (IntDist.getValue().isNegative()) {
+    IntDist = nonloc::ConcreteInt(BVF.getValue(-IntDist.getValue()));
+    BinOp = (BinOp == BO_Add) ? BO_Sub : BO_Add;
+  }
+  const auto NewPos =
+    Pos->setTo(SVB.evalBinOp(State, BinOp,
+                             nonloc::SymbolVal(Pos->getOffset()),
+                             IntDist, SymMgr.getType(Pos->getOffset()))
+               .getAsSymbol());
+  return setIteratorPosition(State, Iter, NewPos);
 }
 
 // This function tells the analyzer's engine that symbols produced by our

diff  --git a/clang/test/Analysis/iterator-modelling.cpp b/clang/test/Analysis/iterator-modelling.cpp
index b2551939986a..4e40319cedc1 100644
--- a/clang/test/Analysis/iterator-modelling.cpp
+++ b/clang/test/Analysis/iterator-modelling.cpp
@@ -100,6 +100,16 @@ void plus_equal(const std::vector<int> &v) {
   clang_analyzer_express(clang_analyzer_iterator_position(i)); //expected-warning{{$v.begin() + 2}}
 }
 
+void plus_equal_negative(const std::vector<int> &v) {
+  auto i = v.end();
+
+  clang_analyzer_denote(clang_analyzer_container_end(v), "$v.end()");
+
+  i += -2;
+
+  clang_analyzer_express(clang_analyzer_iterator_position(i)); //expected-warning{{$v.end() - 2}}
+}
+
 void minus_equal(const std::vector<int> &v) {
   auto i = v.end();
 
@@ -110,6 +120,16 @@ void minus_equal(const std::vector<int> &v) {
   clang_analyzer_express(clang_analyzer_iterator_position(i)); //expected-warning{{$v.end() - 2}}
 }
 
+void minus_equal_negative(const std::vector<int> &v) {
+  auto i = v.begin();
+
+  clang_analyzer_denote(clang_analyzer_container_begin(v), "$v.begin()");
+
+  i -= -2;
+
+  clang_analyzer_express(clang_analyzer_iterator_position(i)); //expected-warning{{$v.begin() + 2}}
+}
+
 void copy(const std::vector<int> &v) {
   auto i1 = v.end();
 
@@ -132,6 +152,17 @@ void plus(const std::vector<int> &v) {
   clang_analyzer_express(clang_analyzer_iterator_position(i2)); //expected-warning{{$v.begin() + 2}}
 }
 
+void plus_negative(const std::vector<int> &v) {
+  auto i1 = v.end();
+
+  clang_analyzer_denote(clang_analyzer_container_end(v), "$v.end()");
+
+  auto i2 = i1 + (-2);
+
+  clang_analyzer_eval(clang_analyzer_iterator_container(i2) == &v); // expected-warning{{TRUE}}
+  clang_analyzer_express(clang_analyzer_iterator_position(i2)); //expected-warning{{$v.end() - 2}}
+}
+
 void minus(const std::vector<int> &v) {
   auto i1 = v.end();
 
@@ -143,6 +174,17 @@ void minus(const std::vector<int> &v) {
   clang_analyzer_express(clang_analyzer_iterator_position(i2)); //expected-warning{{$v.end() - 2}}
 }
 
+void minus_negative(const std::vector<int> &v) {
+  auto i1 = v.begin();
+
+  clang_analyzer_denote(clang_analyzer_container_begin(v), "$v.begin()");
+
+  auto i2 = i1 - (-2);
+
+  clang_analyzer_eval(clang_analyzer_iterator_container(i2) == &v); // expected-warning{{TRUE}}
+  clang_analyzer_express(clang_analyzer_iterator_position(i2)); //expected-warning{{$v.begin() + 2}}
+}
+
 void copy_and_increment1(const std::vector<int> &v) {
   auto i1 = v.begin();