[cfe-commits] [PATCH] Implement a better plugin API for clang, v5

[Sean Silva]

> Isolating this is impossible without either using lambdas (not yet
> permissible in LLVM/clang), function class boilerplate (*a lot* of boiler
> plate), macros that take expressions with unbound variables as parameters
> (or just other macros), or refactoring every plugin-hookable place. The
> amount of duplicated code is about 4 lines of code; none of the possible
> options (with the possible exception of lambdas) would make things
> significantly easier or clearer considering how large they would be. I tried
> doing this earlier, in the first few versions of my patch; I gave up trying
> to do this long ago since none of the options are palatable, and it's just
> simply not worth it.

I'm not seeing this. It could be as simple as having each class with
"callbacks" have a method similar to PP.addPPCallbacks(). The repeated
loops are not what concerns me. An easy way to accomplish this is to
document how to add plugin extensibility to a class (something that
you have to do anyway...); then, starting with an empty callbacks
struct (or whatever), implement the current functionality
(DiagnosticClient/ASTConsumer/PPCallbacks) by following that
documentation you wrote. In fact, adding back the current
functionality could (probably should) be separate patches.

> The problem with your suggestion here is that it becomes impossible to
> disambiguate "you mistyped the function hook" from "you compiled with the
> wrong version of clang."

Not with the CLANG_PLUGIN_HOOK_FUNC approach. It would be trivial to
try looking for the symbols from previous versions of clang when the
CLANG_PLUGIN_HOOK_FUNC for the current version is not found; if a
previous version's CLANG_PLUGIN_HOOK_FUNC is found, then the
diagnostic can be adjusted appropriately. This silently does the right
thing without the user even having to concern themselves about it.

Another approach I can think of is to move the creation of the
callbacks struct to the hook function somehow, and then make it have
an inlined constructor that embeds the version information there. This
approach also silently does the right thing.

> The downside to your approach is it means that everyone who wants to
> use CompilerInstance has to learn about the PluginManager as well (go C++
> :-/ ).

This is the kind of thing usually solved with a pimpl. Actually, as I
look at it more closely, I see no reason why initializePlugins()
should even be exposed in the header since things will break if this
is ever called other than in the one place where it supposed to be
called, so a pimpl is probably needed.

> I understand what you're saying, but from an aesthetic design
> point, the plugin manager shouldn't care *how* the arguments to plugins are
> structured; that's the job of the part of the tool that deals with the
> argument parsing (i.e., the CompilerInstance).

The loadPlugin interface is broken anyway, as currently nothing is
statically enforcing the ordering on method calls for PluginManager.
For example, if makes no sense whatsoever to call addPPCallbacks
before callOnTUHook, or callOnTUHook before all calls to loadPlugin
are done. I think that a redesign is in order to enforce these
invariants by design (this is C++: users deserve to have the compiler
enforce these things).

--Sean Silva

