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

[Yafei Liu via Phabricator via cfe-commits]

psionic12 added a comment.

> clangd (and other clang tools) get deployed in environments where all access to the filesystem goes through a llvm::vfs::Filesystem, and all filenames referred to in the compile command are within that virtual filesystem rather than the real one.
> (Again, this isn't an issue if we require these paths to be specified on the *clangd* startup command line, as opposed to the clang flags that form the compile command)

Yes I know how clang manager files through vfs, it just that loading libraries does not involve vfs at all, the path of a lib is passed directly to the system level API (eg, dlopen on Linux, System::Load on Windows), so I still can't understand what you are concerning, maybe you could point out a more specific example? Or, could you help to point out what's the difference between passing a plugin path through *clangd* startup command line and through clang flags?



================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3905
+    std::string Error;
+    if (llvm::sys::DynamicLibrary::LoadLibraryPermanently(Path.c_str(), &Error))
+      Diags.Report(diag::err_fe_unable_to_load_plugin) << Path << Error;
----------------
sammccall wrote:
> psionic12 wrote:
> > sammccall wrote:
> > > is this threadsafe on all platforms?
> > While I can't tell, clang driver use this for a while and seems no problem, could you help to point out what's the worst case your concerned about?
> clang-driver isn't multithreaded, so wouldn't show up problems here.
> 
> I'm concerned that if thread A loads plugin X, thread B loads plugin X, and thread C loads plugin Y, all concurrently and using this code path, whether they will all load correctly.
> 
> In clangd these threads could correspond to two open files and a background-index worker.
> 
> (I don't know much about dynamic loading, this may be totally fine)
In this case I can make sure multiple thread works just fine with `LoadLibraryPermanently`, I checked all the dynamic loading API on most platforms and all of the are thread-safe(it's rare to see system APIs which are not thread safe, to me).


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3914
+    if (P->getActionType() == PluginASTAction::ReplaceAction) {
+      Res.getFrontendOpts().ProgramAction = clang::frontend::PluginAction;
+      Res.getFrontendOpts().ActionName = Plugin.getName().str();
----------------
sammccall wrote:
> psionic12 wrote:
> > sammccall wrote:
> > > we can't support replacement of the main action in clangd. What's the plan there - ignore the plugin?
> > Could you help to explain why action replacements are not supported? 
> > 
> > As far as I know, replacement of actions is related with Actions, which does not have directly relationship with clangd,
> Clangd uses some custom FrontendActions (different ones for different ways we're parsing the file) to implement its features (e.g. track which parts of the AST are part of the main-file without deserializing the preamble, and to do indexing).
> If you replace these actions, normal features will not work.
> 
> e.g.:
> - https://github.com/llvm/llvm-project/blob/4df8efce80e373dd1e05bd4910c796a0c91383e7/clang-tools-extra/clangd/ParsedAST.cpp#L97
> - https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/clangd/index/IndexAction.cpp#L206
> 
> If we replace these with the plugin's action, clangd won't work 
I think this is the part I didn't expected, seems a standalone action loading logic needs to exist.

I'll try to come up a patch which has standalone plugin loading and guard it with experimental checking.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92155