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

Fri Nov 27 00:31:27 PST 2020

sammccall added a comment.

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

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

This is exactly the problem, filenames specified in *clang* flags are *supposed* to be read from the VFS. (In practice this probably just means we'd need to disable this feature in environments where VFS is used)

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

Sure. TL;DR is: clangd flags are configured by the user, user can be fully responsible for security/stability.
clang flags are configured by the project. If they're bad, we can e.g. give bad diagnostics, but can't crash or compromise security.

More detail:

In the simplest possible case, clangd is configured as follows:

1. user downloads clangd binary
2. user installs an LSP plugin for their editor, and configures the plugin to use /usr/bin/clangd for C++ files. clangd starts when the editor does
3. the build system for $PROJECT generates $PROJECT/compile_commands.json
4. when the user opens $PROJECT/src/foo.cpp in the editor, it notifies clangd. clangd searches for $PROJECT/compile_commands.json, finds the clang arguments, and uses them to parse foo.cpp

*clangd* command-line flags would be added explicitly by the user at step 2. We can reasonably ask the user to be aware/responsible for security/stability implications of doing this, including with their particular clangd version. We can also ask them to run `clangd --check` without the plugin flag to test whether the plugin is causing a stability problem.

*clang* command-line flags are added implicitly in step 3. Or they could simply be checked into the repository - nothing ensures they were generated locally by the build system. The point is in typical usage they are not controlled by the user directly, and from a security perspective are not trusted (as safely opening files from untrusted repos is a reasonable expectation). So if we're loading plugins based on instructions in clang command-line flags, clangd bears most of the responsibility for making sure that's safe and correct (and I don't see a way to do that).

================
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;
----------------
psionic12 wrote:
> 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).
That sounds good! I do see mutexes in the posix implementation so I guess the LLVM wrapper is intended to be threadsafe.

================
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();
----------------
psionic12 wrote:
> 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.
It seems plugins can run before/after/replace the main action.

I wonder if it'd break things to silently "promote" a replace plugin to e.g. an after one, which I guess would solve this.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92155