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:
> > > 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?
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
