[cfe-dev] [RFC PATCH] making the static analyzer a PluginASTAction

[Jordy Rose]

Happy that someone's working on this! Unfortunately I don't have too many cycles right now either, but here are some small comments based on a quick read-through of your patch.

My larger issue is how much people depend on the current argument set for clang -cc1. Anyone using -Xanalyzer like they "should" will be fine, but those who directly call -cc1 (or mistakenly use -Xclang) will see everything break. (Now I see why these have been kept separate!) For maximum compatibility we'd keep all the old options as aliases, but that seems silly if this is really the direction we're going. It might be nice to still accept -analyzer, though, even if all we do is emit a warning that says '-analyze has been disabled, use -plugin analyzer instead'.

Plugin arguments are really ugly right now. Maybe someday we can have some kind of argument grouping system, i.e. "all options in this section are intended for the 'foo' plugin". But that's not your problem. :-)

I can't comment on some of the larger design choices right now, like exactly /what/ the separation of duties should be between the driver and the plugins, and where OptTable and argument parsing should live. But I'm glad you're pushing this; it's a step towards being able to distribute the analyzer and the compiler separately, which could someday turn out to be useful. (...apart from shrinking download sizes, of course.)

Thank you!
Jordy


--- a/lib/Driver/Tools.cpp
+++ b/lib/Driver/Tools.cpp
@@ -1213,6 +1213,12 @@
 static bool shouldUseFramePointer(const ArgList &Args,
   return true;
 }
 
+#define addPluginArg(CmdArgs, plugin, argument) \
+do {\
+  CmdArgs.push_back("-plugin-arg-" plugin);\
+  CmdArgs.push_back(argument);\
+} while (0)
+

Minor thing: can we give this a more macro-y name? I see why it has to be a macro (for the string concatenation), but it seems like it's asking for trouble to give it a function name.


--- /dev/null
+++ b/lib/StaticAnalyzer/Frontend/AnalyzerOptions.cpp
+namespace {
+
+class AnalyzerOptTable : public driver::OptTable {
+public:
+  AnalyzerOptTable()
+    : OptTable(AnalyzerInfoTable,
+               sizeof(AnalyzerInfoTable) / sizeof(AnalyzerInfoTable[0])) {}
+};
+
+}
+
+driver::OptTable *clang::createAnalyzerOptTable() {
+  return new AnalyzerOptTable();
+}

Uh, why not just return "new driver::OptTable(...)"? Maybe someday we'll need a special class for this, but not now. (Not sure why the other option tables in Driver do this too.)