On Fri, Aug 3, 2012 at 8:39 AM, Joshua Cranmer <pidgeot18 at gmail.com> wrote:
> On 8/2/2012 5:44 PM, Sean Silva wrote:
>>
>> These are all doing the same thing and in basically the same way. Try
>> to isolate and factor this commonality. This will set a clear
>> precedent for how to add "observer"-like plugin functionality to a
>> class.
>
>
> Isolating this is impossible without either using lambdas (not yet
> permissible in LLVM/clang), function class boilerplate (*a lot* of boiler
> plate), macros that take expressions with unbound variables as parameters
> (or just other macros), or refactoring every plugin-hookable place. The
> amount of duplicated code is about 4 lines of code; none of the possible
> options (with the possible exception of lambdas) would make things
> significantly easier or clearer considering how large they would be. I tried
> doing this earlier, in the first few versions of my patch; I gave up trying
> to do this long ago since none of the options are palatable, and it's just
> simply not worth it.
>
>
>> +  ArrayRef<FrontendOptions::CompilerPlugin> CompilerPlugins =
>> +    getFrontendOpts().CompilerPlugins;
>> +  for (unsigned i = 0, e = CompilerPlugins.size(); i != e; ++i) {
>> +    std::string Error;
>> +    StringRef Path = CompilerPlugins[i].Plugin;
>> +    if (!PluginMgr->loadPlugin(Path, CompilerPlugins[i].Args, Error)) {
>> +      getDiagnostics().Report(diag::err_fe_unable_to_load_plugin)
>> +        << Path << Error;
>> +      return false;
>> +    }
>> +  }
>>
>> How about PluginMgr->loadPlugins(getFrontendOpts().CompilerPlugins),
>> and move this to PluginManager.cpp? In general, try to obsessively
>> reduce the number of plugin-related lines of code that are not in
>> PluginManager.{h,cpp}; this will make future maintenance easier. I
>> would shoot for 1 line for each logical point of interaction with the
>> plugin system.
>
>
> I expect in the future that there will be other plugin attachment points
> (e.g., something in the driver) that ought to be able to reuse the
> interface. I understand what you're saying, but from an aesthetic design
> point, the plugin manager shouldn't care *how* the arguments to plugins are
> structured; that's the job of the part of the tool that deals with the
> argument parsing (i.e., the CompilerInstance).
>
>
>> +bool CompilerInstance::initializePlugins() {
>> +  assert(!hasPluginManager() && "Already loaded plugins");
>> +  PluginMgr.reset(new plugin::PluginManager);
>>
>> I *really* dislike this pattern which has crept into CompilerInstance.
>> There are all these hidden invariants for which the only
>> "documentation" is asserts (and who knows how many such asserts are
>> missing!). It's extremely error-prone. One way to circumvent this
>> could be to have PluginManager have a sane default constructor and
>> embed it by value in CompilerInstance; I believe that all the methods
>> on PluginManager naturally become noops when there are no plugins
>> loaded. This also prevents there from being a bunch of
>> "hasPluginManager()" checks littering the source code.
>
> The diagnostics consumer one isn't a no-op (it turns a non-owning client to
> an owning client, but ChainedDiagnosticConsumer doesn't allow me to touch
> this). The downside to your approach is it means that everyone who wants to
> use CompilerInstance has to learn about the PluginManager as well (go C++
> :-/ ).
>
>
>> +    ErrorMsg = "Plugin is not compatible with this version of clang";
>> +    return false;
>>
>> This `ErrorMsg` should probably be a real diagnostic and include stuff
>> like "your plugin was compiled for version Foo but this Clang is
>> version Bar". Also, we discussed the merit of even checking this; is
>> it really going to catch bugs? My thinking is that it won't; perhaps
>> the most compelling use case is if your system administrator/automated
>> package update updates clang without you knowing, but I'm not
>> convinced of the usefulness. Also, I think there are better ways of
>> doing this particular check than forcing the plugin writer to add
>> boilerplate; off the top of my head, you could just #define
>> clang_plugin_on_tu to clang_plugin_on_tu_$MAJOR_$MINOR so it will fail
>> to load if the version mismatches (or maybe make it a bit more
>> explicit that it is a macro by calling it CLANG_PLUGIN_HOOK_FUNC or
>> something).
>
> The problem with your suggestion here is that it becomes impossible to
> disambiguate "you mistyped the function hook" from "you compiled with the
> wrong version of clang." One line of boilerplate for versioning is very
> small indeed (compared to other previous approaches).
>
>> I think it is possible to have DynamicLibrary.h included in just
>> PluginManager.cpp. It might require some slight architectural changes
>> but I think it will lead to an overall better design. I similarly
>> think that it's possible for Plugin.h to only be included in
>> PluginManager.cpp: I can't see a compelling reason for any other part
>> of Clang to have to concern itself with what the low-level Plugin
>> interface is.
>
> This is a similar C++-dependency-requirements sucks issue. It's solvable if
> one of the entries ends up introducing a pointer, but none of the levels are
> particularly enticing for introducing pointers.
>
> --
> Joshua Cranmer
> News submodule owner
> DXR coauthor
>