[PATCH] Implement a sane plugin API for clang

[Sean Silva]

On Fri, Mar 15, 2013 at 9:03 PM, Joshua Cranmer <pidgeot18 at gmail.com> wrote:

>  On 3/14/2013 3:42 PM, Sean Silva wrote:
>
> On Thu, Mar 14, 2013 at 12:30 AM, Joshua Cranmer <pidgeot18 at gmail.com>wrote:
>
>> On 3/13/2013 10:05 PM, Sean Silva wrote:
>>
>>> While I'm all for the idea of improving the plugin API, I think that a
>>> modest reduction in boilerplate is not sufficiently compelling to foist a
>>> new plugin API on people who already have existing code. The funny thing
>>> about boilerplate is that it's easy to copy-paste, so it doesn't really
>>> impede people from achieving their goals since they can just copy the code
>>> that already works. The primary problem of boilerplate is that it has the
>>> effect of deterring newbies, and that issue can be easily combated with
>>> improved documentation, which avoids breaking every external plugin and
>>> tutorial on plugins.
>>>
>>
>>  One thing that REALLY sucks with the current approach is the need to
>> specify clang -Xclang -load -Xclang <plugin tarball> -Xclang -add-plugin
>> <plugin name> -Xclang -plugin-<name>-arg -Xclang <blah> ...
>> With the new approach, the command line is clang -fplugin=<tarball>
>> -fplugin-arg-<name>-<arg>=<blah>, which is a much shorter command line and
>> can actually be passed into CFLAGS/CXXFLAGS without driving libtool bonkers
>> (I detest the need for wrapper scripts just to pass arguments) and also
>> eliminates warnings whenever people use $(CXX) $(CXXFLAGS) as the linker.
>
>
>
> Realistically a tiny script might be a better long-term design, allowing
> e.g. "clang++ `clang-plugin-config myPlugin.so arg1 arg2` foo.cpp".
> Remember that the primary advantage of plugins vs libtooling/libclang is
> that they are run as part of a build process, meaning that in reality these
> command lines are meant to be generated by a "configure" step and not by
> hand. So really the "user friendliness" is determined by "how easy is it to
> integrate a clang plugin into my build", and not by the exact commandline
> syntax per se. This kind of script could also serve as a useful layer of
> indirection and "user friendliness", e.g. it could recognize a
> "CLANG_PLUGIN_PATH" or other niceties that would be dubious to add to clang
> itself.
>
>
> A "tiny script" sounds to me a much worse option: you now have to run two
> processes per compiler invocation instead of one (and on Windows, process
> creation can take hundreds of milliseconds, so it's a noticeable slowdown).
>

The clang driver already forks at least one process per compiler
invocation. Your comments apply equally to that and I don't see anybody
running to fix them (or even complaining), so I'm not convinced that this
is really as significant of an issue as you make it out to be.


> Also, you have to find more binaries to run it: if I specify CXX via a
> path, how should a build system know where to run clang-plugin-config from?
> You could guess by looking up the dirname of CXX and hoping it's there,
>

I'm not sure I follow your point here. I image clang-plugin-config and the
wrapper to be installed next to clang and be looked up/executed as usual.

but you are also advocating using shell scripts to represent CXX in another
> email, which renders this approach impossible.
>

I also don't see the connection with my suggestion in the other email. In
fact, the wrapper script for plugins and the compile_commands.json
harvester could probably be the same script, and at configure time
clang-plugin-config (or, perhaps better just `clang-config` now that it is
going beyond plugins) would arrange for the wrapper script to perform the
requested actions.

>   If you really want to immediately push plugins forward in a big way, it
> would be monumental to set up a buildbot that runs a clang plugin that does
> extra checking that isn't really appropriate for being integrated as a
> diagnostic into the compiler proper. For example, a plugin that warns on
> incorrect uses of dyn_cast<>. For maximum effect this should be developed
> in-tree (probably in clang-tools-extra. Even though it has "tools" in the
> name, I don't think anybody would be opposed to developing plugins in
> there). It should also have an easy way for people in our community to come
> up with and implement good extra checks and get them integrated into that
> buildbot.
>
>
> I am working on adding a compiler static checker plugin to Mozilla that
> would check the guarantees our old dehydra plugin used to check: a "must
> override" annotation (all subclasses must provide their own implementation
> of this method), a "stack class" annotation (this class cannot be allocated
> from the heap), and a warning that gets emitted every time you emit a
> static initializer.
>
>
Awesome. Please keep us up to date with this work. Some of these checks
seem like they could be relevant to llvm/clang too.


>   The changes in this patch retain almost all of the same functionality
>> as the original plugin approach (including the ability to do things like
>> add custom compile passes via the RegisterPass statics) while wrapping it
>> in a much saner wrapper.
>>
>>
>  My opposition to the current patch is that it does not provide enough
> value to our users to compensate for the inconvenience that it will cause
> them (by breaking their code). My opposition is not technical; I don't
> doubt that your approach here is an improvement from a purely technical
> standpoint.
>
>
> The current plugin approach presumes that it is a pure consumer of the
> AST, which isn't a viable option in my opinion. One thing I would like to
> do in the future is be able to map Decls in the AST to functions emitted in
> the LLVM IR, which is completely impossible under the current architecture.
> Note also that I'm not removing the current (more or less broken) plugin
> architecture, so I'm not compelling people to switch.
>

You did delete the only code (PrintFunctionNames) in tree that AFAIK tests
the previous functionality, which I interpreted as meaning that it was dead
to you.


> Rather, this is about enabling future changes that permit plugins to not
> take the view that they happen independently of code generation.
>

This did not get through to me from the OP. Could you explain how the
design you implement in this patch achieves that? It should be the emphasis
of the review (and IMHO warrants a "does this direction and implementation
approach sound good to everyone" cfe-dev discussion before proposing code
to be committed).

Also, the command line parsing stuff should be in a separate patch, and IMO
the -fplugin should be just a driver arg: that way, the previous
commandline args for plugins (directly via cc1) remains in a live code path.

As I said earlier, the compatibilty stuff also deserves a rehash, since I'm
still not convinced that it is really useful.

-- Sean Silva
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130316/d0429398/attachment.html>