[analyzer] Checker: check::BeginAnalysis

[Charusso]

Summary

This callback is called when the analysis starts. It could be used to
construct the checker's fields which are related to the Preprocessor.

void CoolFunctionChecker::checkBeginAnalysis(CheckerContext &C) const {
  Preprocessor &PP = C.getPreprocessor();
  UseMyCoolLib = PP.isMacroDefined("__MY_COOL_LIB__");
}

[Charusso]

I felt that it will be easier, eh.

clang/lib/StaticAnalyzer/Core/CoreEngine.cpp

118

I think we need to merge the BeginFunction into the BeginWorklist.

[Szelethus]

YES PLEASE. Debug checkers that only dump from check::EndAnalysis won't rely on the analysis not actually crashing. Which is ironically exactly when we want to use them.

clang/lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp

199

Hmmm, what use case do you have in mind? Do we actually want this to happen? By the time checkers are constructed, the AST is already built, alongside all the other data structures that will be inspected by the analyzer. Since checkers are constructed once for each clang invocation (not for every BeginAnalysis -- EndAnalysis cycle), I would imagine that the checker registry functions are more appropriate for such initializations.

I'm only aware of one non-debug checker that uses EndAnalysis (maybe RetailCount?), that clears some sort of a locally stored map that it shouldn't really have anyways (since checkers are supposed to be stateless), and I struggle to think of a realistic, supported use case for this callback other than debug checkers.

199

Turns out deadcode.UnreachableCode uses EndAnalysis quite cleverly as well.

[Charusso]

• Do not restrict what you supposed to do with the callback.

[Charusso]

In D69745#1732348, @Szelethus wrote:

YES PLEASE. Debug checkers that only dump from check::EndAnalysis won't rely on the analysis not actually crashing. Which is ironically exactly when we want to use them.

Thanks for the idea! I am not that cool with debug checkers and I think you are the first user who made a creative decision how to use this. My use case is the Preprocessor only if I do not touch the checker-registry based construction.

clang/lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp

199

Yes, good point. It is made for the Preprocessor mainly which is very path-sensitive, but that is not that good idea to construct e.g. the BugType multiple times. So that, I do not want to restrict creativity and I have removed that line.

[NoQ]

I think we need to merge the BeginFunction into the BeginWorklist.

Basically this, but the other way round: your BeginAnalysis is BeginFunction with extra steps (namely, checking C.inTopFrame()).

Backstory: I wanted to add BeginAnalysis a few years ago because i wanted to mark argc and argv as tainted when analyzing main(). But then i noticed that we already have BeginFunction so i decided not to add it. No, i didn't mark them as tainted yet :/

YES PLEASE. Debug checkers that only dump from check::EndAnalysis won't rely on the analysis not actually crashing.

There are currently two such checkers: debug.ViewExplodedGraph and debug.Stats. It doesn't make sense to move them to BeginAnalysis though, because the data that they're dumping isn't available yet. If the checker dumps data that isn't collected during path sensitive analysis, it should subscribe to ASTCodeBody instead. This way activating the checker won't trigger path-sensitive analysis to occur.

[Charusso]

In D69745#1732739, @NoQ wrote:

Basically this, but the other way round: your BeginAnalysis is BeginFunction with extra steps (namely, checking C.inTopFrame()).

Backstory: I wanted to add BeginAnalysis a few years ago because i wanted to mark argc and argv as tainted when analyzing main(). But then i noticed that we already have BeginFunction so i decided not to add it. No, i didn't mark them as tainted yet :/

Hm, that is a cool workaround. I think every UnknownSpaceRegion stuff is tainted, and that is a strong mark here.

[NoQ]

In D69745#1732744, @Charusso wrote:

I think every UnknownSpaceRegion stuff is tainted, and that is a strong mark here.

Mmm, no, that's definitely not true. Basically, if you analyze a method of a class as your top frame, you have your whole class reside in UnknownSpaceRegion, being represented as SymRegion{reg_$0<this>}. It doesn't mean that there are no contracts that constrain possible values of fields within the class.