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

[Gábor Horváth]

================
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;
 
----------------
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?

================
Comment at: clang-tidy/ClangTidy.cpp:409
@@ -412,1 +408,3 @@
 
+void populateStaticAnalyzerCheckerList(StringRef Plugin, const char **Argv) {
+  IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts(new DiagnosticOptions());
----------------
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.

================
Comment at: clang-tidy/ClangTidy.cpp:421
@@ +420,3 @@
+    CommandLine.push_back("-Xclang");
+    CommandLine.push_back("-load");
+    CommandLine.push_back("-Xclang");
----------------
alexfh wrote:
> BTW, does this option rely on the plugin support turned on at build time? If yes, we need to make most of this code #ifdefed out using the same preprocessor macros.
Good point, I think yes, this plugin support requires that it is turned on compile time.

================
Comment at: clang-tidy/tool/ClangTidyMain.cpp:149
@@ -148,1 +148,3 @@
 
+static cl::opt<std::string> PluginPath(
+    "plugin-path",
----------------
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.

http://reviews.llvm.org/D9555

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