This is an archive of the discontinued LLVM Phabricator instance.

[Analyzer] Handle pointer implemented as iterators in iterator checkers
ClosedPublic

Authored by baloghadamsoftware on Jun 19 2020, 7:03 AM.

Details

Summary

Iterators are an abstraction of pointers and in some data structures iterators may be implemented by pointers. This patch adds support for iterators implemented as pointers in all the iterator checkers (including iterator modeling).

Diff Detail

Event Timeline

baloghadamsoftware marked an inline comment as done.Jun 19 2020, 7:12 AM
baloghadamsoftware added inline comments.
clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
256

This is the problematic point which is not working. I left the comments intentionally in the code.

The problem is that in postStmt we are after the operation. Thus the value of the operand (SubExpr) is not i anymore, but the former value of i (a pointer to a symbolic region initially). Instead, the result is i in case of prefix operators, but also the former value in case of postfix operators. This is correct, of course, because here, after the call the value of i was changed, thus it is not equal to the parameter. However, we need the region of i here and/or the new value bound to it (e.g. the pointer to an element region which is usually the result of a ++ or -- on a pointer to a symbolic region). How to reach that? Of course, in preStmt the operand is i as it should be. The same is true for binary operators += and -=.

baloghadamsoftware marked an inline comment as done.Jun 19 2020, 9:57 AM
baloghadamsoftware added inline comments.
clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
256

Of course, I know it is all wrong to increment the former value. This is also not the goal, probably I cannot reuse handleIncrement() and the like here. The question is how to get the region of the variable or even better the new region (e.g. the pointer to element region) bound to it? In the prefix case I have the variable, but is there a generic way to get the region bound to it? By generic I mean that I hope that I do not have to branch on all possible lvalue expressions.

baloghadamsoftware marked an inline comment as done.Jun 21 2020, 10:34 AM
baloghadamsoftware added inline comments.
clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
256

Wait! I know the solution! I will try it tomorrow.

baloghadamsoftware retitled this revision from [Analyzer][WIP] Handle pointer implemented as iterators in iterator checkers to [Analyzer] Handle pointer implemented as iterators in iterator checkers.
baloghadamsoftware edited the summary of this revision. (Show Details)

Working.

gamesh411 added inline comments.Jun 24 2020, 1:02 AM
clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
623

Same as in the other patch. I think variable name TempState is too generic, should be AdvancedState or something more descriptive.

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

Just being annoying here, but I saw you use // expected (with a space between the slash and expected) consistently elsewhere. Could you please also use that here in these tests as well? Being uniform with those helps spot other errors and typos IMHO.

Updated according to the comments.

baloghadamsoftware marked 2 inline comments as done.Jun 24 2020, 4:21 AM
gamesh411 accepted this revision.Jun 30 2020, 9:02 AM

Thanks, it LGTM now!

This revision is now accepted and ready to land.Jun 30 2020, 9:02 AM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Jul 1 2020, 3:15 AM

This (or a follow-up) broke tests on windows: http://45.33.8.238/win/18877/step_7.txt

Please take a look and revert for now if it takes a while to fix.

This (or a follow-up) broke tests on windows: http://45.33.8.238/win/18877/step_7.txt

Please take a look and revert for now if it takes a while to fix.

It was this one: D82385. I reverted that test function so one of the fixes now remains untested.