[PATCH] [clang-tidy] Support for Static Analyzer plugins

[Alexander Kornienko]

================
Comment at: clang-tidy/ClangTidy.cpp:61
@@ -57,10 +60,3 @@
 
-static StringRef StaticAnalyzerChecks[] = {
-#define GET_CHECKERS
-#define CHECKER(FULLNAME, CLASS, DESCFILE, HELPTEXT, GROUPINDEX, HIDDEN)       \
-  FULLNAME,
-#include "../../../lib/StaticAnalyzer/Checkers/Checkers.inc"
-#undef CHECKER
-#undef GET_CHECKERS
-};
+static std::vector<std::string> StaticAnalyzerChecks;
 
----------------
xazax.hun wrote:
> alexfh wrote:
> > It was fine to have this as a global variable while it was a compile-time constant. But it's a bad idea to use a global mutable variable to pass configuration to the library. Actually, I'd leave the compile-time checks list as a fallback, and only override it if clang-tidy is instructed to use a static analyzer plugin.
> It is a good idea to fall back to the checker list generated from header files.
> 
> What do you think, where should I put the mutable vector? In ClangTidyOptions?
See the comment below.

Also, please make this variable an array of StringRefs again.

================
Comment at: clang-tidy/ClangTidy.cpp:409
@@ -412,1 +408,3 @@
 
+void populateStaticAnalyzerCheckerList(StringRef Plugin, const char **Argv) {
+  IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts(new DiagnosticOptions());
----------------
xazax.hun wrote:
> alexfh wrote:
> > Is the only useful effect of this function to fill the StaticAnalyzerChecks variable? It's bad on its own as any other interface which only useful effect is side-effect.
> > 
> > AFAIU, you still rely on the compilation arguments to instruct the static analyzer that it needs to load a plugin? That doesn't sound like a good idea either for multiple reasons, most importantly that:
> >   1. This requires modification of the compilation arguments in the build system in order to run an analysis implemented in a plugin. Looks like an overkill and a totally wrong way of doing this.
> >   2. Compile arguments will affect what is loaded and executed by clang-tidy, which also seems wrong. If it is already the case, it was definitely unintended and we need to fix this instead of relying on this.
> >   3. 1 and 2 also mean that for correct usage clang-tidy's -plugin-path option will always need to be in sync with the -load options in the compilation database (read: in the build system). Extremely brittle and confusing.
> > 
> > 
> Yes, the only effect of this function is to initialize the checker list. What else would you prefer? Once the mutable list is the member of ClangTidyOptions, I could make this function a method.
> 
> With plugin-path argument, the command line of compilation do not need to be modified from the user's point of view. I only forgot to update the test case. Once the plugin path is given in the command line, this method should just work with any kind of unmodified compilation database.
> 
> The reason why it is not needed to add the -load argument to the command line:
> 
> This function loads the plugin which is kept in the memory for the whole runtime. Upon plugin load, the checkers are registered. So even though the load argument is not given to the frontend action that does the checking, the plugin already in the memory and the checkers already are registered, so they will be used.
> 
> If you find this method hacky or not future proof, I could create an argument adjuster object here which could be used during the next steps. However StaticAnalyzerChecks have to be initialized early enough so that the user can list the checkers that are loaded from the plugin.
> Yes, the only effect of this function is to initialize the checker list.

Well, this contradicts to the explanation of how plugins are loaded that you give below ;) But now I get it, and looks like the function should be called "loadPlugins" or similar in order to make the important side-effect obvious from the function name. And as we now rely on a mutable global state anyway, it may be fine to introduce (#ifdefed only when plugins are enabled) an additional global vector to store dynamically constructed static analyzer check list. At least I don't see a substantially better alternative.

Filtering out -load arguments from the arguments we get from a compilation database looks like a useful thing to do unless there's a better way to disable loading of plugins during the analysis.

================
Comment at: clang-tidy/tool/ClangTidyMain.cpp:149
@@ -148,1 +148,3 @@
 
+static cl::opt<std::string> PluginPath(
+    "plugin-path",
----------------
xazax.hun wrote:
> alexfh wrote:
> > Does static analyzer support only one plugin at a time?
> > 
> > Also, it _may_ be better to put this into ClangTidyOptions, but I'm not sure yet. Upsides are that then we'd avoid modification of most other interfaces. Obvious downsides are that this option only makes sense for the command-line front-end.
> > 
> > Can you explain your use case in more detail?
> I never tried to use multiple plugins, however you are right, the static analyzer supports to load multiple ones at the same time. I see your point to put this into ClangTidyOptions, so one can configure it using yaml files.
> 
> My usecase is the following:
> We have a script, that uses LD_PRELOAD to load a shared object that hijacks the exec function call family and filters and logs compiler calls. After this log file is created a script invokes clang tidy with slightly modified compilation arguments. This way we can support any build system on posix systems including incremental build support.We just got the permission to open source this tool set, so it is possible that we will try to upstream this tool soon. 
I'm still not sure about moving the "-plugin-path" option to `ClangTidyOptions`.

> My usecase is the following:
> We have a script, that uses LD_PRELOAD to load a shared object that 
> hijacks the exec function call family and filters and logs compiler calls. 

Similar to this one? https://github.com/rizsotto/Bear

> After this log file is created a script invokes clang tidy with slightly 
> modified compilation arguments. This way we can support any build 
> system on posix systems including incremental build support.

You mean you run analyses on each build? Sounds interesting, though there's a (possibly more effective) alternative: to run analyses as a part of submitting the code for review.

But anyway, that doesn't answer why do you need plugins and how you use them.

http://reviews.llvm.org/D9555

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/