This is an archive of the discontinued LLVM Phabricator instance.

[Analyzer] Iterator Modeling - Model `std::advance()`, `std::prev()` and `std::next()`
ClosedPublic

Authored by baloghadamsoftware on Mar 18 2020, 6:45 AM.

Details

Summary

Whenever the analyzer budget runs out just at the point where std::advance(), std::prev() or std::next() is invoked the function are not inlined. This results in strange behavior such as std::prev(v.end()) equals v.end(). To prevent this model these functions if they were not inlined. It may also happend that although std::advance() is inlined but a function it calls inside (e.g. __advance() in some implementations) is not. This case is also handled in this patch.

Diff Detail

Event Timeline

Whenever the analyzer budget runs out just at the point where std::advance(), std::prev() or std::next() is invoked the function are not inlined. This results in strange behavior such as std::prev(v.end()) equals v.end(). To prevent this model these functions if they were not inlined. It may also happend that although std::advance() is inlined but a function it calls inside (e.g. __advance() in some implementations) is not. This case is also handled in this patch.

I suppose we could get this strange behavior with std::distance too? Would it be worth to handle that as well here?

martong added inline comments.Mar 18 2020, 11:22 AM
clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
193

Perhaps putting this hunk into a separate function or lambda could decrease the nesting level, b/c you could have an early return if there is no Handler.

194

Maybe a comment could be helpful here about the common signature of the prev/next functions. Like:

advanceFun ( It it, Distance n = 1 )

By the way, can we call std::advance with one argument?

210

Should this be __advance?

584

Some comments would be helpful here. Also, should we use this function only with advance() or it could be useful perhaps in other context?

596

I have a rough presumption that this hunk is a general pattern to get the previous node for the previous iterator position. So perhaps it would be useful as a standalone free/static member function too?

Updated according to the comments and automatic formatting suggestions.

baloghadamsoftware marked 9 inline comments as done.Mar 20 2020, 1:31 AM

I suppose we could get this strange behavior with std::distance too? Would it be worth to handle that as well here?

Modeling of std::distance() is planned after we model the size of the container properly.

clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
193

Early return is not possible because the execution must continue if the is no handler. However, I refactored checkPostCall now, the handling of both overloaded operators and advance-like functions are moved to separate functions.

194

I added them to the map.

210

No. __advance() is non-standard, I cannot rely on it. If the function was std::advance(), it was inlined, but still did not change the iterator's position means that it did not do its job. Probably the function it calls inside (which could be __advance() in most implementations) was not inlined. In this case we model the behavior of std::advance() explicitely.

584

I renamed the function and added a short comment. Is it OK?

596

I moved this to a standalone function and named it accordingly.

martong accepted this revision.Mar 20 2020, 9:43 AM

LGTM!

clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
193

Thanks, that's much better structured this way.

584

Yeah, looks ok.

596

Thanks, it's easier to read the code this way.

This revision is now accepted and ready to land.Mar 20 2020, 9:43 AM
This revision was automatically updated to reflect the committed changes.
baloghadamsoftware marked 4 inline comments as done.