[PATCH] D54438: [analyzer][WIP] Reimplement dependencies between checkers

[Umann Kristóf via Phabricator via cfe-commits]

Szelethus added a comment.

Okay. I was wrong, and you were right.

`MallocChecker` seriously suffers from overcrowding. Especially with the addition of `InnerPointerChecker`, it should be split up, but doing that is very much avoidable for the purposes of this effort while still achieving every goal of it (which I'll detail in the next couple points). But, I spent so much time with it, it kinda grew on me, so I'll probably tackle the problem on my downtime.

The change of course as follows, checkmarks indicate that I already have a working solution locally: (largely copied from D54438#1296695 <https://reviews.llvm.org/D54438#1296695>)

- ✔️ Introduce the boolean `ento::shouldRegister##CHECKERNAME(const LangOptions &LO)` function very similarly to `ento::register##CHECKERNAME`. This will force every checker to implement this function, but maybe it isn't that bad: I saw a lot of ObjC or C++ specific checkers that should probably not register themselves based on some `LangOptions` (mine too), but they do anyways. Add an assert when `getChecker` is called on a checker that isn't already registered.
  - ❌ What I didn't do just yet but might be a good idea, is to rename `register##CHECKERNAME` to `enable##CHECKERNAME`, and let's reserve the term "registering" to `CheckerRegistry`.
  - This patch will //not// feature any functional change, despite the observation I just made.

- ✔️ There are in fact a variety of checkers that contain subcheckers like `MallocChecker`, which I think is all good, but the concept that if a subchecker is enabled it only sort of registeres the main checker (in `MallocChecker`'s case it enables modeling, but not reporting) is very hard to follow. I propose that all such checkers should clearly have a base, called something `DynamicMemoryModeling`, or `IteratorModeling` etc.

- ✔️ `UnixAPI` contains a dual checker, but they actually don't interact at all, split them up.

- ✖️ Introduce dependencies, all checkers register themselves and only themselves. On my local branch I also like that it's super obvious which checker is interacting with which one, for example, `IteratorChecker` also has multiple parts, but with this patch, it's super obvious from a glance at `Checkers.td`.
  - ✔️ We can actually ensure that dependencies are registered before the checkers that depend on them, thanks to asserts added in the first part.
  - ❌ Add an assert when `CheckerManager::registerChecker` is called on an already registered checker.

- ❌ Move `CheckerManager::registerChecker<T>` out of the registry functions.
  - ❌ Since `shouldRegister##CHECKERNAME` is a thing, we can move everything to the checker's constructor, supply a `CheckerManager`, eliminating the function entirely.
  - ❌ At long last, get rid of `CheckerManager::setCurrentCheckerName` and `CheckerManager::getCurrentCheckerName`.
    - ❌ It was discussed multiple times (D48285#inline-423172 <https://reviews.llvm.org/D48285#inline-423172>, D49438#inline-433993 <https://reviews.llvm.org/D49438#inline-433993>), that acquiring the options for the checker isn't too easy, as it's name will be assigned only later on, so currently all checkers initialize their options past construction. This can be solved either by supplying the checker's name to every constructor, or simply storing all enabled checkers in `AnalyzerOptions`, and acquire it from there. I'll see.

- ✖️ Properly document why `CheckerRegistry` and `CheckerManager` are separate entities, how are the tblgen files processed, how are dependencies handled, and so on.

- ✖️ Rebase my local checker option-related branches, and celebrate. I wouldn't like to go in any more detail, who knows what lies ahead.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54438/new/

https://reviews.llvm.org/D54438