--- a/lib/Frontend/CompilerInvocation.cpp
+++ b/lib/Frontend/CompilerInvocation.cpp
@@ -912,7 +819,6 @@
 }
 
 void CompilerInvocation::toArgs(std::vector<std::string> &Res) {
-  AnalyzerOptsToArgs(getAnalyzerOpts(), Res);
   CodeGenOptsToArgs(getCodeGenOpts(), Res);
   DependencyOutputOptsToArgs(getDependencyOutputOpts(), Res);
   DiagnosticOptsToArgs(getDiagnosticOpts(), Res);


You've moved this functionality to the plugin, but you haven't actually called it again from here. This is saved a bit by the fact that I think no one actually /uses/ CompilerInvocation::toArgs to record analyzer invocations, but if we're being proper CompilerInvocation should (eventually?) be able to toArgs and CreateFromArgs /any/ plugin's options, right?


--- a/lib/StaticAnalyzer/Frontend/CMakeLists.txt
+++ b/lib/StaticAnalyzer/Frontend/CMakeLists.txt
@@ -13,6 +14,7 @@
 add_dependencies(clangStaticAnalyzerFrontend
   clangStaticAnalyzerCheckers
   clangStaticAnalyzerCore
+  ClangStaticAnalyzerOptions
   ClangAttrClasses
   ClangAttrList
   ClangDeclNodes

What is this? I don't see a "ClangStaticAnalyzerOptions" library anywhere.


--- a/lib/Frontend/InitPreprocessor.cpp
+++ b/lib/Frontend/InitPreprocessor.cpp
@@ -530,7 +530,9 @@
     Builder.defineMacro("__weak", "__attribute__((objc_gc(weak)))");
 
   // Define a macro that exists only when using the static analyzer.
-  if (FEOpts.ProgramAction == frontend::RunAnalysis)
+  // XXX: This is a total hack, this should be added from the plugin.
+  if (FEOpts.ProgramAction == frontend::PluginAction
+      && FEOpts.ActionName == "analyzer")
     Builder.defineMacro("__clang_analyzer__");

How about "CI->getPreprocessorOpts().addMacroDef(...)"? I'm not sure where to put it, exactly, but PluginASTAction has a lot of access to the CompilerInvocation. (I believe BeginInvocation would work, but there ought to be a place where you only have to do it once, instead of once per file, right?)


--- a/lib/StaticAnalyzer/Frontend/FrontendActions.cpp
+++ b/lib/StaticAnalyzer/Frontend/FrontendActions.cpp
@@ -17,7 +29,245 @@
   return CreateAnalysisConsumer(CI.getPreprocessor(),
                                 CI.getFrontendOpts().OutputFile,
-                                CI.getAnalyzerOpts(),
+                                this->Opts,
                                 CI.getFrontendOpts().Plugins);
 }

Very small thing: convention in Clang is not to use 'this'. (You've done this several times in this file.)


+// TODO: parsing all the current arguments is gonna require factoring
+// the Arg* and Opt* classes out of lib/Driver into lib/Basic,
+// and splitting the -analyzer-* options out of Driver/CC1Options.td
+// into a new file, StaticAnalyzer/AnalyzerOptions.td.

...which you've now done already, right?


+  driver::OptTable *Table = createAnalyzerOptTable();
+  std::vector<const char*> cargs;
+  for (unsigned i = 0, e = args.size(); i != e; ++i)
+    cargs.push_back(args[i].c_str());
+
+  const char *const *ArgBegin = &cargs[0];
+  const char *const *ArgEnd = ArgBegin + cargs.size();
+  unsigned MissingArgIndex, MissingArgCount;
+  driver::InputArgList *Args = Table->ParseArgs(ArgBegin, ArgEnd,
+                                           MissingArgIndex, MissingArgCount);

Yuck. Can we tag this with a FIXME? ParseArgs should really take an ArrayRef anyway, and it ought to be able to handle std::vector<std::string> if we're going to use it that way.


+  bool success = Args && ParseAnalyzerArgs(this->Opts, *Args, D);

Personally I'd prefer early returns: if (!Args), then if (!ParseAnalyzerArgs).


+  // Honor -analyzer-checker-help.
+  // This should happen AFTER plugins have been loaded!
+  if (this->Opts.ShowCheckerHelp)
+    ento::printCheckerHelp(llvm::outs(), CI->getFrontendOpts().Plugins);

The old behavior of -analyzer-checker-help is to terminate after printing the help. Not sure if we want to keep this or not, but at the very least we shouldn't get a "no inputs" error. (Maybe I should write a simple test case for this...).

Also, the comment about plugins is probably unnecessary once the analyzer is a plugin itself, and should be removed.


+  delete Args;
+  delete Table;

Please use LLVM's RTTI for this: llvm::OwningPtr. That way my preference for early returns won't cause a memory leak. :-)



On Dec 17, 2011, at 5:06, nobled wrote:

> Not sure how to post large changes; I squashed the changes into a
> single patch attached here. Most of its size is from updating
> test/Analysis/. I was working on it as a series of ten patches though,
> if anyone wants a git branch for easier review.
> 
> This change mostly moves the option parsing code out of libFrontend
> into libStaticAnalyzer. It can't compiler the analyzer as a separate
> loadable module, yet.
> 
> Ultimately, this could remove the dependencies that libFrontendTool
> has on all the static analyzer libraries, but can't quite yet because
> the final change I tried to make (removing the RunAnalysis enum and
> the related code) mysteriously caused "unable to find plugin
> 'analyzer'" regressions, so I left that change out.
> 
> Also allll the tests were updated from using `clang -cc1 -analyze` to
> using `clang -cc1 -plugin analyzer`, with some copious
> -plugin-arg-analyzer sprinkled in between to pass the arguments to the
> plugin layer.
> 
> Somehow, this change exposed a problem in lib/Driver/Tools.cpp, which
> was by default adding "-static-analyzer-checker=security" to the
> analyzer. I'm not sure how it ever worked before, because after this
> change I got the error "no analyzer checkers are associated with
> 'security'", which was true; changing it to "experimental.security"
> fixed it.
> 
> There is a small hack in InitPreprocessor.cpp now, though, since I'm
> not sure how to add a predefined macro ("__clang_analyzer__") from a
> PluginASTAction.
> <0009-StaticAnalyzer-factor-into-an-AST-plugin.patch>_______________________________________________
> cfe-dev mailing list
> cfe-dev at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev