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 &Call) {
+ Optional<SVal> 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<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.
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
