This is an archive of the discontinued LLVM Phabricator instance.

[Analyzer] [NFC] Iterator Checkers - Separate iterator modeling and the actual checkers
ClosedPublic

Authored by baloghadamsoftware on Nov 15 2019, 9:02 AM.

Details

Summary

A monolithic checker class is hard to maintain. This patch splits it up into a modelling part, the three checkers and a debug checker. The common functions are moved into a library.

Diff Detail

Event Timeline

whisperity retitled this revision from [Analyzer] Iterator Checkers - Separate iterator modelling and the actual checkers to [Analyzer] [NFC] Iterator Checkers - Separate iterator modelling and the actual checkers.Nov 18 2019, 2:18 AM
whisperity edited the summary of this revision. (Show Details)
baloghadamsoftware retitled this revision from [Analyzer] [NFC] Iterator Checkers - Separate iterator modelling and the actual checkers to [Analyzer] [NFC] Iterator Checkers - Separate iterator modeling and the actual checkers.Nov 18 2019, 10:22 PM
NoQ added a comment.Nov 19 2019, 2:36 PM

I love how this checker is pioneering in so many ways.

clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
1307–1317

Maybe move this function to Iterator.cpp as well, and then move the definitions for iterator maps from Iterator.h to Iterator.cpp, which will allow you to use the usual REGISTER_MAP_WITH_PROGRAMSTATE macros, and additionally guarantee that all access to the maps goes through the accessor methods that you provide?

baloghadamsoftware marked an inline comment as done.Nov 19 2019, 11:56 PM
baloghadamsoftware added inline comments.
clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
1307–1317

Hmm, I was trying hard to use these macros but failed so I reverted to the manual solution. I will retry now.

Charusso added inline comments.Nov 20 2019, 12:22 AM
clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
1307–1317

Here is a how-to: https://reviews.llvm.org/D69726

You need to add the fully qualified names to the register macro because of the global scope. I hope it helps.

baloghadamsoftware marked an inline comment as done and an inline comment as not done.Nov 20 2019, 5:39 AM
baloghadamsoftware added inline comments.
clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
1307–1317

OK, I checked it now. If we want to put the maps into Iterator.cpp then we also have to move a couple of functions there which are only used by the modeling part: the internals of checkDeadSymbols() and checkLiveSymbols() must go there, although no other checker should use them. Also processIteratorPositions() iterates over these maps, thus it must also go there. Should I do it? Generally I like the current solution better, only functions used by multiple checker classes are in the library.

On the other hand I do not see why I should move assumeNoOverflow() to Iterator.cpp? This function is only used by the modeling part, and does not refer to the maps. You mean that we should ensure this constraint whenever setting the position for any iterator? This would mean that we should decompose every symblic expression and (re)assume this range on the symbolic part. Or we should replace setPosition() by at least two different functions (e.g. setAdvancedPosition() and setConjuredPosition()) but this means a total reqriting of the modeling.

I plan some refactoring but this first patch is meant to be a single separation of code.

baloghadamsoftware marked 2 inline comments as done.Nov 28 2019, 7:57 AM
baloghadamsoftware added inline comments.
clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
1307–1317

@Charusso It does not help because the macros put the maps into a local namespace. If it is in a header, it means separate maps for every checker and the modeling which of course does not work.

1307–1317

@NoQ What is the decision? Should I move everything to the lib such as the body of checkDeadSymbols() and checkLiveSymbols() which I am reluctant because they belong only to the modeling, or leave it as it is now?

NoQ accepted this revision.Dec 10 2019, 4:25 PM

I plan some refactoring but this first patch is meant to be a single separation of code.

Sounds great!

clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
1307–1317

I think all the functions for manipulating the state trait (including dead symbol cleanup and escapes) should live in the modeling file. Like, it doesn't mean that checkDeadSymbols itself should necessarily live there, but in any case the file should provide a nice cleanup method that can be invoked from the real checkDeadSymbols.

This way you can encapsulate all the implementation details in the cpp file and only interact with it with fancy accessors.

I'm perfectly ok with delaying this work ^.^

This revision is now accepted and ready to land.Dec 10 2019, 4:25 PM
This revision was automatically updated to reflect the committed changes.