This is an archive of the discontinued LLVM Phabricator instance.

[Analyzer] Use note tags to track iterator increments and decrements
Needs ReviewPublic

Authored by baloghadamsoftware on Feb 13 2020, 3:45 AM.

Details

Reviewers
NoQ
Szelethus
Summary

Note tags marking the points where an iterator is incremented or decremented helps finding the root cause of iterator errors, especially out-of-range dereferences and out-of-range increments and decrements. This patch adds such tags.

Diff Detail

Event Timeline

Minor update: ignore parentheses and casts when taking the name of the expression.

Split into two patches. This is the first one.

A few nits, otherwise the patch is great!

clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
707–708

Are It1 and It2 used? Why do we default the latter to UndefinedVal?

clang/test/Analysis/iterator-modelling.cpp
1–5 ↗(On Diff #248447)

Can we prettify this? :)

clang/test/Analysis/iterator-range.cpp
0–12

And this?

Szelethus added inline comments.Mar 11 2020, 10:40 AM
clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
707–708

Ah, okay, we use it in a followup patch. Still, we shouldn't use UndefinedVal, as discussed in D75514.

Updated according to the comments.

baloghadamsoftware marked 4 inline comments as done.Mar 12 2020, 1:03 PM
baloghadamsoftware added inline comments.
clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
707–708

I completely removed them from this patch.

NoQ added inline comments.Mar 15 2020, 8:16 PM
clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
661–662

Can we assert out the ChangeVal == 0 case or make a better message for it ("unchanged" or something like that)?

NoQ added inline comments.Mar 15 2020, 8:33 PM
clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
661–662

Another important thing to do here is to track the RHS value via trackExpressionValue(). I.e., why do we think that iterator is incremented by 1 and not by 2?

baloghadamsoftware marked an inline comment as done.Mar 17 2020, 7:36 AM
baloghadamsoftware added inline comments.
clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
661–662

I agree, that is important, but where should I call it? This is the modeling checker, which models the increments and the decrements, but trackExpressionValue() can only be invoked for bug reports. Should I put it into the NoteTag lambda?

NoQ added inline comments.Mar 23 2020, 7:22 PM
clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
661–662

Yup, it should be possible to invoke trackExpressionValue(), and generally attach more visitors, inside a NoteTag.

Rebased and added call for trackExpressionValue().

baloghadamsoftware marked 4 inline comments as done.Mar 25 2020, 10:34 AM
baloghadamsoftware added inline comments.
clang/test/Analysis/iterator-modelling.cpp
434 ↗(On Diff #252615)

Strange, that we do not get this note for prev.

NoQ added inline comments.Mar 30 2020, 3:38 PM
clang/test/Analysis/iterator-modelling.cpp
434 ↗(On Diff #252615)

Mmm, why 0-1?

baloghadamsoftware marked an inline comment as done.
baloghadamsoftware added inline comments.
clang/test/Analysis/iterator-modelling.cpp
434 ↗(On Diff #252615)

Because it depends whether next() is inlined.