This is an archive of the discontinued LLVM Phabricator instance.

[Analyzer] Iterator Checker - Forbid decrements past the begin() and increments past the end() of containers
ClosedPublic

Authored by baloghadamsoftware on Oct 29 2018, 5:03 AM.

Details

Summary

Previously, the iterator range checker only warned upon dereferencing of iterators outside their valid range as well as increments and decrements of out-of-range iterators where the result remains out-of-range. However, the C++ standard is more strict than this: decrementing begin() or incrementing end() results in undefined behaviour even if the iterator is not dereferenced afterwards. Coming back to the range once out-of-range is also undefined.

This patch corrects the behaviour of the iterator range checker: warnings are given for any operation whose result is ahead of begin() or past the end() (which is the past-end iterator itself, thus now we are speaking of past past-end).

Diff Detail

Event Timeline

whisperity added inline comments.Oct 30 2018, 4:24 AM
lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
421–470

This is a bit of an organisational comment (and the compiler of course hopefully optimises this out), but could you, for the sake of code readability, break the if-elseif-elseif-elseif chain's common check out into an outer if, and only check for the inner parts? Thinking of something like this:

if (ChecksEnabled[CK_InvalidatedIteratorChecker])
{
   /* yadda */
}

if (ChecksEnabled[CK_IteratorRangeChecker])
{
  X* OOperator = Func->getOverloadedOperator();
  if (isIncrementOperator(OOperator))
  {
    /* yadda */
  } else if (isDecrementOperator(OOperator)) {
    /* yadda */
  }
}

/* etc. */
1075–1076

is never a, also . at the end

1577

(Minor: I don't like the name of this function, advancePosition (akin to std::advance) sounds cleaner to my ears.)

1584

For the sake of clarity, I think an assert should be introduced her, lest someone ends up shifting the position with <=.

2340–2355

How does the introduction of IncludeEnd change this behaviour? What does the parameter refer to?

This comment was removed by baloghadamsoftware.

Restored corrected patch after uploading an incorrect one.

Updated according to the comments.

baloghadamsoftware marked 5 inline comments as done.Nov 23 2018, 7:25 AM
NoQ accepted this revision.Nov 30 2018, 2:16 PM

Makes perfect sense to me!

test/Analysis/iterator-range.cpp
210

I guess we'll have to come up with a separate warning text for this case, as there's no access happening through the iterator.

Regardless of how the final message would look, i suggest adding logic for having a different message now, so that we didn't forget about this case later.

This revision is now accepted and ready to land.Nov 30 2018, 2:16 PM

Thank you for the review!

You are absolutely right, these error messages were not accurate and even misleading. Now I updated them. To achieve this I also had to separate the function isOutOfRange() into three different functions.

whisperity retitled this revision from [Analyzer] Iterator Checker - Forbid increments past the begin() and decrements past the end() of containers to [Analyzer] Iterator Checker - Forbid decrements past the begin() and increments past the end() of containers.Dec 3 2018, 4:26 AM
whisperity edited the summary of this revision. (Show Details)
whisperity set the repository for this revision to rC Clang.

One error message slightly updated.

This revision was automatically updated to reflect the committed changes.