[PATCH] D25660: [Analyzer] Checker for iterators dereferenced beyond their range.
Balogh, Ádám via cfe-commits
cfe-commits at lists.llvm.org
Thu Nov 17 06:31:05 PST 2016
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();
const auto item = find(start, v.end());
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?
More information about the cfe-commits