[PATCH] D77229: [Analyzer] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware updated this revision to Diff 294156.
baloghadamsoftware added a comment.

Test coverage was hugely increased by rebasing this patch to D88216 
. Two kind of tests failed. This last update 
addreses them:

1. Handle the assignment operator of iterator objects. This is necessary for 
assignments more complex than an assignment to a variable (which is handled by 
`checkBind()`), such as an assignment to a structure/class member.

2. Put back post-checking of `MaterializeTemporaryExpr`. This special kind of 
expression is not only used to materialize temporary expressions from 
`LazyCompoundVal`s but also for temporaries as return values.

Furthermore I added some comments explaining the functions which retrieve an 
iterator argument and iterator return value.

Now all tests pass again with the increased coverage. @NoQ if you are still in 
doubt, please suggest me further test cases to explore. I understand your 
concerns but cannot address them without some suggestions where my solution 
could fail.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229

Files:
  clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/DebugIteratorModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/InvalidatedIteratorChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/Iterator.cpp
  clang/lib/StaticAnalyzer/Checkers/Iterator.h
  clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/IteratorRangeChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/MismatchedIteratorChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/STLAlgorithmModeling.cpp
  clang/test/Analysis/iterator-modeling.cpp

Index: clang/test/Analysis/iterator-modeling.cpp
===
--- clang/test/Analysis/iterator-modeling.cpp
+++ clang/test/Analysis/iterator-modeling.cpp
@@ -42,7 +42,7 @@
 
   if (i != v.begin()) {
 clang_analyzer_warnIfReached();
-  }
+}
 }
 
 void end(const std::vector ) {
@@ -1034,6 +1034,7 @@
 
   clang_analyzer_express(clang_analyzer_iterator_position(i0)); // expected-warning-re {{$V.begin(){{$
   // clang_analyzer_express(clang_analyzer_iterator_position(i3)); FIXME: expecte warning $i1 - 1
+
 }
 
 void vector_insert_ahead_of_end(std::vector , int n) {
@@ -1942,6 +1943,55 @@
   clang_analyzer_eval(clang_analyzer_iterator_container(j) == ); // expected-warning{{TRUE}}
 }
 
