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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
