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

[nobled]

(And the tests/ patch again...)

> On Sat, Jan 28, 2012 at 12:22 AM, Jordy Rose <jediknil at belkadan.com> wrote:
>> A few additions to what I said before (some of which probably still need to be changed eventually):
>>
>> AnalysisAction.h:
>> +  void toArgs(std::vector<std::string> &args);
>>
>> Had a thought: since this isn't hooked up right now in CompilerInvocation anyway, why not make it a virtual method on PluginASTAction? Eventually we'll probably want to be able to serialize any plugin invocation anyway.
> Yeah, I figured I would add that method after this series so the
> functionality would become available again.
>
>>
>> Or, the flip side: CompilerInvocation's FrontendOptsToArgs already blindly serializes all plugin options, so maybe we can just kill this.
>>
>>
>> FrontendActions.h:
>> +extern "C" void clang_LinkInStaticAnalyzerPlugin(void);
>>
>> Yuck...but as far as I see, unavoidable in some form. This is a lot less brittle than relying on compiler options to make sure libClangStaticAnalyzer doesn't get left out.
>>
>>
>> FrontendActions.cpp:
>> +  // If -analyzer-checker-help was given, this should be a no-op.
>> +  if (Opts.ShowCheckerHelp)
>> +    return new ASTConsumer();
>> +
>>
>> This isn't really a no-op, since it means the compiler will still go through the whole parsing phase to build an AST. I *think* it's safe to return NULL here, but I haven't tried it.
> Ah, you're right. I tested it out with my patch, and running this:
>
> $ bin/clang -cc1 -plugin analyzer -plugin-arg-analyzer -analyzer-checker-help
>
> will print the help, then hold, waiting for the input file on STDIN.
>
> However, current behavior is broken in its own way already, as far as
> I can tell:
>
> $ clang --analyze -Xanalyzer -analyzer-checker-help
> clang-3: error: no input files
> $ clang -cc1 -analyze -analyzer-checker-help || echo returned nonzero!
> <the normal -analyzer-checker-help spew>
> returned nonzero!
>
> ...So if I wanted, I can just preserve the current behavior by
> returning 'false' from ParseArgs, indicating failure, without actually
> setting a diagnostic. Which is what the current
> return-zero(false)-from-ExecuteCompilerInvocation code does,
> effectively.
>>
>>
>> +  // TODO: CreateASTConsumer() is far too late to add a macro, and
>> +  // ParseArgs() is far too early for getCompilerInstance() to work yet,
>> +  // so we have to switch to a non-const CompilerInstance argument here
>> +  // in order to remove the plugin-specific hack from InitPreprocessor.cpp.
>> +  //CI.getPreprocessorOpts().addMacroDef("__clang_analyzer__");
>>
>> How about BeginInvocation? Or BeginSourceFileAction? FrontendAction has those hooks specifically for this sort of thing. I haven't /tried/ it, but it's before the Preprocessor is created, and after the CompilerInstance is initialized with a file. Yes, it means we add the option for every file, but it's not like InitPreprocessor isn't doing the same work now.
>>
>> (Sorry, the fixme here is really bugging me.)
> Ah, of course. Done in BeginInvocation now. (I see that you mentioned
> it before, but I got stuck on that not-per-file idea, doh.) There's
> one small peculiarity with using addMacroDef() (the define gets listed
> alongside all the driver/user-specified -D commandline options in the
> preprocessed file instead of with the builtins), but it's not one that
> should really matter.
>
>>
>>
>> +static void AnalyzerOptsToArgs(const AnalyzerOptions &Opts,
>> +                               std::vector<std::string> &Res) {
>> +  if (Opts.ShowCheckerHelp)
>> +    Res.push_back("-analyzer-checker-help");
>>
>> It's not so important (since this isn't ever called right now), but this function isn't correct anymore -- all of these options would need "-plugin-arg-analyzer" in front. But again, since plugin args are already blindly serialized by CompilerInvocation, fixing this function may not be worth it.
>>
>> (Also, there's probably no reason anymore to have this outside of AnalysisAction::toArgs.)
>>
>>
>> Longer-term (can fix later): once all the analyzer plugin args go through -plugin-arg-analyzer, we can strip the 'analyzer' prefix off each option.
> Yep, I had the same thought.
>>
>> I'm really happy this is happening. It'll help dogfood better plugin support, I hope. Not to mention we (read: Ted) might be able to publish updates to the analyzer more frequently than Clang updates, at least in theory.
>>
>> (In practice, the analyzer relies so much on libClangAST that this probably isn't feasible. Not yet, anyway.)
>>
>> Thanks again, nobled!
>> Jordy
>>
>>
>> On Jan 24, 2012, at 4:08, nobled wrote:
>>
>>> Attached an updated patch--it had to be rebased against recent changes
>>> in the Driver/Tools.cpp code.
>>>
>>> Also available as broken-down discrete steps on this git branch:
>>> https://github.com/nobled/clang/tree/anal-plugin
>>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0017-update-all-Analysis-tests-to-new-plugin-interface.patch
Type: text/x-patch
Size: 124594 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20120130/38bc51ed/attachment.bin>