[PATCH] D25660: [Analyzer] Checker for iterators dereferenced beyond their range.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ accepted this revision.
NoQ added a reviewer: NoQ.
NoQ added a comment.
This revision is now accepted and ready to land.

In https://reviews.llvm.org/D25660#598576, @baloghadamsoftware wrote:

> In https://reviews.llvm.org/D25660#590778, @NoQ wrote:
>
> > - Agree on the `evalAssume()` implementation (i'm still not quite understanding what the problem is here, see the new inline comments);
>
>
> I think it will be settled soon.


This part makes a lot of sense to me now, cool!

Hmm, so we model `!($x)` as `$x == 0`. That's tricky. Maybe we should also consider a test like `if ( (i == v.end()) == true )`; once it's done, we're be doing as good of a job as `RangeConstraintManager` does on numeric symbols, which would be great and not worth improving further.

> 
> 
>> - We should probably not warn by default on unchecked std::find (see comments following the `push_back(2016)` example), unless some strong arguments against such code patterns are provided;
> 
> It is automatic. The role of evalCall is only to reduce the exploded graph. If I remove it, we get the same result (that is why we have a nonStdFind there, to check this case). but with far more states. Especially in case of vector, where the GNU implementation is quite complicated because of optimizations.

Yep, i agree that some kind of evalCall is useful. However, it's now causing more positives than it should, and i think this behavior needs to be eventually avoided, because false positives are very scary - eg. we should try to end up with one state instead of two. Because by splitting states, we declare the possibility of both branches, which in this case is not always correct.

> 
> 
>> - A BugReporterVisitor should be added to report iterator state changes to the user across the diagnostic path;
> 
> I also thought of this. The question is where to start the chain.

At least, the very last state update to the region that failed (without copies) should be easy to support. Copies would be tricky - i'm thinking of tagging nodes where copies happened with special program point tags that help us understand which region was the source for the copy.

>> - More checks could be implemented in this checker, eg. passing `end()` as first argument of `std::find()` is an instant bug (somebody accidentally swapped begin and end?).
> 
> Good idea, but what if it is intentional? I do not mean that we pass end() directly, but if we do a loop of find() functions where the beginning of the next range is always the successor of the last found element, we may result in a range of [end(), end()[, which I think is a valid empty range:
> 
>   const auto start = v.begin();
>   while(true) {
>      const auto item = find(start, v.end());
>      if(item==v.end())
>         break;
>      doSomething(*item);
>      start = ++item;
>   }
>    

I misread the docs, sorry><

>> A list of ideas on improving core experience, mostly for myself because i seem to be the only one with strong opinions on this:
>> 
>> - Provide a checker callback for structure copies, which would unify the multitude of similar callbacks in this checker;
> 
> A callback? Or just move the copy into a simple (or template?) function?

A callback would certainly be better, because it removes a lot of boilerplate from the checker (to subscribe to one callback instead of five would be great). But that's a future plan, not for this patch.

>> - Consider removing the conjured structure symbol hack.
> 
> Which hack do you mean here? In evalCall() of the various std functions? As I mentioned, they can be removed, but then we will get more states in the exploded graph.

I've just made an attempt in https://reviews.llvm.org/D27202.

________

I think this is good to go as an alpha checker!
I'm still in favor of more comments in this code.
One more minor inline nit.



================
Comment at: lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp:459
+  } else if (const auto TruthVal = RetVal.getAs<nonloc::ConcreteInt>()) {
+    if (State = processComparison(
+            State, getRegionOrSymbol(LVal), getRegionOrSymbol(RVal),
----------------
This produces a `-Wparentheses` warning, i think we should silence it by putting an extra `()` around operator `=` because the assignment is intentional here.


https://reviews.llvm.org/D25660