[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 &Call) {
+  Optional<SVal> 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<T> container1 = ...;
> > >   Container2<Container1::iterator> 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<int> &c) {
> >   auto i = c.begin();
> > 
> >   clang_analyzer_eval(clang_analyzer_iterator_container(i) == &c); // expected-warning{{TRUE}}
> > }
> > ```
> > 
> > It breaks very easily if you modify it slightly:
> > ```lang=c++
> > void begin_ptr_iterator(const cont_with_ptr_iterator<int> &c) {
> >   auto i = c.begin();
> >   ++i; // <==
> > 
> >   clang_analyzer_eval(clang_analyzer_iterator_container(i) == &c); // 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 `&SymRegion{conj_$3}` as a glvalue whereas you should have treated it as a prvalue. In other words, your checker thinks that `&SymRegion{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 (`&i` and `&j`) with the region they point to (`&SymRegion{reg_$1<int * SymRegion{reg_$0<const std::vector<int> & v>}._start>}` for `i` before the increment and `&Element{SymRegion{reg_$1<int * SymRegion{reg_$0<const std::vector<int> & 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