[PATCH] D92155: Load plugins when creating a CompilerInvocation.

Thu Nov 26 08:26:21 PST 2020

sammccall added a comment.

In D92155#2418693 <https://reviews.llvm.org/D92155#2418693>, @psionic12 wrote:

> In D92155#2418617 <https://reviews.llvm.org/D92155#2418617>, @sammccall wrote:
>
>> - what are the plugins you want to use with clangd? (i.e what kinds of plugins are most important, what are they useful for)
>
> Any plugins which will report diagnostics, especially customized attribute plugins, you can check `clang/examples/` for details. Examples like `AnnotationFunctions`, `Attribute` and `CallSuperAttribute`(which is wrote by me) all defined a customized attribute and apply some rules on it, if the code do not followed the rule, diagnostics are reported.

Right, I understand (a little bit, at least!) what plugins *can* do. What I'm asking specifically is: this feature has a cost, how important is supporting it? Are there codebases where these attributes are widely used, and enforcement at development time is particularly valuable?

>> - are plugins always threadsafe?
>
> Could you give an example about what kind of thread-safety problem you are imagining? As long as I wrote clang specific plugins, only callbacks are needed to implement, no thread functionalities are used, and as far as I know, clang use single thread to deal with lex, parser, and sema.  So I think thread safety won't be a problem here.

Sorry, I probably should have said "can plugins be thread-hostile".
Clangd runs several parsing actions in parallel, each on a single separate thread.
If a plugin assumes there's only a single instance of it running and uses static data accordingly, we're going to hit problems in clangd. 
The problem is that precisely because clang uses a single thread, (some) plugins may think this is a reasonable assumption. And it's not possible to tell "from the outside" whether a given plugin is unsafe in this way.

>> - are there any meaningful restrictions from a security perspective (e.g. can it load arbitrary .sos from disk, given control over compile args)
>
> Sorry I can't tell, but I think this patch is some sort of NFC patch, what you concern will or will not exist with or without this patch.

It looks like this patch moves `LoadLibraryPermanently` to a different location, per the patch description clangd does not currently load plugins. That's why I'm concerned this patch may introduce unsafe loading of untrusted .sos.

>> - how robust are plugins when the main frontendaction is nonstandard (e.g. uses preambles, skips function bodies)
>
> Different plugins have different callbacks, these callbacks will only get called if the compile work flow goes there, that is to say if the frontendcation skip the bodies somehow(sorry I don't know how could it be done, could you help to explain more?), the callback will not triggered.

SkipFunctionBodies is an option in FrontendOpts that produces a different AST that clang itself (which does not set this option) can't produce. Basically I'm saying "can we tell if a plugin is going to work with our weird ASTs, or do we just have to try and maybe crash".

>> - the filesystem access to the libraries isn't virtualized (vfs) and I don't know if it really can be.
>
> Sorry I don't quite understand, as long as I know the plugin is no more than a shared library, and the driver is no more than a executable, the load procedure seems no different with other `dlopen` cases.

We don't support any other dlopen cases in clangd.

>> - consequences of bad behavior/crash are worse, because clangd is long-running
>
> Does clangd shares the same process with an `FrontendAction`? Because in clang, if a plugin carshes, the whole process does terminated, clang didn't handle this yet(just a core dump).

Yes. And I'm not convinced it's possible to handle this safely without large architectural changes.

> Anyway, some of your concerns seems out of my scope, since clangd have never worked with plugins before, do you think it's better to add some one who knows these? (I just don't know who to add...)

I think we need someone who understands plugins well why they're safe (or how they can be made safe) to work with clangd, and who's willing to do much of the work to do so.
Absent that, I think we probably shouldn't enable them (beyond an experiment).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92155/new/

https://reviews.llvm.org/D92155