[PATCH] D28771: [Analyzer] Various fixes for the IteratorPastEnd checker

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.


================
Comment at: lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp:530
+  auto value = RVal;
+  if (auto loc = value.getAs<Loc>()) {
+    value = State->getRawSVal(*loc);
----------------
baloghadamsoftware wrote:
> NoQ wrote:
> > Is there a test case for this hack?
> > 
> > I'd also consider inspecting the AST (probably before passing the values to `handleRandomIncrOrDecr()`) and making the decision based on that. Because even though this pattern ("if a value is a loc and we expect a nonloc, do an extra dereference") is present in many places in the analyzer, in most of these places it doesn't work correctly (what if we try to discriminate between `int*` and `int*&`?).
> I just want to get the sign of the integer value (if it is available). It turned out that I cannot do comparison between loc and nonloc. (Strange, because I can do anything else). After I created this hack, the Analyzer did not crash anymore on the llvm/clang code.
> 
> I do not fully understand what I should fix here and how? In this particular place we expect some integer, thus no int* or int*&.
Loc value, essentially, *is* a pointer or reference value. If you're getting a Loc, then your expectations of an integer are not met in the actual code. In this case you *want* to know why they are not met, otherwise you may avoid the crash, but do incorrect things and run into false positives. So i'd rather have this investigated carefully.

You say that you are crashing otherwise - and then it should be trivial for you to attach a debugger and `dump()` the expression for which you expect to take the integer value, and see why it suddenly has a pointer type in a particular case. From that you'd easily see what to do.

Also, crashes are often easy to auto-reduce using tools like `creduce`. Unlike false positives, which may turn into true positives during reduction.

If you still don't see the reason why your workaround is necessary and what exactly it does, could you attach a preprocessed file and an analyzer runline for the crash, so that we could have a look together?


================
Comment at: lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp:553
+
+  // When increasing by positive or decreasing by negative an iterator past its
+  // end, then it is a bug. We check for bugs before the operator call.
----------------
baloghadamsoftware wrote:
> NoQ wrote:
> > I think incrementing `end()` by `0` is not a bug (?)
> I think it is not a bug, but how to solve it properly? If I chose just greaterThanZero, then we have the same problem for decrementing end() by 0. Is it worth to create three states here?
Yep, i believe that indeed, we need three states here. There are three possible cases.


https://reviews.llvm.org/D28771