[PATCH] Implement a sane plugin API for clang

[Jordan Rose]

On May 5, 2013, at 22:19 , Joshua Cranmer <pidgeot18 at gmail.com> wrote:

> 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.

I know. I think the general lack of interest from the community says a few things about this: the general response to your new architecture has been "mildly positive" or thereabouts, but nobody's been dying for this and so nobody's shaping it in any significant way. Adding new UI that needs to be future-proof should always be taken seriously

But on the other hand, you have had this up for a long time, and I can see how this is a stepping-stone to more significant improvements (I like where the custom C++11 attributes idea is going). So I won't block this, and I appreciate you working on it.


>>> +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.

This is true of the Clang driver, but not necessarily true of a (hypothetical) clang compilation or indexing server. Then again, leaving out the callback for now doesn't preclude adding it later.


>>> +/// 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.

Fair enough. I doubt VersionTuple is changing any time soon, but you make a good point.


>>> +    /// 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.

Ha, okay.

Jordan

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