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

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

Szelethus created this revision.
Szelethus added reviewers: george.karpenkov, NoQ, xazax.hun, rnkovacs.
Herald added subscribers: cfe-commits, dkrupp, donat.nagy, jfb, mikhail.ramalho, a.sidorin, mgrang, szepet, whisperity, mgorny.

This patch solves the problem I explained in an earlier revision[1]. It's huge, I'm still looking for ways to split it up, hence the WIP tag. Also, it fixes a major bug, but has no tests.

The solution is implemented as follows:

- Add a new field to the `Checker` class in `CheckerBase.td` called `Dependencies`, which is a list of `Checker`s. 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 :/
- Introduce two new checkers: `CStringBase` and `MallocBase` are essentially checkers without any of their modules/subcheckers enabled. These are important, as for example `InnerPointerChecker` would like to see `MallocChecker` enabled, but does not add `CK_MallocChecker` to `MallocChecker::ChecksEnabled` on it's own. This was weird, so from now on, "barebone" versions of `CStringChecker` and `MallocChecker` may be enabled via the new checkers. Note that this doesn't change anything on the user facing interface.
- Move `InnerPointerChecker` to it's own directory, separate the checker class into a header file, move its registry function to `MallocChecker`.
- Clear up and registry functions in `MallocChecker`, happily remove old FIXMEs.
- Remove `lib/StaticAnalyzer/Checkers/InterCheckerAPI.h`.
- Add a new `addDependency` function to `CheckerRegistry`.
- Neatly format `RUN` lines in files I looked at while debugging.

[1]**(Copied from https://reviews.llvm.org/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//.


Repository:
  rC Clang

https://reviews.llvm.org/D54438

Files:
  include/clang/StaticAnalyzer/Checkers/CheckerBase.td
  include/clang/StaticAnalyzer/Checkers/Checkers.td
  include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h
  lib/StaticAnalyzer/Checkers/CMakeLists.txt
  lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  lib/StaticAnalyzer/Checkers/InnerPointer/InnerPointerChecker.cpp
  lib/StaticAnalyzer/Checkers/InnerPointer/InnerPointerChecker.h
  lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
  lib/StaticAnalyzer/Checkers/InterCheckerAPI.h
  lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
  test/Analysis/NewDelete+MismatchedDeallocator_intersections.cpp
  test/Analysis/NewDelete-checker-test.cpp
  test/Analysis/inner-pointer.cpp
  utils/TableGen/ClangSACheckersEmitter.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D54438.173725.patch
Type: text/x-patch
Size: 38991 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20181112/f97ec0d1/attachment-0001.bin>