The solution is implemented as follows:
- Add a new field to the Checker class in CheckerBase.td called Dependencies, which is a list of Checkers. I know some of us have mixed feelings on the use of tblgen, but I personally found it very helpful, because few things are more miserable then working with the preprocessor, and this extra complication is actually another point where things can be verified, properly organized, and so on. The best way for me to prove that it shouldn't be dropped is actually use it and document it properly, so I did just that in this patch.
- Move unix checkers before cplusplus, as there is no forward declaration in tblgen :/
- Add the following new checkers:
- StackAddrEscapeBase
- StackAddrEscapeBase
- CStringModeling
- DynamicMemoryModeling (base of the MallocChecker family)
- IteratorModeling (base of the IteratorChecker family)
- ValistBase
- SecuritySyntaxChecker (base of bcmp, bcopy, etc...)
- NSOrCFErrorDerefChecker (base of NSErrorChecker and CFErrorChecker)
- IvarInvalidationModeling (base of IvarInvalidation checker family)
- RetainCountBase (base of RetainCount and OSObjectRetainCount)
- Clear up and registry functions in MallocChecker, happily remove old FIXMEs.
- Add a new addDependency function to CheckerRegistry.
- Neatly format RUN lines in files I looked at while debugging.
[1](Copied from D54397's summary!) While on the quest of fixing checker options the same way I cleaned up non-checker options, although I'm making good progress, I ran into a wall: In order to emit warnings on incorrect checker options, we need to ensure that checkers actually acquire their options properly -- but, I unearthed a huge bug in checker registration: dependencies are currently implemented in a way that breaks the already very fragile registration infrastructure.
Here is where the problem really originates from: this is a snippet from CheckerRegistry::initializeManager.
// Initialize the CheckerManager with all enabled checkers. for (const auto *i :enabledCheckers) { checkerMgr.setCurrentCheckName(CheckName(i->FullName)); i->Initialize(checkerMgr); }
Note that Initialize is a function pointer to register##CHECKERNAME, like registerInnerPointerChecker. Since for each register function call the current checker name is only set once, when InnerPointerChecker attempts to also register it's dependent MallocChecker, both it and MallocChecker will receive the cplusplus.InnerPointer name. This results in MallocChecker trying to acquire it's Optimistic option as cplusplus.InnerPointerChecker:Optimistic.
Clearly, this problem has to be solved at a higher level -- it makes no sense to be able to affect other checkers in a registry function. Since I'll already make changes to how checker registration works, I'll probably tune some things for the current C++ standard, add much needed documentation, and try to make the code a lot less confusing.
Any reason this checker shouldn't get a dependecy too?
If I run it with
clang -cc1 -analyze -analyzer-checker=core -analyzer-checker=nullability.NullReturnedFromNonnull empty.c
on an empty file empty.c I get
clang: ../tools/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h:171: CHECKER *clang::ento::CheckerManager::getChecker() [CHECKER = (anonymous namespace)::NullabilityChecker]: Assertion `CheckerTags.count(tag) != 0 && "Requested checker is not registered! Maybe you should add it as a " "dependency in Checkers.td?"' failed.
If I add
Dependencies<[NullabilityBase]>,
to it, then it doesn't trigger the assert.
I don't know anything about this code, what do you think about it?