This is an archive of the discontinued LLVM Phabricator instance.

[Analyzer] Checker for mismatched iterators
AbandonedPublic

Authored by baloghadamsoftware on Feb 1 2017, 1:31 PM.

Details

Summary

A new checker to check whether multiple iterators which should refer to the same container refer to different ones. Here we include comparisons, constructors which take an iterator pair and any template function which takes iterator pair(s).

Diff Detail

Event Timeline

The checker itself disappeared mystically from the previous diff...

NoQ edited edge metadata.Feb 9 2017, 5:55 AM

Hello, thanks for another useful checker! I make quite a few of these mistakes regularly.

This one looks similar to the IteratorPastEnd checker, so much that i'd definitely advice re-using some code. At the very least, functions like isIterator() should definitely deserve a header somewhere in include/clang/StaticAnalyzer/Checkers.

Also, did you consider merging these checkers together into one file? Just because they have so much in common.

The usual stuff: do you already have any quality statistics, eg. how many warnings did you see emitted by this checker, how many of them are true/false positives. With that, better warning messages, and a bug reporter visitor to highlight the origin of the misplaced iterator, i think this checker should be enabled by default (go out of alpha).

lib/StaticAnalyzer/Checkers/MismatchedIteratorChecker.cpp
311

Hmm. Now i suspect that the checkBind() callback should have covered this, both here and in the previous checker. Did you try using that instead, and see if other callbacks are covered by checkBind() as well?

356

I'd consider !isLive() here due to, uhm, subtle differences we currently have between these two (there's a hard-to-fix bug that causes some symbols to never make it to the dead set, see D18860).

In D29419#671839, @NoQ wrote:

This one looks similar to the IteratorPastEnd checker, so much that i'd definitely advice re-using some code. At the very least, functions like isIterator() should definitely deserve a header somewhere in include/clang/StaticAnalyzer/Checkers.

Also, did you consider merging these checkers together into one file? Just because they have so much in common.

Yes. Actually these two checkers served for me as prototypes, but it turned out quite early that if I want to implement the most important iterator checker, thus the checker for invalidated iterators it requires a tracker structure that includes all the data we need for these two checkers. So it would be a wast of resources to duplicate these data. So now I am also working on the merged version.

Updated according to the comments.

baloghadamsoftware marked 2 inline comments as done.Feb 20 2017, 8:01 AM
baloghadamsoftware added inline comments.
lib/StaticAnalyzer/Checkers/MismatchedIteratorChecker.cpp
311

You are right. It seems to eliminate the need for checking C++ construct expressions, declarations with initial values and assignments.

hiraditya added inline comments.
lib/StaticAnalyzer/Checkers/MismatchedIteratorChecker.cpp
376

This can be rewritten as:

if (auto *N = C.generateNonFatalErrorNode(State, &Tag))
  reportMismatchedBug("Iterator access mismatched.", Iter1, C, N);
zaks.anna edited edge metadata.Feb 24 2017, 10:13 PM

So it would be a wast of resources to duplicate these data. So now I am also working on the merged version.

Would it make sense to just resume the review on the merged patch?

baloghadamsoftware marked an inline comment as done.

Superseeded by patch D31975.