[PATCH] Implement a sane plugin API for clang

[Joshua Cranmer]

On 5/3/2013 6:18 PM, Jordan Rose wrote:
>>> We really ought to have a preliminary discussion on cfe-dev to map 
>>> out the directions we want to take the plugin API, and what 
>>> *compelling*, *enabling* changes we can make to it.
>
> While I mostly like what you've come up with, Sean has a point that 
> the community hasn't really picked it up. I have your e-mail from a 
> year ago still flagged ("[cfe-dev] Thinking about clang plugins"). No 
> one responded to it then, but I think it was a good summary of the 
> problems you were/are trying to solve. I would say to bring that up 
> again...except most likely nothing will happen until after the 3.3 
> release (and for us Apple folks, after WWDC).

I had originally wanted to get this patch into the 3.1 release. That 
nothing can come before 3.3 is... disheartening, to say the least.
>> +Clang Plugins run a set of callbacks over code. These callbacks are 
>> set up by
>> +the plugin when the compiler calls a function exported by the 
>> plugin. The main
>> +function exported by the plugin is clang_plugin_begin_tu, which is 
>> called before
>> +any file is compiled.
>
> s/any/each/, I think. Is there a separate callback that is guaranteed 
> to only be called once, or at least once per CompilerInstance?

If you call clang a.cpp b.cpp, the last time I checked, this spawns two 
clang -cc1 processes. Since the plugin is loaded in the clang -cc1 
process, it gets loaded twice, so call-per-invocation is effectively 
equivalent to call-per-translation unit. My original implementation had 
a call-per-invocation callback, but I removed it after discovering this 
fact since its effect were misleading.

>> +/// A struct containing sufficient information to express ABI 
>> compatibility for
>> +/// plugins. It is sufficient for plugins to use the
>> +/// DECLARE_PLUGIN_COMPATIBILITY macro to find compatibility.
>> +struct Version {
>> +  /// The major version of clang (corresponds to CLANG_VERSION_MAJOR)
>> +  unsigned Major;
>> +  /// The minor version of clang (corresponds to CLANG_VERSION_MINOR)
>> +  unsigned Minor;
>> +};
>
> VersionTuple is a bit overkill here, but do we really need another 
> version struct?

My main rationale for making a new version struct was that this would be 
effectively ABI-frozen. Since the version code is used to decide whether 
or not to attempt to use the plugin, an ABI-mismatch is particularly 
deadly for the compiler in this code.

>> All non-null callbacks will be deleted by the compiler internally.
>
> "On return, the compiler takes ownership of any non-null callbacks."
>
> ...although I'm not sure this is a good idea. Why not reuse the same 
> callbacks over multiple TUs?

As mentioned earlier, in practice, a single invocation gets called for a 
single TU.
>> +    /// Arguments to the plugin
>> +    SmallVector<std::pair<std::string, std::string>, 4> Args;
>
> Should this just be a std::vector? I think the most common case will 
> be 0 args, in which case SmallVector doesn't do much good.

I originally had this as a std::vector, and was told to make it a 
SmallVector instead.

> ...any reason not to do this with a map, to avoid N iterations over 
> the arguments?

There is likely to be very few arguments and plugins in use for a given 
invocation; with such small numbers, the extra overhead of a map isn't 
worth it.

-- 
Joshua Cranmer
News submodule owner
DXR coauthor

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130506/5b900b63/attachment.html>