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

Thu Nov 26 09:16:25 PST 2020

psionic12 added a comment.

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

Well, I can't tell you how widely plugin used, because they often used in third-party projects. Firefox team seems use them a lot, the `ParsedAttrInfo` plugin implementation seems write by them if I remember right. And our team use both plugins and clangd a lot, that's why I'm willing to contribute to make them work together. As about the importance, my personal opinion is that since there's a feature, we should try to polish it...

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

That seems a serious problem, but I got a question, what if the clang itself declared some static data (I'll investigate later, just ask for now)?

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

Well, since it called plugin, it's users choose to use it or not, I personally think we can't check if an `.so` is trusted, and neither should we care...

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

I'll test this.

> - the filesystem access to the libraries isn't virtualized (vfs) and I don't know if it really can be.

Sorry I still can't understand this, could you help to explain more? What I mean before is that since this part of code is used in clang driver as well, it should be no problem get called by 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.

Well, I guess currently I can do nothing to improve this, but if a user uses a plugin, it crashed, that's should be the plugin's problem...

> Absent that, I think we probably shouldn't enable them (beyond an experiment).

How about capsule the loading functionalities to a function, and call it from clangd if experimental feature is checked? In this way nothing changed if the clangd's experimental feature is off.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92155