This is an archive of the discontinued LLVM Phabricator instance.

[Analyzer] Fix errors in iterator modeling
ClosedPublic

Authored by baloghadamsoftware on Jun 23 2020, 8:21 AM.

Details

Summary

There is major a bug found in iterator modeling: upon adding a value to or subtracting a value from an iterator the position of the original iterator is also changed beside the result. This patch fixes this bug.

To catch such bugs in the future we also changed the tests to look for regular expressions including an end-of-line symbol ($) so we can prevent false matches where only the tested prefix matches.

Another minor bug is that when printing the state, all the iterator positions are printed in a single line. This patch also fixes this.

Diff Detail

Event Timeline

Hey! See my inline comments, but after those and a quick clang-format, it looks good.

clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
542โ€“543

TmpState feels too generic, maybe AdvancedPosition is more descriptive? Just a suggestion.

613

I would welcome a one-line comment here, stating that this variable is here for formatting reasons (so that the messages are joined by a newline, and no prefix or postfix newline is present in the output). Or rename to LinesOutput, LinesPrinted or something similar.

clang/test/Analysis/iterator-modeling.cpp
36

It seems important to me that this test is fixed, as we were not testing whether the message really ends with $v.begin(). ๐Ÿ‘

Code reformatted according to Lint Pre-Merge check suggestions.

baloghadamsoftware edited the summary of this revision. (Show Details)

Updated according to the comments.

baloghadamsoftware marked 3 inline comments as done.Jun 24 2020, 2:53 AM
Szelethus added inline comments.Jun 24 2020, 5:46 AM
clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
530โ€“532

I fear this might be a stupid question, but what's up with 5 + it? Why does the LHS have to be the iterator? Am I missing something here?

541

What does Tgt mean?

baloghadamsoftware marked 2 inline comments as done.Jun 24 2020, 6:14 AM
baloghadamsoftware added inline comments.
clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
530โ€“532

There is nothing wrong with it, but it is not (yet) supported, because I simply forgot. However I think it is much less common than it + 5. This patch is just a fixit patch, so I try not to add new features here, just fix existing ones.

541

Tgt is the abbreviation of "target". The target of + and - is their results, but of += and -= is their left operands. However, I did not change this line in this patch.

gamesh411 accepted this revision.Jun 24 2020, 6:44 AM
gamesh411 added inline comments.
clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
530โ€“532

Good catch, we should definitely support that! I can even volunteer if you are ok with that.

This revision is now accepted and ready to land.Jun 24 2020, 6:44 AM
This revision was automatically updated to reflect the committed changes.