[PATCH] D77229: [Analyzer] Avoid handling of LazyCompundVals in IteratorModeling

Thu Sep 24 12:22:39 PDT 2020

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