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:
> baloghadamsoftware wrote:
> > NoQ wrote:
> > > baloghadamsoftware wrote:
> > > > 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.
> > > > //seeing the relation between the bug and this function//
> > >
> > > The bug is in the feature that provides additional test coverage for the
> > > function. Due to the bug we're still at zero test coverage for the issue
> > > under discussion. Because my previous attempts at productive discussion
> > > have so far failed due to you arguing that "all tests are passing", i'm
> > > asking you for two months already to present an actual concrete test to
> > > cover the issue. That's it. That's the relation.
> > Sorry, if you feel that your attempts at productive discussion have so far
> > failed. Actually, I try to do everything you suggest just to push this my
> > patches forward. I try to implement the test cases you suggest with my best
> > effort and if they fail, then I fix them. However, this far I have seen no
> > test cases which pass before and fail after this particular patch. I
> > understood your concern with the arguments and I changed the code. However,
> > I still do not understand it for return values. Is it OK if I check the AST
> > return type for `CXXRecordType`? It does not seem to solve anything (it
> > does not solve the bug you described), but if you require that, then I will
> > do it. I worked months to establish an infrastructure to be able to replace
> > my previous solution which was based on `LazyCompundVal`s. This patch is
> > just what you wanted before continuing my other patches: use this new
> > infrastructure instead of reaching the region behind `LazyCompoundVal`s.
> > All my patches are stopped because of this one. The checkers give false
> > positives, there are no note tags for iterator changes. All I want is to
> > fix these issues, but if you cannot explain me what is actually wrong with
> > this one, then these checkers remain unusable. Our company is committed to
> > develop in upstream, but I feel that my efforts are in vain to upstream my
> > work.
> Now I created D88216 and here I got a few failures. I will look up at them,
> of course.
>
> Sorry, if you think that it is an ignorance and arrogance on my side that I
> say that "all tests are passing". *It is not!* I am just struggling to
> understand you what is wrong in my approach. Our technical background about
> the C++ language and the analyzer are very different. Also, as I mentioned I
> am not a good tester. You claim that I included *zero* test cases when I
> really thought that I implemented a *full coverage*. That is the purpose of
> the review: if I forgot something, then you notice politely that I forgot to
> test it for some cases and it will not work. There is no need to be harsh and
> aggressive with someone who did not offend you. I feel that I am still
> learning the Analyzer. There are more and more parts that became clearer
> during the years, but I am still very far from a fully understanding. I do
> not think that I deserve this tone just because of that.
>
> Even after rereading you comments I am not sure what do you mean I am missing
> here. Should I check for the AST type of the return value? Or should I check
> whether it is really an iterator? To me the return value seems "binary" in
> the sense that either the return value is a `LazyCompoundVal` *and* we have
> az object under construction *or* the return value is something else (A
> symbol, a region or an `Unknown`) *and* we do not have any object under
> construction. Is this incorrect then?
> To me the return value seems "binary" in the sense that either the return
> value is a LazyCompoundVal *and* we have az object under construction *or*
> the return value is something else (A symbol, a region or an Unknown) *and*
> we do not have any object under construction. Is this incorrect then?
Yes, it is. In some cases the return value is `Unknown`. Not much more use for
us than `LazyCompoundVal`. (I still not fully understand in which cases we get
`Unknown`.)
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
