[cfe-dev] Checker Plugins

[Ted Kremenek]

Hi Jordy,

I'm still forming some thoughts on this.  My initial impression while I think this is a good direction, it really does feel overly complicated, particularly with the use of clang_getCheckersForPackage_PACKAGE functions.

I'm most mixed about the CheckerPackage abstraction.  How would it actually be used?  It seems to me that:

(a) First a plugin must be loaded, e.g. using llvm::sys::DynamicLibrary.

(b) Then the CheckerPackage::create() function is called to register checkers, but this seems to require a priori knowledge of the package name.

In your code snippet below, it's not clear to me where (b) lives.  Does it live in the plugin?  If so, this seems like a lot of indirection, when we could possibly instead use the plugin registry mechanism instead to find the CheckerManager and then register the checkers.  What's the benefit of the clang_getCheckersForPackage abstraction?

The mechanism I had more in mind was that a user writes a plugin, which links against clang and the static analyzer libraries.  It uses the llvm::Registry mechanism to load checkers using static constructors when the plugin is loaded.  This would be very simple, akin to what we can do with optimization passes:

http://llvm.org/docs/WritingAnLLVMPass.html#registering

In that example, we have RegisterRegAlloc, but instead we could have something like "RegisterCheckers".  It could take as an argument a function that returns a factory object (perhaps CheckerPackage?) that allows one to instantiate checkers given a CheckerManager.  The key thing to keep in mind is that his simple approach is very flexible.  One could possibly register several checkers into different packages.  We can also probably decouple checker registration from package registration.  Packages are just groups; when a checker gets loaded, it could just declare that it goes into a specific package.  If that group doesn't exist, the CheckerManager just creates one with the default visibility.  If the package gets explicitly registered (using a similar mechanism, e.g. "RegisterCheckerPackage"), that registration could provide enabled/disabled information for that package, visibility, and strings to describe that package.

In this mode, we get a nice decoupling.  Checker writers only have to declare the package the checker goes into by providing a package string.  They can also provide strings that describe the checker, which is then used by the registration mechanism.  Packages can be managed and registered separately.

This simpler, declarative approach also resolves the -print-help problem.  Instead of -print-help, I would expect we move in a direction where checkers just provide descriptive strings, and the help is formatted by whoever cares about it.  Checkers, and packages, should not get in the business of printing out anything.  That should be up to the client of the CheckerManager.

Anyhow, I'm stilling mulling this over, but I think we want to keep this simple and flexible.

What do you think?

On Jul 27, 2011, at 1:48 PM, Jordy Rose wrote:

> Okay, I've come up with a naming-based version of this, probably not final but working fairly well. The plugin client's job is fairly simple:
> 
> using clang::ento::CheckerInfo;
> extern "C" clang_getCheckersForPackage_PACKAGE (CheckerInfo::List &out) {
>  // First include checkers from subpackages.
>  clang_getCheckersForPackage_SUBPACKAGE1(out);
>  clang_getCheckersForPackage_SUBPACKAGE2(out);
>  CheckerInfo::markAllSubpackage(out);
> 
>  // Then add checkers from this package.
>  out.push_back(CheckerInfo::create<MyChecker>("MyChecker",
>    "short description"));
>  // The name of the checker class does not have to match the name the
>  // client uses to enable your checker.
>  out.push_back(CheckerInfo::create<MyOtherChecker>("OtherChecker",
>    "short description"));
>  // "Hidden" checkers are only enabled if they or their package is
>  // explicitly requested, not if they are in a subpackage.
>  out.push_back(CheckerInfo::create<HiddenChecker>("HiddenChecker",
>    "short description", true));
> }
> 
> The use of a clang_getCheckersForPackage_PACKAGE function means that we could eventually migrate all internal checkers to this format as well.
> 
> The main thing that this doesn't handle is that -analyzer-checker-help doesn't know what packages a plugin might have, so it just uses the basename of the plugin you're loading. The next step would be to look for a clang_checkerPackages_PLUGIN array that contains the names of any packages in PLUGIN.dylib. (Or maybe libPLUGIN.dylib.)
> 
> Where should I be going with this?
> Jordy
> 
> <CheckerPlugins.3.patch>