+template 
+struct delegated_ctor_iterator {
+  delegated_ctor_iterator(const T&, int);
+  delegated_ctor_iterator(const T& t) : delegated_ctor_iterator(t, 0) {}
+  delegated_ctor_iterator operator++();
+  delegated_ctor_iterator operator++(int);
+  T& operator*();
+};
+
+template 
+struct container_with_delegated_ctor_iterator {
+  typedef delegated_ctor_iterator iterator;
+  iterator begin() const { return delegated_ctor_iterator(T()); }
+};
+
+void
+test_delegated_ctor_iterator(
+const container_with_delegated_ctor_iterator ) {
+  auto i = c.begin(); // no-crash
+  clang_analyzer_denote(clang_analyzer_container_begin(c), "$c.begin()");
+  clang_analyzer_express(clang_analyzer_iterator_position(i)); // expected-warning{{$c.begin()}}
+}
+
+template 
+struct base_ctor_iterator {
+  base_ctor_iterator(const T&);
+  base_ctor_iterator operator++();
+  base_ctor_iterator operator++(int);
+  T& operator*();
+};
+
+template 
+struct derived_ctor_iterator: public base_ctor_iterator {
+  derived_ctor_iterator(const T& t) : base_ctor_iterator(t) {}
+};
+
+template 
+struct container_with_derived_ctor_iterator {
+  typedef derived_ctor_iterator iterator;
+  iterator begin() const { return derived_ctor_iterator(T()); }
+};
+
+void
+test_derived_ctor_iterator(const container_with_derived_ctor_iterator ) {
+  auto i = c.begin();
+  clang_analyzer_denote(clang_analyzer_container_begin(c), "$c.begin()");
+  clang_analyzer_express(clang_analyzer_iterator_position(i)); // expected-warning{{$c.begin()}}
+}
+
 void clang_analyzer_printState();
 
 void print_state(std::vector ) {
Index: clang/lib/StaticAnalyzer/Checkers/STLAlgorithmModeling.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/STLAlgorithmModeling.cpp
+++ clang/lib/StaticAnalyzer/Checkers/STLAlgorithmModeling.cpp
@@ -24,12 +24,12 @@
 namespace {
 
 class STLAlgorithmModeling : public Checker {
-  bool evalFind(CheckerContext , const CallExpr *CE) const;
+  void evalFind(CheckerContext , const CallExpr *CE, SVal Begin,
+SVal End) const;
 
-  void Find(CheckerContext , const CallExpr *CE, unsigned paramNum) const;
-
-  using FnCheck = bool (STLAlgorithmModeling::*)(CheckerContext &,
-const CallExpr *) const;
+  using FnCheck = void (STLAlgorithmModeling::*)(CheckerContext &,
+ const CallExpr *, SVal Begin,
+   

[PATCH] D77229: [Analyzer] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/Iterator.cpp:330-336
+SVal getReturnIterator(const CallEvent ) {
+  Optional RetValUnderConstr = Call.getReturnValueUnderConstruction();
+  if (RetValUnderConstr.hasValue())
+return *RetValUnderConstr;
+
+  return Call.getReturnValue();
+}

baloghadamsoftware wrote:
> baloghadamsoftware wrote:
> > NoQ wrote:
> > > baloghadamsoftware wrote:
> > > > baloghadamsoftware wrote:
> > > > > NoQ wrote:
> > > > > > baloghadamsoftware wrote:
> > > > > > > NoQ wrote:
> > > > > > > > NoQ wrote:
> > > > > > > > > NoQ wrote:
> > > > > > > > > > I still believe you have not addressed the problem while 
> > > > > > > > > > moving the functions from D81718 to this patch. The caller 
> > > > > > > > > > of this function has no way of knowing whether the return 
> > > > > > > > > > value is the prvalue of the iterator or the glvalue of the 
> > > > > > > > > > iterator.
> > > > > > > > > > 
> > > > > > > > > > Looks like most callers are safe because they expect the 
> > > > > > > > > > object of interest to also be already tracked. But it's 
> > > > > > > > > > quite possible that both are tracked, say:
> > > > > > > > > > 
> > > > > > > > > > ```lang=c++
> > > > > > > > > >   Container1 container1 = ...;
> > > > > > > > > >   Container2 container2 = { 
> > > > > > > > > > container1.begin() };
> > > > > > > > > >   container2.begin(); // ???
> > > > > > > > > > ```
> > > > > > > > > > 
> > > > > > > > > > Suppose `Container1::iterator` is implemented as an object 
> > > > > > > > > > and `Container2::iterator` is implemented as a pointer. In 
> > > > > > > > > > this case `getIteratorPosition(getReturnIterator())` would 
> > > > > > > > > > yield the position of `container1.begin()` whereas the 
> > > > > > > > > > correct answer is the position of `container2.begin()`.
> > > > > > > > > > 
> > > > > > > > > > This problem may seem artificial but it is trivial to avoid 
> > > > > > > > > > if you simply stop defending your convoluted solution of 
> > > > > > > > > > looking at value classes instead of AST types.
> > > > > > > > > Ugh, the problem is much worse. D82185 is entirely broken for 
> > > > > > > > > the exact reason i described above and you only didn't notice 
> > > > > > > > > it because you wrote almost no tests.
> > > > > > > > > 
> > > > > > > > > Consider the test you've added in D82185:
> > > > > > > > > 
> > > > > > > > > ```lang=c++
> > > > > > > > > void begin_ptr_iterator(const cont_with_ptr_iterator ) 
> > > > > > > > > {
> > > > > > > > >   auto i = c.begin();
> > > > > > > > > 
> > > > > > > > >   clang_analyzer_eval(clang_analyzer_iterator_container(i) == 
> > > > > > > > > ); // expected-warning{{TRUE}}
> > > > > > > > > }
> > > > > > > > > ```
> > > > > > > > > 
> > > > > > > > > It breaks very easily if you modify it slightly:
> > > > > > > > > ```lang=c++
> > > > > > > > > void begin_ptr_iterator(const cont_with_ptr_iterator ) 
> > > > > > > > > {
> > > > > > > > >   auto i = c.begin();
> > > > > > > > >   ++i; // <==
> > > > > > > > > 
> > > > > > > > >   clang_analyzer_eval(clang_analyzer_iterator_container(i) == 
> > > > > > > > > ); // Says FALSE!
> > > > > > > > > }
> > > > > > > > > ```
> > > > > > > > > The iterator obviously still points to the same container, so 
> > > > > > > > > why does the test fail? Because you're tracking the wrong 
> > > > > > > > > iterator: you treated your `{conj_$3}` as a glvalue 
> > > > > > > > > whereas you should have treated it as a prvalue. In other 
> > > > > > > > > words, your checker thinks that `{conj_$3}` is the 
> > > > > > > > > location of an iterator object rather than the iterator 
> > > > > > > > > itself, and after you increment the pointer it thinks that 
> > > > > > > > > it's a completely unrelated iterator.
> > > > > > > > > 
> > > > > > > > > There's a separate concern about why does it say `FALSE` 
> > > > > > > > > (should be `UNKNOWN`) but you get the point.
> > > > > > > > The better way to test D82185 would be to make all existing 
> > > > > > > > tests with iterator objects pass with iterator pointers as 
> > > > > > > > well. Like, make existing container mocks use either iterator 
> > > > > > > > objects or iterator pointers depending on a macro and make two 
> > > > > > > > run-lines in each test file, one with `-D` and one without it. 
> > > > > > > > Most of the old tests should have worked out of the box if you 
> > > > > > > > did it right; the few that don't pass would be hidden under 
> > > > > > > > #ifdef for future investigation.
> > > > > > > Thank you for your review and especially for this tip! It is 
> > > > > > > really a good idea. I changed it now and it indeed shows the 
> > > > > > > problem you reported. It seems that my checker mixes up the 
> > > > > > > region of the pointer-typed variable (`` and ``) with the 
> > > > > > > region they point to (`{reg_$1 > > > > > > 

[PATCH] D77229: [Analyzer] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/Iterator.cpp:330-336
+SVal getReturnIterator(const CallEvent ) {
+  Optional RetValUnderConstr = Call.getReturnValueUnderConstruction();
+  if (RetValUnderConstr.hasValue())
+return *RetValUnderConstr;
+
+  return Call.getReturnValue();
+}

baloghadamsoftware wrote:
> NoQ wrote:
> > baloghadamsoftware wrote:
> > > baloghadamsoftware wrote:
> > > > NoQ wrote:
> > > > > baloghadamsoftware wrote:
> > > > > > NoQ wrote:
> > > > > > > NoQ wrote:
> > > > > > > > NoQ wrote:
> > > > > > > > > I still believe you have not addressed the problem while 
> > > > > > > > > moving the functions from D81718 to this patch. The caller of 
> > > > > > > > > this function has no way of knowing whether the return value 
> > > > > > > > > is the prvalue of the iterator or the glvalue of the iterator.
> > > > > > > > > 
> > > > > > > > > Looks like most callers are safe because they expect the 
> > > > > > > > > object of interest to also be already tracked. But it's quite 
> > > > > > > > > possible that both are tracked, say:
> > > > > > > > > 
> > > > > > > > > ```lang=c++
> > > > > > > > >   Container1 container1 = ...;
> > > > > > > > >   Container2 container2 = { 
> > > > > > > > > container1.begin() };
> > > > > > > > >   container2.begin(); // ???
> > > > > > > > > ```
> > > > > > > > > 
> > > > > > > > > Suppose `Container1::iterator` is implemented as an object 
> > > > > > > > > and `Container2::iterator` is implemented as a pointer. In 
> > > > > > > > > this case `getIteratorPosition(getReturnIterator())` would 
> > > > > > > > > yield the position of `container1.begin()` whereas the 
> > > > > > > > > correct answer is the position of `container2.begin()`.
> > > > > > > > > 
> > > > > > > > > This problem may seem artificial but it is trivial to avoid 
> > > > > > > > > if you simply stop defending your convoluted solution of 
> > > > > > > > > looking at value classes instead of AST types.
> > > > > > > > Ugh, the problem is much worse. D82185 is entirely broken for 
> > > > > > > > the exact reason i described above and you only didn't notice 
> > > > > > > > it because you wrote almost no tests.
> > > > > > > > 
> > > > > > > > Consider the test you've added in D82185:
> > > > > > > > 
> > > > > > > > ```lang=c++
> > > > > > > > void begin_ptr_iterator(const cont_with_ptr_iterator ) {
> > > > > > > >   auto i = c.begin();
> > > > > > > > 
> > > > > > > >   clang_analyzer_eval(clang_analyzer_iterator_container(i) == 
> > > > > > > > ); // expected-warning{{TRUE}}
> > > > > > > > }
> > > > > > > > ```
> > > > > > > > 
> > > > > > > > It breaks very easily if you modify it slightly:
> > > > > > > > ```lang=c++
> > > > > > > > void begin_ptr_iterator(const cont_with_ptr_iterator ) {
> > > > > > > >   auto i = c.begin();
> > > > > > > >   ++i; // <==
> > > > > > > > 
> > > > > > > >   clang_analyzer_eval(clang_analyzer_iterator_container(i) == 
> > > > > > > > ); // Says FALSE!
> > > > > > > > }
> > > > > > > > ```
> > > > > > > > The iterator obviously still points to the same container, so 
> > > > > > > > why does the test fail? Because you're tracking the wrong 
> > > > > > > > iterator: you treated your `{conj_$3}` as a glvalue 
> > > > > > > > whereas you should have treated it as a prvalue. In other 
> > > > > > > > words, your checker thinks that `{conj_$3}` is the 
> > > > > > > > location of an iterator object rather than the iterator itself, 
> > > > > > > > and after you increment the pointer it thinks that it's a 
> > > > > > > > completely unrelated iterator.
> > > > > > > > 
> > > > > > > > There's a separate concern about why does it say `FALSE` 
> > > > > > > > (should be `UNKNOWN`) but you get the point.
> > > > > > > The better way to test D82185 would be to make all existing tests 
> > > > > > > with iterator objects pass with iterator pointers as well. Like, 
> > > > > > > make existing container mocks use either iterator objects or 
> > > > > > > iterator pointers depending on a macro and make two run-lines in 
> > > > > > > each test file, one with `-D` and one without it. Most of the old 
> > > > > > > tests should have worked out of the box if you did it right; the 
> > > > > > > few that don't pass would be hidden under #ifdef for future 
> > > > > > > investigation.
> > > > > > Thank you for your review and especially for this tip! It is really 
> > > > > > a good idea. I changed it now and it indeed shows the problem you 
> > > > > > reported. It seems that my checker mixes up the region of the 
> > > > > > pointer-typed variable (`` and ``) with the region they point 
> > > > > > to (`{reg_$1 > > > > > std::vector & v>}._start>}` for `i` before the increment and 
> > > > > > `{SymRegion{reg_$1 > > > > > std::vector & v>}._start>},1 S64b,int}` for both `i` and `j` 
> > > > > > after the increment).
> > > > > > 
> > > > > > What I fail to see and I am asking you 

[PATCH] D77229: [Analyzer] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/Iterator.cpp:330-336
+SVal getReturnIterator(const CallEvent ) {
+  Optional RetValUnderConstr = Call.getReturnValueUnderConstruction();
+  if (RetValUnderConstr.hasValue())
+return *RetValUnderConstr;
+
+  return Call.getReturnValue();
+}

NoQ wrote:
> baloghadamsoftware wrote:
> > baloghadamsoftware wrote:
> > > NoQ wrote:
> > > > baloghadamsoftware wrote:
> > > > > NoQ wrote:
> > > > > > NoQ wrote:
> > > > > > > NoQ wrote:
> > > > > > > > I still believe you have not addressed the problem while moving 
> > > > > > > > the functions from D81718 to this patch. The caller of this 
> > > > > > > > function has no way of knowing whether the return value is the 
> > > > > > > > prvalue of the iterator or the glvalue of the iterator.
> > > > > > > > 
> > > > > > > > Looks like most callers are safe because they expect the object 
> > > > > > > > of interest to also be already tracked. But it's quite possible 
> > > > > > > > that both are tracked, say:
> > > > > > > > 
> > > > > > > > ```lang=c++
> > > > > > > >   Container1 container1 = ...;
> > > > > > > >   Container2 container2 = { 
> > > > > > > > container1.begin() };
> > > > > > > >   container2.begin(); // ???
> > > > > > > > ```
> > > > > > > > 
> > > > > > > > Suppose `Container1::iterator` is implemented as an object and 
> > > > > > > > `Container2::iterator` is implemented as a pointer. In this 
> > > > > > > > case `getIteratorPosition(getReturnIterator())` would yield the 
> > > > > > > > position of `container1.begin()` whereas the correct answer is 
> > > > > > > > the position of `container2.begin()`.
> > > > > > > > 
> > > > > > > > This problem may seem artificial but it is trivial to avoid if 
> > > > > > > > you simply stop defending your convoluted solution of looking 
> > > > > > > > at value classes instead of AST types.
> > > > > > > Ugh, the problem is much worse. D82185 is entirely broken for the 
> > > > > > > exact reason i described above and you only didn't notice it 
> > > > > > > because you wrote almost no tests.
> > > > > > > 
> > > > > > > Consider the test you've added in D82185:
> > > > > > > 
> > > > > > > ```lang=c++
> > > > > > > void begin_ptr_iterator(const cont_with_ptr_iterator ) {
> > > > > > >   auto i = c.begin();
> > > > > > > 
> > > > > > >   clang_analyzer_eval(clang_analyzer_iterator_container(i) == 
> > > > > > > ); // expected-warning{{TRUE}}
> > > > > > > }
> > > > > > > ```
> > > > > > > 
> > > > > > > It breaks very easily if you modify it slightly:
> > > > > > > ```lang=c++
> > > > > > > void begin_ptr_iterator(const cont_with_ptr_iterator ) {
> > > > > > >   auto i = c.begin();
> > > > > > >   ++i; // <==
> > > > > > > 
> > > > > > >   clang_analyzer_eval(clang_analyzer_iterator_container(i) == 
> > > > > > > ); // Says FALSE!
> > > > > > > }
> > > > > > > ```
> > > > > > > The iterator obviously still points to the same container, so why 
> > > > > > > does the test fail? Because you're tracking the wrong iterator: 
> > > > > > > you treated your `{conj_$3}` as a glvalue whereas you 
> > > > > > > should have treated it as a prvalue. In other words, your checker 
> > > > > > > thinks that `{conj_$3}` is the location of an iterator 
> > > > > > > object rather than the iterator itself, and after you increment 
> > > > > > > the pointer it thinks that it's a completely unrelated iterator.
> > > > > > > 
> > > > > > > There's a separate concern about why does it say `FALSE` (should 
> > > > > > > be `UNKNOWN`) but you get the point.
> > > > > > The better way to test D82185 would be to make all existing tests 
> > > > > > with iterator objects pass with iterator pointers as well. Like, 
> > > > > > make existing container mocks use either iterator objects or 
> > > > > > iterator pointers depending on a macro and make two run-lines in 
> > > > > > each test file, one with `-D` and one without it. Most of the old 
> > > > > > tests should have worked out of the box if you did it right; the 
> > > > > > few that don't pass would be hidden under #ifdef for future 
> > > > > > investigation.
> > > > > Thank you for your review and especially for this tip! It is really a 
> > > > > good idea. I changed it now and it indeed shows the problem you 
> > > > > reported. It seems that my checker mixes up the region of the 
> > > > > pointer-typed variable (`` and ``) with the region they point to 
> > > > > (`{reg_$1 & 
> > > > > v>}._start>}` for `i` before the increment and 
> > > > > `{SymRegion{reg_$1 > > > > std::vector & v>}._start>},1 S64b,int}` for both `i` and `j` 
> > > > > after the increment).
> > > > > 
> > > > > What I fail to see and I am asking you help in it is that the 
> > > > > relation between this problem and the `getReturnIterator()` function. 
> > > > > This function retrieves the object from the construction context if 
> > > > > there is one, but for plain 

[PATCH] D77229: [Analyzer] Avoid handling of LazyCompundVals in IteratorModeling

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/Iterator.cpp:330-336
+SVal getReturnIterator(const CallEvent ) {
+  Optional RetValUnderConstr = Call.getReturnValueUnderConstruction();
+  if (RetValUnderConstr.hasValue())
+return *RetValUnderConstr;
+
+  return Call.getReturnValue();
+}

baloghadamsoftware wrote:
> baloghadamsoftware wrote:
> > NoQ wrote:
> > > baloghadamsoftware wrote:
> > > > NoQ wrote:
> > > > > NoQ wrote:
> > > > > > NoQ wrote:
> > > > > > > I still believe you have not addressed the problem while moving 
> > > > > > > the functions from D81718 to this patch. The caller of this 
> > > > > > > function has no way of knowing whether the return value is the 
> > > > > > > prvalue of the iterator or the glvalue of the iterator.
> > > > > > > 
> > > > > > > Looks like most callers are safe because they expect the object 
> > > > > > > of interest to also be already tracked. But it's quite possible 
> > > > > > > that both are tracked, say:
> > > > > > > 
> > > > > > > ```lang=c++
> > > > > > >   Container1 container1 = ...;
> > > > > > >   Container2 container2 = { 
> > > > > > > container1.begin() };
> > > > > > >   container2.begin(); // ???
> > > > > > > ```
> > > > > > > 
> > > > > > > Suppose `Container1::iterator` is implemented as an object and 
> > > > > > > `Container2::iterator` is implemented as a pointer. In this case 
> > > > > > > `getIteratorPosition(getReturnIterator())` would yield the 
> > > > > > > position of `container1.begin()` whereas the correct answer is 
> > > > > > > the position of `container2.begin()`.
> > > > > > > 
> > > > > > > This problem may seem artificial but it is trivial to avoid if 
> > > > > > > you simply stop defending your convoluted solution of looking at 
> > > > > > > value classes instead of AST types.
> > > > > > Ugh, the problem is much worse. D82185 is entirely broken for the 
> > > > > > exact reason i described above and you only didn't notice it 
> > > > > > because you wrote almost no tests.
> > > > > > 
> > > > > > Consider the test you've added in D82185:
> > > > > > 
> > > > > > ```lang=c++
> > > > > > void begin_ptr_iterator(const cont_with_ptr_iterator ) {
> > > > > >   auto i = c.begin();
> > > > > > 
> > > > > >   clang_analyzer_eval(clang_analyzer_iterator_container(i) == ); 
> > > > > > // expected-warning{{TRUE}}
> > > > > > }
> > > > > > ```
> > > > > > 
> > > > > > It breaks very easily if you modify it slightly:
> > > > > > ```lang=c++
> > > > > > void begin_ptr_iterator(const cont_with_ptr_iterator ) {
> > > > > >   auto i = c.begin();
> > > > > >   ++i; // <==
> > > > > > 
> > > > > >   clang_analyzer_eval(clang_analyzer_iterator_container(i) == ); 
> > > > > > // Says FALSE!
> > > > > > }
> > > > > > ```
> > > > > > The iterator obviously still points to the same container, so why 
> > > > > > does the test fail? Because you're tracking the wrong iterator: you 
> > > > > > treated your `{conj_$3}` as a glvalue whereas you should 
> > > > > > have treated it as a prvalue. In other words, your checker thinks 
> > > > > > that `{conj_$3}` is the location of an iterator object 
> > > > > > rather than the iterator itself, and after you increment the 
> > > > > > pointer it thinks that it's a completely unrelated iterator.
> > > > > > 
> > > > > > There's a separate concern about why does it say `FALSE` (should be 
> > > > > > `UNKNOWN`) but you get the point.
> > > > > The better way to test D82185 would be to make all existing tests 
> > > > > with iterator objects pass with iterator pointers as well. Like, make 
> > > > > existing container mocks use either iterator objects or iterator 
> > > > > pointers depending on a macro and make two run-lines in each test 
> > > > > file, one with `-D` and one without it. Most of the old tests should 
> > > > > have worked out of the box if you did it right; the few that don't 
> > > > > pass would be hidden under #ifdef for future investigation.
> > > > Thank you for your review and especially for this tip! It is really a 
> > > > good idea. I changed it now and it indeed shows the problem you 
> > > > reported. It seems that my checker mixes up the region of the 
> > > > pointer-typed variable (`` and ``) with the region they point to 
> > > > (`{reg_$1 & 
> > > > v>}._start>}` for `i` before the increment and 
> > > > `{SymRegion{reg_$1 > > > std::vector & v>}._start>},1 S64b,int}` for both `i` and `j` after 
> > > > the increment).
> > > > 
> > > > What I fail to see and I am asking you help in it is that the relation 
> > > > between this problem and the `getReturnIterator()` function. This 
> > > > function retrieves the object from the construction context if there is 
> > > > one, but for plain pointers there is never one. Thus this function is 
> > > > always `Call.getReturnValue()` like before this patch.
> > > > I am asking you help
> > > 
> > > I spent way more time on that already than i find reasonable. 

[PATCH] D77229: [Analyzer] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/Iterator.cpp:330-336
+SVal getReturnIterator(const CallEvent ) {
+  Optional RetValUnderConstr = Call.getReturnValueUnderConstruction();
+  if (RetValUnderConstr.hasValue())
+return *RetValUnderConstr;
+
+  return Call.getReturnValue();
+}

baloghadamsoftware wrote:
> NoQ wrote:
> > baloghadamsoftware wrote:
> > > NoQ wrote:
> > > > NoQ wrote:
> > > > > NoQ wrote:
> > > > > > I still believe you have not addressed the problem while moving the 
> > > > > > functions from D81718 to this patch. The caller of this function 
> > > > > > has no way of knowing whether the return value is the prvalue of 
> > > > > > the iterator or the glvalue of the iterator.
> > > > > > 
> > > > > > Looks like most callers are safe because they expect the object of 
> > > > > > interest to also be already tracked. But it's quite possible that 
> > > > > > both are tracked, say:
> > > > > > 
> > > > > > ```lang=c++
> > > > > >   Container1 container1 = ...;
> > > > > >   Container2 container2 = { 
> > > > > > container1.begin() };
> > > > > >   container2.begin(); // ???
> > > > > > ```
> > > > > > 
> > > > > > Suppose `Container1::iterator` is implemented as an object and 
> > > > > > `Container2::iterator` is implemented as a pointer. In this case 
> > > > > > `getIteratorPosition(getReturnIterator())` would yield the position 
> > > > > > of `container1.begin()` whereas the correct answer is the position 
> > > > > > of `container2.begin()`.
> > > > > > 
> > > > > > This problem may seem artificial but it is trivial to avoid if you 
> > > > > > simply stop defending your convoluted solution of looking at value 
> > > > > > classes instead of AST types.
> > > > > Ugh, the problem is much worse. D82185 is entirely broken for the 
> > > > > exact reason i described above and you only didn't notice it because 
> > > > > you wrote almost no tests.
> > > > > 
> > > > > Consider the test you've added in D82185:
> > > > > 
> > > > > ```lang=c++
> > > > > void begin_ptr_iterator(const cont_with_ptr_iterator ) {
> > > > >   auto i = c.begin();
> > > > > 
> > > > >   clang_analyzer_eval(clang_analyzer_iterator_container(i) == ); // 
> > > > > expected-warning{{TRUE}}
> > > > > }
> > > > > ```
> > > > > 
> > > > > It breaks very easily if you modify it slightly:
> > > > > ```lang=c++
> > > > > void begin_ptr_iterator(const cont_with_ptr_iterator ) {
> > > > >   auto i = c.begin();
> > > > >   ++i; // <==
> > > > > 
> > > > >   clang_analyzer_eval(clang_analyzer_iterator_container(i) == ); // 
> > > > > Says FALSE!
> > > > > }
> > > > > ```
> > > > > The iterator obviously still points to the same container, so why 
> > > > > does the test fail? Because you're tracking the wrong iterator: you 
> > > > > treated your `{conj_$3}` as a glvalue whereas you should 
> > > > > have treated it as a prvalue. In other words, your checker thinks 
> > > > > that `{conj_$3}` is the location of an iterator object 
> > > > > rather than the iterator itself, and after you increment the pointer 
> > > > > it thinks that it's a completely unrelated iterator.
> > > > > 
> > > > > There's a separate concern about why does it say `FALSE` (should be 
> > > > > `UNKNOWN`) but you get the point.
> > > > The better way to test D82185 would be to make all existing tests with 
> > > > iterator objects pass with iterator pointers as well. Like, make 
> > > > existing container mocks use either iterator objects or iterator 
> > > > pointers depending on a macro and make two run-lines in each test file, 
> > > > one with `-D` and one without it. Most of the old tests should have 
> > > > worked out of the box if you did it right; the few that don't pass 
> > > > would be hidden under #ifdef for future investigation.
> > > Thank you for your review and especially for this tip! It is really a 
> > > good idea. I changed it now and it indeed shows the problem you reported. 
> > > It seems that my checker mixes up the region of the pointer-typed 
> > > variable (`` and ``) with the region they point to 
> > > (`{reg_$1 & 
> > > v>}._start>}` for `i` before the increment and 
> > > `{SymRegion{reg_$1 
> > > & v>}._start>},1 S64b,int}` for both `i` and `j` after the increment).
> > > 
> > > What I fail to see and I am asking you help in it is that the relation 
> > > between this problem and the `getReturnIterator()` function. This 
> > > function retrieves the object from the construction context if there is 
> > > one, but for plain pointers there is never one. Thus this function is 
> > > always `Call.getReturnValue()` like before this patch.
> > > I am asking you help
> > 
> > I spent way more time on that already than i find reasonable. Please figure 
> > this out on your own by fixing the bug.
> I do not see why I got so a rude answer. I was just asking help in //seeing 
> the relation between the bug and this function//. Because I do not see 

[PATCH] D77229: [Analyzer] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/Iterator.cpp:330-336
+SVal getReturnIterator(const CallEvent ) {
+  Optional RetValUnderConstr = Call.getReturnValueUnderConstruction();
+  if (RetValUnderConstr.hasValue())
+return *RetValUnderConstr;
+
+  return Call.getReturnValue();
+}

NoQ wrote:
> baloghadamsoftware wrote:
> > NoQ wrote:
> > > NoQ wrote:
> > > > NoQ wrote:
> > > > > I still believe you have not addressed the problem while moving the 
> > > > > functions from D81718 to this patch. The caller of this function has 
> > > > > no way of knowing whether the return value is the prvalue of the 
> > > > > iterator or the glvalue of the iterator.
> > > > > 
> > > > > Looks like most callers are safe because they expect the object of 
> > > > > interest to also be already tracked. But it's quite possible that 
> > > > > both are tracked, say:
> > > > > 
> > > > > ```lang=c++
> > > > >   Container1 container1 = ...;
> > > > >   Container2 container2 = { container1.begin() 
> > > > > };
> > > > >   container2.begin(); // ???
> > > > > ```
> > > > > 
> > > > > Suppose `Container1::iterator` is implemented as an object and 
> > > > > `Container2::iterator` is implemented as a pointer. In this case 
> > > > > `getIteratorPosition(getReturnIterator())` would yield the position 
> > > > > of `container1.begin()` whereas the correct answer is the position of 
> > > > > `container2.begin()`.
> > > > > 
> > > > > This problem may seem artificial but it is trivial to avoid if you 
> > > > > simply stop defending your convoluted solution of looking at value 
> > > > > classes instead of AST types.
> > > > Ugh, the problem is much worse. D82185 is entirely broken for the exact 
> > > > reason i described above and you only didn't notice it because you 
> > > > wrote almost no tests.
> > > > 
> > > > Consider the test you've added in D82185:
> > > > 
> > > > ```lang=c++
> > > > void begin_ptr_iterator(const cont_with_ptr_iterator ) {
> > > >   auto i = c.begin();
> > > > 
> > > >   clang_analyzer_eval(clang_analyzer_iterator_container(i) == ); // 
> > > > expected-warning{{TRUE}}
> > > > }
> > > > ```
> > > > 
> > > > It breaks very easily if you modify it slightly:
> > > > ```lang=c++
> > > > void begin_ptr_iterator(const cont_with_ptr_iterator ) {
> > > >   auto i = c.begin();
> > > >   ++i; // <==
> > > > 
> > > >   clang_analyzer_eval(clang_analyzer_iterator_container(i) == ); // 
> > > > Says FALSE!
> > > > }
> > > > ```
> > > > The iterator obviously still points to the same container, so why does 
> > > > the test fail? Because you're tracking the wrong iterator: you treated 
> > > > your `{conj_$3}` as a glvalue whereas you should have treated 
> > > > it as a prvalue. In other words, your checker thinks that 
> > > > `{conj_$3}` is the location of an iterator object rather than 
> > > > the iterator itself, and after you increment the pointer it thinks that 
> > > > it's a completely unrelated iterator.
> > > > 
> > > > There's a separate concern about why does it say `FALSE` (should be 
> > > > `UNKNOWN`) but you get the point.
> > > The better way to test D82185 would be to make all existing tests with 
> > > iterator objects pass with iterator pointers as well. Like, make existing 
> > > container mocks use either iterator objects or iterator pointers 
> > > depending on a macro and make two run-lines in each test file, one with 
> > > `-D` and one without it. Most of the old tests should have worked out of 
> > > the box if you did it right; the few that don't pass would be hidden 
> > > under #ifdef for future investigation.
> > Thank you for your review and especially for this tip! It is really a good 
> > idea. I changed it now and it indeed shows the problem you reported. It 
> > seems that my checker mixes up the region of the pointer-typed variable 
> > (`` and ``) with the region they point to (`{reg_$1 > SymRegion{reg_$0 & v>}._start>}` for `i` before the 
> > increment and `{SymRegion{reg_$1 > std::vector & v>}._start>},1 S64b,int}` for both `i` and `j` after the 
> > increment).
> > 
> > What I fail to see and I am asking you help in it is that the relation 
> > between this problem and the `getReturnIterator()` function. This function 
> > retrieves the object from the construction context if there is one, but for 
> > plain pointers there is never one. Thus this function is always 
> > `Call.getReturnValue()` like before this patch.
> > I am asking you help
> 
> I spent way more time on that already than i find reasonable. Please figure 
> this out on your own by fixing the bug.
I do not see why I got so a rude answer. I was just asking help in //seeing the 
relation between the bug and this function//. Because I do not see any. I think 
the bug is somewhere in handling unary and binary operators for pointers. I 
struggled with that part for this same reason and I thought I solved it but now 
I see that I 

[PATCH] D77229: [Analyzer] Avoid handling of LazyCompundVals in IteratorModeling

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/Iterator.cpp:330-336
+SVal getReturnIterator(const CallEvent ) {
+  Optional RetValUnderConstr = Call.getReturnValueUnderConstruction();
+  if (RetValUnderConstr.hasValue())
+return *RetValUnderConstr;
+
+  return Call.getReturnValue();
+}

baloghadamsoftware wrote:
> NoQ wrote:
> > NoQ wrote:
> > > NoQ wrote:
> > > > I still believe you have not addressed the problem while moving the 
> > > > functions from D81718 to this patch. The caller of this function has no 
> > > > way of knowing whether the return value is the prvalue of the iterator 
> > > > or the glvalue of the iterator.
> > > > 
> > > > Looks like most callers are safe because they expect the object of 
> > > > interest to also be already tracked. But it's quite possible that both 
> > > > are tracked, say:
> > > > 
> > > > ```lang=c++
> > > >   Container1 container1 = ...;
> > > >   Container2 container2 = { container1.begin() };
> > > >   container2.begin(); // ???
> > > > ```
> > > > 
> > > > Suppose `Container1::iterator` is implemented as an object and 
> > > > `Container2::iterator` is implemented as a pointer. In this case 
> > > > `getIteratorPosition(getReturnIterator())` would yield the position of 
> > > > `container1.begin()` whereas the correct answer is the position of 
> > > > `container2.begin()`.
> > > > 
> > > > This problem may seem artificial but it is trivial to avoid if you 
> > > > simply stop defending your convoluted solution of looking at value 
> > > > classes instead of AST types.
> > > Ugh, the problem is much worse. D82185 is entirely broken for the exact 
> > > reason i described above and you only didn't notice it because you wrote 
> > > almost no tests.
> > > 
> > > Consider the test you've added in D82185:
> > > 
> > > ```lang=c++
> > > void begin_ptr_iterator(const cont_with_ptr_iterator ) {
> > >   auto i = c.begin();
> > > 
> > >   clang_analyzer_eval(clang_analyzer_iterator_container(i) == ); // 
> > > expected-warning{{TRUE}}
> > > }
> > > ```
> > > 
> > > It breaks very easily if you modify it slightly:
> > > ```lang=c++
> > > void begin_ptr_iterator(const cont_with_ptr_iterator ) {
> > >   auto i = c.begin();
> > >   ++i; // <==
> > > 
> > >   clang_analyzer_eval(clang_analyzer_iterator_container(i) == ); // 
> > > Says FALSE!
> > > }
> > > ```
> > > The iterator obviously still points to the same container, so why does 
> > > the test fail? Because you're tracking the wrong iterator: you treated 
> > > your `{conj_$3}` as a glvalue whereas you should have treated 
> > > it as a prvalue. In other words, your checker thinks that 
> > > `{conj_$3}` is the location of an iterator object rather than 
> > > the iterator itself, and after you increment the pointer it thinks that 
> > > it's a completely unrelated iterator.
> > > 
> > > There's a separate concern about why does it say `FALSE` (should be 
> > > `UNKNOWN`) but you get the point.
> > The better way to test D82185 would be to make all existing tests with 
> > iterator objects pass with iterator pointers as well. Like, make existing 
> > container mocks use either iterator objects or iterator pointers depending 
> > on a macro and make two run-lines in each test file, one with `-D` and one 
> > without it. Most of the old tests should have worked out of the box if you 
> > did it right; the few that don't pass would be hidden under #ifdef for 
> > future investigation.
> Thank you for your review and especially for this tip! It is really a good 
> idea. I changed it now and it indeed shows the problem you reported. It seems 
> that my checker mixes up the region of the pointer-typed variable (`` and 
> ``) with the region they point to (`{reg_$1 SymRegion{reg_$0 & v>}._start>}` for `i` before the 
> increment and `{SymRegion{reg_$1 std::vector & v>}._start>},1 S64b,int}` for both `i` and `j` after the 
> increment).
> 
> What I fail to see and I am asking you help in it is that the relation 
> between this problem and the `getReturnIterator()` function. This function 
> retrieves the object from the construction context if there is one, but for 
> plain pointers there is never one. Thus this function is always 
> `Call.getReturnValue()` like before this patch.
> I am asking you help

I spent way more time on that already than i find reasonable. Please figure 
this out on your own by fixing the bug.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77229: [Analyzer] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware marked an inline comment as done.
baloghadamsoftware added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/Iterator.cpp:330-336
+SVal getReturnIterator(const CallEvent ) {
+  Optional RetValUnderConstr = Call.getReturnValueUnderConstruction();
+  if (RetValUnderConstr.hasValue())
+return *RetValUnderConstr;
+
+  return Call.getReturnValue();
+}

NoQ wrote:
> NoQ wrote:
> > NoQ wrote:
> > > I still believe you have not addressed the problem while moving the 
> > > functions from D81718 to this patch. The caller of this function has no 
> > > way of knowing whether the return value is the prvalue of the iterator or 
> > > the glvalue of the iterator.
> > > 
> > > Looks like most callers are safe because they expect the object of 
> > > interest to also be already tracked. But it's quite possible that both 
> > > are tracked, say:
> > > 
> > > ```lang=c++
> > >   Container1 container1 = ...;
> > >   Container2 container2 = { container1.begin() };
> > >   container2.begin(); // ???
> > > ```
> > > 
> > > Suppose `Container1::iterator` is implemented as an object and 
> > > `Container2::iterator` is implemented as a pointer. In this case 
> > > `getIteratorPosition(getReturnIterator())` would yield the position of 
> > > `container1.begin()` whereas the correct answer is the position of 
> > > `container2.begin()`.
> > > 
> > > This problem may seem artificial but it is trivial to avoid if you simply 
> > > stop defending your convoluted solution of looking at value classes 
> > > instead of AST types.
> > Ugh, the problem is much worse. D82185 is entirely broken for the exact 
> > reason i described above and you only didn't notice it because you wrote 
> > almost no tests.
> > 
> > Consider the test you've added in D82185:
> > 
> > ```lang=c++
> > void begin_ptr_iterator(const cont_with_ptr_iterator ) {
> >   auto i = c.begin();
> > 
> >   clang_analyzer_eval(clang_analyzer_iterator_container(i) == ); // 
> > expected-warning{{TRUE}}
> > }
> > ```
> > 
> > It breaks very easily if you modify it slightly:
> > ```lang=c++
> > void begin_ptr_iterator(const cont_with_ptr_iterator ) {
> >   auto i = c.begin();
> >   ++i; // <==
> > 
> >   clang_analyzer_eval(clang_analyzer_iterator_container(i) == ); // Says 
> > FALSE!
> > }
> > ```
> > The iterator obviously still points to the same container, so why does the 
> > test fail? Because you're tracking the wrong iterator: you treated your 
> > `{conj_$3}` as a glvalue whereas you should have treated it as a 
> > prvalue. In other words, your checker thinks that `{conj_$3}` is 
> > the location of an iterator object rather than the iterator itself, and 
> > after you increment the pointer it thinks that it's a completely unrelated 
> > iterator.
> > 
> > There's a separate concern about why does it say `FALSE` (should be 
> > `UNKNOWN`) but you get the point.
> The better way to test D82185 would be to make all existing tests with 
> iterator objects pass with iterator pointers as well. Like, make existing 
> container mocks use either iterator objects or iterator pointers depending on 
> a macro and make two run-lines in each test file, one with `-D` and one 
> without it. Most of the old tests should have worked out of the box if you 
> did it right; the few that don't pass would be hidden under #ifdef for future 
> investigation.
Thank you for your review and especially for this tip! It is really a good 
idea. I changed it now and it indeed shows the problem you reported. It seems 
that my checker mixes up the region of the pointer-typed variable (`` and 
``) with the region they point to (`{reg_$1 & v>}._start>}` for `i` before the 
increment and `{SymRegion{reg_$1 & v>}._start>},1 S64b,int}` for both `i` and `j` after the 
increment).

What I fail to see and I am asking you help in it is that the relation between 
this problem and the `getReturnIterator()` function. This function retrieves 
the object from the construction context if there is one, but for plain 
pointers there is never one. Thus this function is always 
`Call.getReturnValue()` like before this patch.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77229: [Analyzer] Avoid handling of LazyCompundVals in IteratorModeling

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/Iterator.cpp:330-336
+SVal getReturnIterator(const CallEvent ) {
+  Optional RetValUnderConstr = Call.getReturnValueUnderConstruction();
+  if (RetValUnderConstr.hasValue())
+return *RetValUnderConstr;
+
+  return Call.getReturnValue();
+}

NoQ wrote:
> NoQ wrote:
> > I still believe you have not addressed the problem while moving the 
> > functions from D81718 to this patch. The caller of this function has no way 
> > of knowing whether the return value is the prvalue of the iterator or the 
> > glvalue of the iterator.
> > 
> > Looks like most callers are safe because they expect the object of interest 
> > to also be already tracked. But it's quite possible that both are tracked, 
> > say:
> > 
> > ```lang=c++
> >   Container1 container1 = ...;
> >   Container2 container2 = { container1.begin() };
> >   container2.begin(); // ???
> > ```
> > 
> > Suppose `Container1::iterator` is implemented as an object and 
> > `Container2::iterator` is implemented as a pointer. In this case 
> > `getIteratorPosition(getReturnIterator())` would yield the position of 
> > `container1.begin()` whereas the correct answer is the position of 
> > `container2.begin()`.
> > 
> > This problem may seem artificial but it is trivial to avoid if you simply 
> > stop defending your convoluted solution of looking at value classes instead 
> > of AST types.
> Ugh, the problem is much worse. D82185 is entirely broken for the exact 
> reason i described above and you only didn't notice it because you wrote 
> almost no tests.
> 
> Consider the test you've added in D82185:
> 
> ```lang=c++
> void begin_ptr_iterator(const cont_with_ptr_iterator ) {
>   auto i = c.begin();
> 
>   clang_analyzer_eval(clang_analyzer_iterator_container(i) == ); // 
> expected-warning{{TRUE}}
> }
> ```
> 
> It breaks very easily if you modify it slightly:
> ```lang=c++
> void begin_ptr_iterator(const cont_with_ptr_iterator ) {
>   auto i = c.begin();
>   ++i; // <==
> 
>   clang_analyzer_eval(clang_analyzer_iterator_container(i) == ); // Says 
> FALSE!
> }
> ```
> The iterator obviously still points to the same container, so why does the 
> test fail? Because you're tracking the wrong iterator: you treated your 
> `{conj_$3}` as a glvalue whereas you should have treated it as a 
> prvalue. In other words, your checker thinks that `{conj_$3}` is 
> the location of an iterator object rather than the iterator itself, and after 
> you increment the pointer it thinks that it's a completely unrelated iterator.
> 
> There's a separate concern about why does it say `FALSE` (should be 
> `UNKNOWN`) but you get the point.
The better way to test D82185 would be to make all existing tests with iterator 
objects pass with iterator pointers as well. Like, make existing container 
mocks use either iterator objects or iterator pointers depending on a macro and 
make two run-lines in each test file, one with `-D` and one without it. Most of 
the old tests should have worked out of the box if you did it right; the few 
that don't pass would be hidden under #ifdef for future investigation.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77229: [Analyzer] Avoid handling of LazyCompundVals in IteratorModeling

[Artem Dergachev via Phabricator via cfe-commits]

NoQ requested changes to this revision.
NoQ added inline comments.
This revision now requires changes to proceed.



Comment at: clang/lib/StaticAnalyzer/Checkers/Iterator.cpp:330-336
+SVal getReturnIterator(const CallEvent ) {
+  Optional RetValUnderConstr = Call.getReturnValueUnderConstruction();
+  if (RetValUnderConstr.hasValue())
+return *RetValUnderConstr;
+
+  return Call.getReturnValue();
+}

NoQ wrote:
> I still believe you have not addressed the problem while moving the functions 
> from D81718 to this patch. The caller of this function has no way of knowing 
> whether the return value is the prvalue of the iterator or the glvalue of the 
> iterator.
> 
> Looks like most callers are safe because they expect the object of interest 
> to also be already tracked. But it's quite possible that both are tracked, 
> say:
> 
> ```lang=c++
>   Container1 container1 = ...;
>   Container2 container2 = { container1.begin() };
>   container2.begin(); // ???
> ```
> 
> Suppose `Container1::iterator` is implemented as an object and 
> `Container2::iterator` is implemented as a pointer. In this case 
> `getIteratorPosition(getReturnIterator())` would yield the position of 
> `container1.begin()` whereas the correct answer is the position of 
> `container2.begin()`.
> 
> This problem may seem artificial but it is trivial to avoid if you simply 
> stop defending your convoluted solution of looking at value classes instead 
> of AST types.
Ugh, the problem is much worse. D82185 is entirely broken for the exact reason 
i described above and you only didn't notice it because you wrote almost no 
tests.

Consider the test you've added in D82185:

```lang=c++
void begin_ptr_iterator(const cont_with_ptr_iterator ) {
  auto i = c.begin();

  clang_analyzer_eval(clang_analyzer_iterator_container(i) == ); // 
expected-warning{{TRUE}}
}
```

It breaks very easily if you modify it slightly:
```lang=c++
void begin_ptr_iterator(const cont_with_ptr_iterator ) {
  auto i = c.begin();
  ++i; // <==

  clang_analyzer_eval(clang_analyzer_iterator_container(i) == ); // Says 
FALSE!
}
```
The iterator obviously still points to the same container, so why does the test 
fail? Because you're tracking the wrong iterator: you treated your 
`{conj_$3}` as a glvalue whereas you should have treated it as a 
prvalue. In other words, your checker thinks that `{conj_$3}` is the 
location of an iterator object rather than the iterator itself, and after you 
increment the pointer it thinks that it's a completely unrelated iterator.

There's a separate concern about why does it say `FALSE` (should be `UNKNOWN`) 
but you get the point.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77229: [Analyzer] Avoid handling of LazyCompundVals in IteratorModeling

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/Iterator.cpp:330-336
+SVal getReturnIterator(const CallEvent ) {
+  Optional RetValUnderConstr = Call.getReturnValueUnderConstruction();
+  if (RetValUnderConstr.hasValue())
+return *RetValUnderConstr;
+
+  return Call.getReturnValue();
+}

I still believe you have not addressed the problem while moving the functions 
from D81718 to this patch. The caller of this function has no way of knowing 
whether the return value is the prvalue of the iterator or the glvalue of the 
iterator.

Looks like most callers are safe because they expect the object of interest to 
also be already tracked. But it's quite possible that both are tracked, say:

```lang=c++
  Container1 container1 = ...;
  Container2 container2 = { container1.begin() };
  container2.begin(); // ???
