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

nobled nobled at dreamwidth.org
Tue Jan 3 06:49:17 PST 2012


On Sat, Dec 31, 2011 at 2:09 AM, Jordy Rose <jediknil at belkadan.com> wrote:
> 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'.
Believe it or not, -Xanalyzer predates the generic -Xclang, so it's
just a lucky accident of history I had this handy abstraction already
in place to take advantage of. (and it was originally "-WA", by
analogy to '-Wl' and friends. shudder.)

Is -cc1 backwards compatibility actually important, though? It's
basically an implementation detail of clang, I thought.

>
> 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.)
Yeah, and hopefully, next we can dump the ARC Migration Tool into a
plugin too(seeing as how it's waaay more domain-specific than the
analyzer), but that requires a new API entirely for generic 'rewrite
the source file' plugin actions that can be chained up before a
'primary' action like codegen. (The current -add-plugin option
'multiplexes' ASTConsumers in parallel, but ARCMT needs to be chained
up sequentially.)

>
> 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.
You mean like an ALL_CAPS name like ADD_PLUGIN_ARG()?

>
>
> --- /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.)
Yeah, I was just blindly following the existing style. Not sure why either.
>
>
> --- 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?
I was thinking about this already actually, and it should be able to
just dump all the plugin arguments from the
ActionName/PluginArgs/AddPluginActions/AddPluginArgs members, exactly
as they were passed to it. Which only has the disadvantage of not
eliminating redundant or ignored options, but that's not really a huge
deal.
>
>
> --- 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.
That's the target for the new TableGen-generated header...
>
>
> --- 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?)
During ParseArgs, I guess?
>
>
> --- 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?
Yeah, that was from a way early draft; already deleted in the newer
version of the patch in the branch.

It turned out to not be strictly necessary for this work, but I did
finally finish a patch to move Arg-parsing into libBasic. I'll send
that out, too.
>
>
> +  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.
Yeah, I was looking at ArrayRef-izing and StringRef-izing whatever I
could in libDriver after this (it's... painful). I'll add an explicit
FIXME.
>
>
> +  bool success = Args && ParseAnalyzerArgs(this->Opts, *Args, D);
>
> Personally I'd prefer early returns: if (!Args), then if (!ParseAnalyzerArgs).
Already fixed in the new patch (already switched to using OwningPtr).
>
>
> +  // 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...).
You're right. Not sure how to fix that, though. ParseArgs can only
return 'success' or 'error', and asking for -help shouldn't make the
program return an error code should it? (Unless it already does that.)
In the future maybe we should add a hook to the plugin class for a
standard 'help' option (so it's just `clang -cc1 -plugin analyzer
-plugin-help`) instead of writing a new one for every plugin.
>
> 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. :-)
Already fixed. Thanks for reviewing what you could!
>
>
>
> 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
>




More information about the cfe-dev mailing list