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

[Balogh, Ádám via cfe-commits]

baloghadamsoftware marked 2 inline comments as done.
baloghadamsoftware added a comment.

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.

> - 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.

> - 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.

> - Our code owners often have strong opinions regarding warning message wording.

I need suggestions here.

> Then there are a few ideas on finding more bugs, which you shouldn't necessarily implement, that came up during the review, eg.:
> 
> - Alexey suspects that iterators implemented as plain pointers (commonly used in LLVM itself) might be unsupported;

I think it is supported now.

> - Alexey points out that ++/-- may be handled in a less conservative manner;

That is a future plan, but then it also results in a new name for the checker, e.g. IteratorRange.

> - 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;

}

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

> - 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.

> Did i forget anything?




https://reviews.llvm.org/D25660