```

Suppose `Container1::iterator` is implemented as an object and 
`Container2::iterator` is implemented as a pointer. In this case 
`getIteratorPosition(getReturnIterator())` would yield the position of 
`container1.begin()` whereas the correct answer is the position of 
`container2.begin()`.

This problem may seem artificial but it is trivial to avoid if you simply stop 
defending your convoluted solution of looking at value classes instead of AST 
types.



Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:569-570
+  if (auto L = Amount.getAs()) {
+AmountV = State->getRawSVal(*L);
+AmountVal = 
   }

When does this happen?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77229: [Analyzer] Avoid handling of LazyCompundVals in IteratorModeling

[Endre Fülöp via Phabricator via cfe-commits]

gamesh411 accepted this revision.
gamesh411 added a comment.
This revision is now accepted and ready to land.

Thanks! LGTM now.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77229: [Analyzer] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware marked an inline comment as done.
baloghadamsoftware added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/STLAlgorithmModeling.cpp:77
 
+  unsigned ArgNum = 999;
+

gamesh411 wrote:
> 999 seems a bit arbitrary here, consider using 
> std::numeric_limits::max(), or llvm::Optional.
I refactored this part. The earlier approach was very ugly and non-standard. I 
just intended to make it work as quickly as possible, but I forgot to refactor 
after that. Thank you for noticing it!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77229: [Analyzer] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware updated this revision to Diff 285326.
baloghadamsoftware added a comment.

Updated according to the latest comment.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229

Files:
  clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/InvalidatedIteratorChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/Iterator.cpp
  clang/lib/StaticAnalyzer/Checkers/Iterator.h
  clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/IteratorRangeChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/MismatchedIteratorChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/STLAlgorithmModeling.cpp
  clang/test/Analysis/iterator-modeling.cpp

Index: clang/test/Analysis/iterator-modeling.cpp
===
--- clang/test/Analysis/iterator-modeling.cpp
+++ clang/test/Analysis/iterator-modeling.cpp
@@ -1990,6 +1990,55 @@
 ++i0;
 }
 
+template 
+struct delegated_ctor_iterator {
+  delegated_ctor_iterator(const T&, int);
+  delegated_ctor_iterator(const T& t) : delegated_ctor_iterator(t, 0) {}
+  delegated_ctor_iterator operator++();
+  delegated_ctor_iterator operator++(int);
+  T& operator*();
+};
+
+template 
+struct container_with_delegated_ctor_iterator {
+  typedef delegated_ctor_iterator iterator;
+  iterator begin() const { return delegated_ctor_iterator(T()); }
+};
+
+void
+test_delegated_ctor_iterator(
+const container_with_delegated_ctor_iterator ) {
+  auto i = c.begin(); // no-crash
+  clang_analyzer_denote(clang_analyzer_container_begin(c), "$c.begin()");
+  clang_analyzer_express(clang_analyzer_iterator_position(i)); // expected-warning{{$c.begin()}}
+}
+
+template 
+struct base_ctor_iterator {
+  base_ctor_iterator(const T&);
+  base_ctor_iterator operator++();
+  base_ctor_iterator operator++(int);
+  T& operator*();
+};
+
+template 
+struct derived_ctor_iterator: public base_ctor_iterator {
+  derived_ctor_iterator(const T& t) : base_ctor_iterator(t) {}
+};
+
+template 
+struct container_with_derived_ctor_iterator {
+  typedef derived_ctor_iterator iterator;
+  iterator begin() const { return derived_ctor_iterator(T()); }
+};
+
+void
+test_derived_ctor_iterator(const container_with_derived_ctor_iterator ) {
+  auto i = c.begin();
+  clang_analyzer_denote(clang_analyzer_container_begin(c), "$c.begin()");
+  clang_analyzer_express(clang_analyzer_iterator_position(i)); // expected-warning{{$c.begin()}}
+}
+
 void clang_analyzer_printState();
 
 void print_state(std::vector ) {
Index: clang/lib/StaticAnalyzer/Checkers/STLAlgorithmModeling.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/STLAlgorithmModeling.cpp
+++ clang/lib/StaticAnalyzer/Checkers/STLAlgorithmModeling.cpp
@@ -24,12 +24,12 @@
 namespace {
 
 class STLAlgorithmModeling : public Checker {
-  bool evalFind(CheckerContext , const CallExpr *CE) const;
+  void evalFind(CheckerContext , const CallExpr *CE, SVal Begin,
+SVal End) const;
 
-  void Find(CheckerContext , const CallExpr *CE, unsigned paramNum) const;
-
-  using FnCheck = bool (STLAlgorithmModeling::*)(CheckerContext &,
-const CallExpr *) const;
+  using FnCheck = void (STLAlgorithmModeling::*)(CheckerContext &,
+ const CallExpr *, SVal Begin,
+ SVal End) const;
 
   const CallDescriptionMap Callbacks = {
 {{{"std", "find"}, 3}, ::evalFind},
@@ -67,51 +67,45 @@
 
 bool STLAlgorithmModeling::evalCall(const CallEvent ,
 CheckerContext ) const {
-  const auto *CE = dyn_cast_or_null(Call.getOriginExpr());
-  if (!CE)
-return false;
-
-  const FnCheck *Handler = Callbacks.lookup(Call);
-  if (!Handler)
-return false;
-
-  return (this->**Handler)(C, CE);
-}
-
-bool STLAlgorithmModeling::evalFind(CheckerContext ,
-const CallExpr *CE) const {
   // std::find()-like functions either take their primary range in the first
   // two parameters, or if the first parameter is "execution policy" then in
   // the second and third. This means that the second parameter must always be
   // an iterator.
-  if (!isIteratorType(CE->getArg(1)->getType()))
+  if (Call.getNumArgs() < 2 || !isIteratorType(Call.getArgExpr(1)->getType()))
 return false;
 
   // If no "execution policy" parameter is used then the first argument is the
-  // beginning of the range.
-  if (isIteratorType(CE->getArg(0)->getType())) {
-Find(C, CE, 0);
-return true;
+  // beginning of the range. If "execution policy" parmeter is used then the
+  // third argument is the end of the range.
+  unsigned ArgNum = 0;
+  if (!isIteratorType(Call.getArgExpr(0)->getType())) {
+if (Call.getNumArgs() < 3 || !isIteratorType(Call.getArgExpr(2)->getType()))
+

[PATCH] D77229: [Analyzer] Avoid handling of LazyCompundVals in IteratorModeling

[Endre Fülöp via Phabricator via cfe-commits]

gamesh411 added a comment.

Aside  from infrastructural questions which I am not qualified ( nor 
particularly knowledgeable :3 ) to address, this looks good to me.




Comment at: clang/lib/StaticAnalyzer/Checkers/STLAlgorithmModeling.cpp:77
 
+  unsigned ArgNum = 999;
+

999 seems a bit arbitrary here, consider using 
std::numeric_limits::max(), or llvm::Optional.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77229: [Analyzer] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware updated this revision to Diff 282876.
baloghadamsoftware added a comment.

`else` branch merged with inner `if`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229

Files:
  clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/InvalidatedIteratorChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/Iterator.cpp
  clang/lib/StaticAnalyzer/Checkers/Iterator.h
  clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/IteratorRangeChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/MismatchedIteratorChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/STLAlgorithmModeling.cpp

Index: clang/lib/StaticAnalyzer/Checkers/STLAlgorithmModeling.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/STLAlgorithmModeling.cpp
+++ clang/lib/StaticAnalyzer/Checkers/STLAlgorithmModeling.cpp
@@ -24,12 +24,12 @@
 namespace {
 
 class STLAlgorithmModeling : public Checker {
-  bool evalFind(CheckerContext , const CallExpr *CE) const;
+  void evalFind(CheckerContext , const CallExpr *CE, SVal Begin,
+SVal End) const;
 
-  void Find(CheckerContext , const CallExpr *CE, unsigned paramNum) const;
-
-  using FnCheck = bool (STLAlgorithmModeling::*)(CheckerContext &,
-const CallExpr *) const;
+  using FnCheck = void (STLAlgorithmModeling::*)(CheckerContext &,
+ const CallExpr *, SVal Begin,
+ SVal End) const;
 
   const CallDescriptionMap Callbacks = {
 {{{"std", "find"}, 3}, ::evalFind},
@@ -67,51 +67,53 @@
 
 bool STLAlgorithmModeling::evalCall(const CallEvent ,
 CheckerContext ) const {
-  const auto *CE = dyn_cast_or_null(Call.getOriginExpr());
-  if (!CE)
-return false;
-
-  const FnCheck *Handler = Callbacks.lookup(Call);
-  if (!Handler)
-return false;
-
-  return (this->**Handler)(C, CE);
-}
-
-bool STLAlgorithmModeling::evalFind(CheckerContext ,
-const CallExpr *CE) const {
   // std::find()-like functions either take their primary range in the first
   // two parameters, or if the first parameter is "execution policy" then in
   // the second and third. This means that the second parameter must always be
   // an iterator.
-  if (!isIteratorType(CE->getArg(1)->getType()))
+  if (Call.getNumArgs() < 2 || !isIteratorType(Call.getArgExpr(1)->getType()))
 return false;
 
+  unsigned ArgNum = 999;
+
   // If no "execution policy" parameter is used then the first argument is the
   // beginning of the range.
-  if (isIteratorType(CE->getArg(0)->getType())) {
-Find(C, CE, 0);
-return true;
+  if (isIteratorType(Call.getArgExpr(0)->getType())) {
+ArgNum = 0;
   }
 
   // If "execution policy" parameter is used then the second argument is the
   // beginning of the range.
-  if (isIteratorType(CE->getArg(2)->getType())) {
-Find(C, CE, 1);
-return true;
+  if (ArgNum > 0 &&
+  Call.getNumArgs() > 2 && isIteratorType(Call.getArgExpr(2)->getType())) {
+ArgNum = 1;
   }
 
-  return false;
+  if (ArgNum == 999)
+return false;
+
+  SVal ArgB = getIteratorArg(Call, ArgNum, C.blockCount());
+  SVal ArgE = getIteratorArg(Call, ArgNum + 1, C.blockCount());
+
+  const auto *CE = dyn_cast_or_null(Call.getOriginExpr());
+  if (!CE)
+return false;
+
+  const FnCheck *Handler = Callbacks.lookup(Call);
+  if (!Handler)
+return false;
+
+  (this->**Handler)(C, CE, ArgB, ArgE);
+  return true;
 }
 
-void STLAlgorithmModeling::Find(CheckerContext , const CallExpr *CE,
-unsigned paramNum) const {
+void STLAlgorithmModeling::evalFind(CheckerContext , const CallExpr *CE,
+SVal Begin, SVal End) const {
   auto State = C.getState();
   auto  = C.getSValBuilder();
   const auto *LCtx = C.getLocationContext();
 
   SVal RetVal = SVB.conjureSymbolVal(nullptr, CE, LCtx, C.blockCount());
-  SVal Param = State->getSVal(CE->getArg(paramNum), LCtx);
 
   auto StateFound = State->BindExpr(CE, LCtx, RetVal);
 
@@ -119,7 +121,7 @@
   // assume that in case of successful search the position of the found element
   // is not ahead of it.
   // FIXME: Reverse iterators
-  const auto *Pos = getIteratorPosition(State, Param);
+  const auto *Pos = getIteratorPosition(State, Begin);
   if (Pos) {
 StateFound = createIteratorPosition(StateFound, RetVal, Pos->getContainer(),
 CE, LCtx, C.blockCount());
@@ -135,13 +137,11 @@
 StateFound = StateFound->assume(GreaterOrEqual.castAs(), true);
   }
 
-  Param = State->getSVal(CE->getArg(paramNum + 1), LCtx);
-
   // If we have an iterator position for the range-end argument then we can
   // assume that in case of successful search the position of the 

[PATCH] D77229: [Analyzer] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware updated this revision to Diff 282626.
baloghadamsoftware added a comment.

Crash Fix: do not retrieve first iterator argument in advance. (It may happen 
that although argument count is more than zero, there is no argument at all.)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229

Files:
  clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/InvalidatedIteratorChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/Iterator.cpp
  clang/lib/StaticAnalyzer/Checkers/Iterator.h
  clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/IteratorRangeChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/MismatchedIteratorChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/STLAlgorithmModeling.cpp

Index: clang/lib/StaticAnalyzer/Checkers/STLAlgorithmModeling.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/STLAlgorithmModeling.cpp
+++ clang/lib/StaticAnalyzer/Checkers/STLAlgorithmModeling.cpp
@@ -24,12 +24,12 @@
 namespace {
 
 class STLAlgorithmModeling : public Checker {
-  bool evalFind(CheckerContext , const CallExpr *CE) const;
+  void evalFind(CheckerContext , const CallExpr *CE, SVal Begin,
+SVal End) const;
 
-  void Find(CheckerContext , const CallExpr *CE, unsigned paramNum) const;
-
-  using FnCheck = bool (STLAlgorithmModeling::*)(CheckerContext &,
-const CallExpr *) const;
+  using FnCheck = void (STLAlgorithmModeling::*)(CheckerContext &,
+ const CallExpr *, SVal Begin,
+ SVal End) const;
 
   const CallDescriptionMap Callbacks = {
 {{{"std", "find"}, 3}, ::evalFind},
@@ -67,51 +67,53 @@
 
 bool STLAlgorithmModeling::evalCall(const CallEvent ,
 CheckerContext ) const {
-  const auto *CE = dyn_cast_or_null(Call.getOriginExpr());
-  if (!CE)
-return false;
-
-  const FnCheck *Handler = Callbacks.lookup(Call);
-  if (!Handler)
-return false;
-
-  return (this->**Handler)(C, CE);
-}
-
-bool STLAlgorithmModeling::evalFind(CheckerContext ,
-const CallExpr *CE) const {
   // std::find()-like functions either take their primary range in the first
   // two parameters, or if the first parameter is "execution policy" then in
   // the second and third. This means that the second parameter must always be
   // an iterator.
-  if (!isIteratorType(CE->getArg(1)->getType()))
+  if (Call.getNumArgs() < 2 || !isIteratorType(Call.getArgExpr(1)->getType()))
 return false;
 
+  unsigned ArgNum = 999;
+
   // If no "execution policy" parameter is used then the first argument is the
   // beginning of the range.
-  if (isIteratorType(CE->getArg(0)->getType())) {
-Find(C, CE, 0);
-return true;
+  if (isIteratorType(Call.getArgExpr(0)->getType())) {
+ArgNum = 0;
   }
 
   // If "execution policy" parameter is used then the second argument is the
   // beginning of the range.
-  if (isIteratorType(CE->getArg(2)->getType())) {
-Find(C, CE, 1);
-return true;
+  if (ArgNum > 0 &&
+  Call.getNumArgs() > 2 && isIteratorType(Call.getArgExpr(2)->getType())) {
+ArgNum = 1;
   }
 
-  return false;
+  if (ArgNum == 999)
+return false;
+
+  SVal ArgB = getIteratorArg(Call, ArgNum, C.blockCount());
+  SVal ArgE = getIteratorArg(Call, ArgNum + 1, C.blockCount());
+
+  const auto *CE = dyn_cast_or_null(Call.getOriginExpr());
+  if (!CE)
+return false;
+
+  const FnCheck *Handler = Callbacks.lookup(Call);
+  if (!Handler)
+return false;
+
+  (this->**Handler)(C, CE, ArgB, ArgE);
+  return true;
 }
 
-void STLAlgorithmModeling::Find(CheckerContext , const CallExpr *CE,
-unsigned paramNum) const {
+void STLAlgorithmModeling::evalFind(CheckerContext , const CallExpr *CE,
+SVal Begin, SVal End) const {
   auto State = C.getState();
   auto  = C.getSValBuilder();
   const auto *LCtx = C.getLocationContext();
 
   SVal RetVal = SVB.conjureSymbolVal(nullptr, CE, LCtx, C.blockCount());
-  SVal Param = State->getSVal(CE->getArg(paramNum), LCtx);
 
   auto StateFound = State->BindExpr(CE, LCtx, RetVal);
 
@@ -119,7 +121,7 @@
   // assume that in case of successful search the position of the found element
   // is not ahead of it.
   // FIXME: Reverse iterators
-  const auto *Pos = getIteratorPosition(State, Param);
+  const auto *Pos = getIteratorPosition(State, Begin);
   if (Pos) {
 StateFound = createIteratorPosition(StateFound, RetVal, Pos->getContainer(),
 CE, LCtx, C.blockCount());
@@ -135,13 +137,11 @@
 StateFound = StateFound->assume(GreaterOrEqual.castAs(), true);
   }
 
-  Param = State->getSVal(CE->getArg(paramNum + 1), LCtx);
-
   // If we have an 

[PATCH] D77229: [Analyzer] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware updated this revision to Diff 274742.
baloghadamsoftware added a comment.

Rebased on `master` which now contains the handling of pointers as iterators. 
All tests passed.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229

Files:
  clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/InvalidatedIteratorChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/Iterator.cpp
  clang/lib/StaticAnalyzer/Checkers/Iterator.h
  clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/IteratorRangeChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/MismatchedIteratorChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/STLAlgorithmModeling.cpp

Index: clang/lib/StaticAnalyzer/Checkers/STLAlgorithmModeling.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/STLAlgorithmModeling.cpp
+++ clang/lib/StaticAnalyzer/Checkers/STLAlgorithmModeling.cpp
@@ -24,12 +24,12 @@
 namespace {
 
 class STLAlgorithmModeling : public Checker {
-  bool evalFind(CheckerContext , const CallExpr *CE) const;
+  void evalFind(CheckerContext , const CallExpr *CE, SVal Begin,
+SVal End) const;
 
-  void Find(CheckerContext , const CallExpr *CE, unsigned paramNum) const;
-
-  using FnCheck = bool (STLAlgorithmModeling::*)(CheckerContext &,
-const CallExpr *) const;
+  using FnCheck = void (STLAlgorithmModeling::*)(CheckerContext &,
+ const CallExpr *, SVal Begin,
+ SVal End) const;
 
   const CallDescriptionMap Callbacks = {
 {{{"std", "find"}, 3}, ::evalFind},
@@ -67,51 +67,53 @@
 
 bool STLAlgorithmModeling::evalCall(const CallEvent ,
 CheckerContext ) const {
-  const auto *CE = dyn_cast_or_null(Call.getOriginExpr());
-  if (!CE)
-return false;
-
-  const FnCheck *Handler = Callbacks.lookup(Call);
-  if (!Handler)
-return false;
-
-  return (this->**Handler)(C, CE);
-}
-
-bool STLAlgorithmModeling::evalFind(CheckerContext ,
-const CallExpr *CE) const {
   // std::find()-like functions either take their primary range in the first
   // two parameters, or if the first parameter is "execution policy" then in
   // the second and third. This means that the second parameter must always be
   // an iterator.
-  if (!isIteratorType(CE->getArg(1)->getType()))
+  if (Call.getNumArgs() < 2 || !isIteratorType(Call.getArgExpr(1)->getType()))
 return false;
 
+  unsigned ArgNum = 999;
+
   // If no "execution policy" parameter is used then the first argument is the
   // beginning of the range.
-  if (isIteratorType(CE->getArg(0)->getType())) {
-Find(C, CE, 0);
-return true;
+  if (isIteratorType(Call.getArgExpr(0)->getType())) {
+ArgNum = 0;
   }
 
   // If "execution policy" parameter is used then the second argument is the
   // beginning of the range.
-  if (isIteratorType(CE->getArg(2)->getType())) {
-Find(C, CE, 1);
-return true;
+  if (ArgNum > 0 &&
+  Call.getNumArgs() > 2 && isIteratorType(Call.getArgExpr(2)->getType())) {
+ArgNum = 1;
   }
 
-  return false;
+  if (ArgNum == 999)
+return false;
+
+  SVal ArgB = getIteratorArg(Call, ArgNum, C.blockCount());
+  SVal ArgE = getIteratorArg(Call, ArgNum + 1, C.blockCount());
+
+  const auto *CE = dyn_cast_or_null(Call.getOriginExpr());
+  if (!CE)
+return false;
+
+  const FnCheck *Handler = Callbacks.lookup(Call);
+  if (!Handler)
+return false;
+
+  (this->**Handler)(C, CE, ArgB, ArgE);
+  return true;
 }
 
-void STLAlgorithmModeling::Find(CheckerContext , const CallExpr *CE,
-unsigned paramNum) const {
+void STLAlgorithmModeling::evalFind(CheckerContext , const CallExpr *CE,
+SVal Begin, SVal End) const {
   auto State = C.getState();
   auto  = C.getSValBuilder();
   const auto *LCtx = C.getLocationContext();
 
   SVal RetVal = SVB.conjureSymbolVal(nullptr, CE, LCtx, C.blockCount());
-  SVal Param = State->getSVal(CE->getArg(paramNum), LCtx);
 
   auto StateFound = State->BindExpr(CE, LCtx, RetVal);
 
@@ -119,7 +121,7 @@
   // assume that in case of successful search the position of the found element
   // is not ahead of it.
   // FIXME: Reverse iterators
-  const auto *Pos = getIteratorPosition(State, Param);
+  const auto *Pos = getIteratorPosition(State, Begin);
   if (Pos) {
 StateFound = createIteratorPosition(StateFound, RetVal, Pos->getContainer(),
 CE, LCtx, C.blockCount());
@@ -135,13 +137,11 @@
 StateFound = StateFound->assume(GreaterOrEqual.castAs(), true);
   }
 
-  Param = State->getSVal(CE->getArg(paramNum + 1), LCtx);
-
   // If we have an iterator position for the range-end argument then we can
   // 

[PATCH] D77229: [Analyzer] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware updated this revision to Diff 272735.
baloghadamsoftware added a comment.

No new functions in `CallEvent` needed, instead new functions in the iterator 
library.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229

Files:
  clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/InvalidatedIteratorChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/Iterator.cpp
  clang/lib/StaticAnalyzer/Checkers/Iterator.h
  clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/IteratorRangeChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/MismatchedIteratorChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/STLAlgorithmModeling.cpp
  clang/test/Analysis/iterator-modeling.cpp

Index: clang/test/Analysis/iterator-modeling.cpp
===
--- clang/test/Analysis/iterator-modeling.cpp
+++ clang/test/Analysis/iterator-modeling.cpp
@@ -1862,7 +1862,7 @@
 void clang_analyzer_printState();
 
 void print_state(std::vector ) {
-  const auto i0 = V.cbegin();
+  auto i0 = V.cbegin();
   clang_analyzer_printState();
 
 // CHECK:  "checker_messages": [
@@ -1871,12 +1871,15 @@
 // CHECK-NEXT: "i0 : Valid ; Container == SymRegion{reg_$[[#]] & V>} ; Offset == conj_$[[#]]{long, LC[[#]], S[[#]], #[[#]]}"
 // CHECK-NEXT:   ]}
 
-  const auto i1 = V.cend();
+  ++i0;
+  auto i1 = V.cend();
   clang_analyzer_printState();
-  
+
 // CHECK:  "checker_messages": [
 // CHECK:   { "checker": "alpha.cplusplus.IteratorModeling", "messages": [
 // CHECK-NEXT: "Iterator Positions :",
 // CHECK-NEXT: "i1 : Valid ; Container == SymRegion{reg_$[[#]] & V>} ; Offset == conj_$[[#]]{long, LC[[#]], S[[#]], #[[#]]}"
 // CHECK-NEXT:   ]}
+
+  --i1;
 }
Index: clang/lib/StaticAnalyzer/Checkers/STLAlgorithmModeling.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/STLAlgorithmModeling.cpp
+++ clang/lib/StaticAnalyzer/Checkers/STLAlgorithmModeling.cpp
@@ -24,12 +24,12 @@
 namespace {
 
 class STLAlgorithmModeling : public Checker {
-  bool evalFind(CheckerContext , const CallExpr *CE) const;
+  void evalFind(CheckerContext , const CallExpr *CE, SVal Begin,
+SVal End) const;
 
-  void Find(CheckerContext , const CallExpr *CE, unsigned paramNum) const;
-
-  using FnCheck = bool (STLAlgorithmModeling::*)(CheckerContext &,
-const CallExpr *) const;
+  using FnCheck = void (STLAlgorithmModeling::*)(CheckerContext &,
+ const CallExpr *, SVal Begin,
+ SVal End) const;
 
   const CallDescriptionMap Callbacks = {
 {{{"std", "find"}, 3}, ::evalFind},
@@ -67,51 +67,53 @@
 
 bool STLAlgorithmModeling::evalCall(const CallEvent ,
 CheckerContext ) const {
-  const auto *CE = dyn_cast_or_null(Call.getOriginExpr());
-  if (!CE)
-return false;
-
-  const FnCheck *Handler = Callbacks.lookup(Call);
-  if (!Handler)
-return false;
-
-  return (this->**Handler)(C, CE);
-}
-
-bool STLAlgorithmModeling::evalFind(CheckerContext ,
-const CallExpr *CE) const {
   // std::find()-like functions either take their primary range in the first
   // two parameters, or if the first parameter is "execution policy" then in
   // the second and third. This means that the second parameter must always be
   // an iterator.
-  if (!isIteratorType(CE->getArg(1)->getType()))
+  if (Call.getNumArgs() < 2 || !isIteratorType(Call.getArgExpr(1)->getType()))
 return false;
 
+  unsigned ArgNum = 999;
+
   // If no "execution policy" parameter is used then the first argument is the
   // beginning of the range.
-  if (isIteratorType(CE->getArg(0)->getType())) {
-Find(C, CE, 0);
-return true;
+  if (isIteratorType(Call.getArgExpr(0)->getType())) {
+ArgNum = 0;
   }
 
   // If "execution policy" parameter is used then the second argument is the
   // beginning of the range.
-  if (isIteratorType(CE->getArg(2)->getType())) {
-Find(C, CE, 1);
-return true;
+  if (ArgNum > 0 &&
+  Call.getNumArgs() > 2 && isIteratorType(Call.getArgExpr(2)->getType())) {
+ArgNum = 1;
   }
 
-  return false;
+  if (ArgNum == 999)
+return false;
+
+  SVal ArgB = getIteratorArg(Call, ArgNum, C.blockCount());
+  SVal ArgE = getIteratorArg(Call, ArgNum + 1, C.blockCount());
+
+  const auto *CE = dyn_cast_or_null(Call.getOriginExpr());
+  if (!CE)
+return false;
+
+  const FnCheck *Handler = Callbacks.lookup(Call);
+  if (!Handler)
+return false;
+
+  (this->**Handler)(C, CE, ArgB, ArgE);
+  return true;
 }
 
-void STLAlgorithmModeling::Find(CheckerContext , const CallExpr *CE,
-unsigned paramNum) const {
+void STLAlgorithmModeling::evalFind(CheckerContext , const 

[PATCH] D77229: [Analyzer] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware updated this revision to Diff 270335.
baloghadamsoftware retitled this revision from "[Analyzer][WIP] Avoid handling 
of LazyCompundVals in IteratorModeling" to "[Analyzer] Avoid handling of 
LazyCompundVals in IteratorModeling".
baloghadamsoftware edited the summary of this revision.
baloghadamsoftware added a comment.

Rebased to previous patches and updated according to them.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229

Files:
  clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/InvalidatedIteratorChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/Iterator.cpp
  clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/IteratorRangeChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/MismatchedIteratorChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/STLAlgorithmModeling.cpp
  clang/test/Analysis/iterator-modeling.cpp

Index: clang/test/Analysis/iterator-modeling.cpp
===
--- clang/test/Analysis/iterator-modeling.cpp
+++ clang/test/Analysis/iterator-modeling.cpp
@@ -1862,7 +1862,7 @@
 void clang_analyzer_printState();
 
 void print_state(std::vector ) {
-  const auto i0 = V.cbegin();
+  auto i0 = V.cbegin();
   clang_analyzer_printState();
 
 // CHECK:  "checker_messages": [
@@ -1871,12 +1871,15 @@
 // CHECK-NEXT: "i0 : Valid ; Container == SymRegion{reg_$[[#]] & V>} ; Offset == conj_$[[#]]{long, LC[[#]], S[[#]], #[[#]]}"
 // CHECK-NEXT:   ]}
 
-  const auto i1 = V.cend();
+  ++i0;
+  auto i1 = V.cend();
   clang_analyzer_printState();
-  
+
 // CHECK:  "checker_messages": [
 // CHECK:   { "checker": "alpha.cplusplus.IteratorModeling", "messages": [
 // CHECK-NEXT: "Iterator Positions :",
 // CHECK-NEXT: "i1 : Valid ; Container == SymRegion{reg_$[[#]] & V>} ; Offset == conj_$[[#]]{long, LC[[#]], S[[#]], #[[#]]}"
 // CHECK-NEXT:   ]}
+
+  --i1;
 }
Index: clang/lib/StaticAnalyzer/Checkers/STLAlgorithmModeling.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/STLAlgorithmModeling.cpp
+++ clang/lib/StaticAnalyzer/Checkers/STLAlgorithmModeling.cpp
@@ -24,12 +24,12 @@
 namespace {
 
 class STLAlgorithmModeling : public Checker {
-  bool evalFind(CheckerContext , const CallExpr *CE) const;
+  void evalFind(CheckerContext , const CallExpr *CE, SVal Begin,
+SVal End) const;
 
-  void Find(CheckerContext , const CallExpr *CE, unsigned paramNum) const;
-
-  using FnCheck = bool (STLAlgorithmModeling::*)(CheckerContext &,
-const CallExpr *) const;
+  using FnCheck = void (STLAlgorithmModeling::*)(CheckerContext &,
+ const CallExpr *, SVal Begin,
+ SVal End) const;
 
   const CallDescriptionMap Callbacks = {
 {{{"std", "find"}, 3}, ::evalFind},
@@ -67,51 +67,53 @@
 
 bool STLAlgorithmModeling::evalCall(const CallEvent ,
 CheckerContext ) const {
-  const auto *CE = dyn_cast_or_null(Call.getOriginExpr());
-  if (!CE)
-return false;
-
-  const FnCheck *Handler = Callbacks.lookup(Call);
-  if (!Handler)
-return false;
-
-  return (this->**Handler)(C, CE);
-}
-
-bool STLAlgorithmModeling::evalFind(CheckerContext ,
-const CallExpr *CE) const {
   // std::find()-like functions either take their primary range in the first
   // two parameters, or if the first parameter is "execution policy" then in
   // the second and third. This means that the second parameter must always be
   // an iterator.
-  if (!isIteratorType(CE->getArg(1)->getType()))
+  if (Call.getNumArgs() < 2 || !isIteratorType(Call.getArgExpr(1)->getType()))
 return false;
 
+  unsigned ArgNum = 999;
+
   // If no "execution policy" parameter is used then the first argument is the
   // beginning of the range.
-  if (isIteratorType(CE->getArg(0)->getType())) {
-Find(C, CE, 0);
-return true;
+  if (isIteratorType(Call.getArgExpr(0)->getType())) {
+ArgNum = 0;
   }
 
   // If "execution policy" parameter is used then the second argument is the
   // beginning of the range.
-  if (isIteratorType(CE->getArg(2)->getType())) {
-Find(C, CE, 1);
-return true;
+  if (ArgNum > 0 &&
+  Call.getNumArgs() > 2 && isIteratorType(Call.getArgExpr(2)->getType())) {
+ArgNum = 1;
   }
 
-  return false;
+  if (ArgNum == 999)
+return false;
+
+  SVal ArgB = Call.getArgObject(ArgNum, C.blockCount());
+  SVal ArgE = Call.getArgObject(ArgNum + 1, C.blockCount());
+
+  const auto *CE = dyn_cast_or_null(Call.getOriginExpr());
+  if (!CE)
+return false;
+
+  const FnCheck *Handler = Callbacks.lookup(Call);
+  if (!Handler)
+return false;
+
+  (this->**Handler)(C, CE, ArgB, ArgE);
+  return true;
 }
 
-void