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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In https://reviews.llvm.org/D54438#1296832, @Szelethus wrote:

> - Some checkers do not register themselves in all cases[1], which should probably be handled at a higher level, preferably in `Checkers.td`. Warnings could be emitted when a checker is explicitly enabled but will not be registered. [1]




In https://reviews.llvm.org/D54438#1296832, @Szelethus wrote:

>   void ento::registerObjCDeallocChecker(CheckerManager &Mgr) {
>     const LangOptions &LangOpts = Mgr.getLangOpts();
>     // These checker only makes sense under MRR.
>     if (LangOpts.getGC() == LangOptions::GCOnly || LangOpts.ObjCAutoRefCount)
>       return;
>  
>     Mgr.registerChecker<ObjCDeallocChecker>();
>   }
>


The can of worms here is as follows.

It is clear that not all checkers are applicable to all languages or all combinations of compiler flags. In this case, significantly different semantics are observed under different Objective-C memory management policies and the checker isn't designed to work correctly for anything but the "manual" memory management policy. The way the code is now written, it is impossible to register the checker for translation units that are compiled with incompatible flags.

This should not cause a warnings because it should be fine to compile different translation units with different flags within the same project, while the decision to enable the checker explicitly is done once per project.

Now, normally these decisions are handled by the Clang Driver - the gcc compatibility wrapper for clang that converts usual gcc-compatible run-lines into `-cc1` run-lines. Eg., the `unix` package is only enabled on UNIX targets and the `osx` package is only enabled on macOS/iOS targets and the `core` package is enabled on all targets and `security.insecureAPI` is disabled on PS4 //because that's what the Driver automatically appends to the -cc1 run-line//. Even though `scan-build` does not call the analyzer through the Driver, it invokes a `-###` run-line to obtain flags first, so it still indirectly consults the Driver.

But the problem with the Driver is that nobody knows about this layer of decision-making (unlike the obvious `Checkers.td`) and therefore it's kinda messy to have all checkers hardcoded separately within the Driver. It kinda makes more sense to split the checkers into packages and let the Driver choose which packages are enabled.

But the problem with packages is that they are very hard to change because they are the UI that external GUIs rely upon. And right now we don't yet have separate packages for Objective-C MRR/ARC/GC modes (btw, GC needs to be removed).

And even if we were to go in that direction, the amount of packages would explode exponentially with the number of different independent flags we need to track. If only we had hashtags, that would have been a great direction to go.

So, generally, i suggest not to jump onto this as long as the analyzer as a whole works more or less correctly. Restricting functionality for the purpose of avoiding mistakes should be done with extreme care because, well, it restricts functionality. Before restricting funcitonality, we need to be more or less certain that nobody will ever need such functionality or that we will be able to provide a safe replacement when necessary without also introducing too much complexity (aka bugs). Which is why i recommend not to bother too much about it. There are much more terrible bugs all over the place, no need to struggle that hard to prevent these small bugs.

Another approach to the original problem would be to replace the language options check in registerBlahBlahChecker() with a set of language options checks in every checker callback. It's annoying to write and clutters the checker but it preserves all the functionality without messing with the registration process. So if you simply want to move these checks out of the way of your work, this is probably the way to go.


Repository:
  rC Clang

https://reviews.llvm.org/D54438