This is an archive of the discontinued LLVM Phabricator instance.

[Analyzer] Iterator Checker - Part 4: Mismatched iterator checker for function parameters
ClosedPublic

Authored by baloghadamsoftware on May 3 2017, 11:14 PM.

Diff Detail

Repository
rC Clang

Event Timeline

NoQ added inline comments.Dec 14 2017, 3:59 PM
lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
396–414

Could you add more comments on how this machinery works, probably with examples?

Rebased to current Part 3. Comment added.

Hello Adam,
This looks like a nice improvement. I have some remarks inline.

lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
384

The function becomes > 100 lines long. Should we refactor this check into a separate function to improve readability?

390

While this assumption is sane and is true for <algorithm> functions, user code can have other design solutions. There is nothing that prevents users from writing a function looking like:

template <typename IterTy>
void f(IterTy FromBegin, IterTy FromEnd, IterTy ToBegin, IterTy ToEnd);

and there is nothing wrong with it.
One of possible solutions is to restrict checker to check only functions from std namespace. What do you think?

412

size_t I = 0?

914

We always report about first iterator, but the mismatched one can be second. I think this deserves a FIXME, at least.

1012

We usually pass StringRefs and SVals by value because they're very cheap for copying. However, the surrounding code follows the same conventions so it's not strongly required to change.

baloghadamsoftware marked an inline comment as done.Jan 17 2018, 1:30 AM
baloghadamsoftware added inline comments.
lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
384

Yes, I think so this would be a good idea. Should I do it now?

390

We can restrict, of course, but first we should measure how it performs on real code. With the restriction, we can get rid of some false positives but we may also loose some true positives.

baloghadamsoftware marked an inline comment as done.Jan 17 2018, 1:31 AM
baloghadamsoftware marked an inline comment as not done.
baloghadamsoftware marked an inline comment as done.

Updated according to some comments.

baloghadamsoftware marked an inline comment as done.Jan 17 2018, 7:01 AM
baloghadamsoftware edited reviewers, added: dcoughlin; removed: zaks.anna.Jan 17 2018, 7:41 AM

Previous rebase was wrong, this is the correct one.

george.karpenkov resigned from this revision.Jul 3 2018, 11:39 AM
george.karpenkov added a subscriber: george.karpenkov.
whisperity added inline comments.Jul 5 2018, 3:13 AM
include/clang/StaticAnalyzer/Checkers/Checkers.td
320

Is there any particular order entries of this file should be in? Seems to be broken now, but I guess when this patch comes up to the top of the stack it shall be fixed at a rebase.

lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
427

functions' parameters' ?

914

If this string is the message that gets printed to the user, I think it must be rephrased a bit. If this message came across me, I'd be puzzled at first to understand what it is trying to mean.

lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
390

One more thing:

The main purpose of iterators is to make algorithms independent of the data representation. So you can decide whether your algorithm works on a specific representation and create non-template function that takes reference for the specific container itself or you make it generic so you use template function which takes iterators. If you chose this latter one for an algorithm that works on two different container then there is no point to restrict the function to only work on two containers with exactly the same representation. Either specific or generic, but there is no point for something in between.

NoQ accepted this revision.Aug 14 2018, 3:00 PM

Looks good. I guess we may have to tone down the heuristic about "all template functions" if we see it fail.

@a.sidorin and @whisperity have some valid minor comments.

This revision is now accepted and ready to land.Aug 14 2018, 3:00 PM

Since rL338263 fixed a bug in the cleanup phase the tests for mismatched iterator checker did not pass. The reason for this is that the region of some LazyCompoundVals are cleaned up while there are still iterator positions connected to the LazyCompoundVal itself. This happens typically for arguments which are constructed in-place (e.g. begin() or end() of a container is invoked in the argument itself).

We applied a fix here that defers cleanup of such iterator positions. No other solution comes to my mind at the moment.

I wanted to upload this fix in a separate patch but I could not create tests for it.

@NoQ please review this fix before I commit the patch.

baloghadamsoftware marked 3 inline comments as done.Sep 3 2018, 2:14 AM
baloghadamsoftware added inline comments.
include/clang/StaticAnalyzer/Checkers/Checkers.td
320

Alphabetical order. Now it seems to be OK.

lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
384

I propose to refactor it later in a separate patch.

This revision was automatically updated to reflect the committed changes.
baloghadamsoftware marked an inline comment as done.