NoQ 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();
+}
----------------
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<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.
> 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
