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();
+}
----------------
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<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.
> 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 did not. However, this function just looks for
> object under construction in the construction context of the function. If the
> function returns an object by value, then there will be one. In other cases
> there will be none. I do not see how this relates to pointers and
> //glvalues// and //prvalues//. For parameters you were fully right and I
> fixed that.
OK. I tested it now on master. It is exactly the same bug. It is no wonder,
because for non-objects such as pointers this function is exactly the same as
`Call.getReturnValue()` since there are no objects under construction at all in
this case. I will debug and fix the bug now and rebase this patch on that fix.
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
