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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

I don't have a lot of background in clang plugins, either on the technical side or how they're used in practice, so some of this may be a bit underinformed.
This sounds OK for experiments as long as it's off by default, but there may be barriers to going any further.

Some questions to try to understand how this is going to play with clangd:

- what are the plugins you want to use with clangd? (i.e what kinds of plugins are most important, what are they useful for)
- are plugins always threadsafe?
- are there any meaningful restrictions from a security perspective (e.g. can it load arbitrary .sos from disk, given control over compile args)
- how robust are plugins when the main frontendaction is nonstandard (e.g. uses preambles, skips function bodies)

Other points that i'm fairly sure will be problematic:

- the filesystem access to the libraries isn't virtualized (vfs) and I don't know if it really can be.
- consequences of bad behavior/crash are worse, because clangd is long-running
- if a plugin works with clang but not clangd, it's unclear whether/where there's a bug



================
Comment at: clang-tools-extra/clangd/tool/CMakeLists.txt:44
+# Support plugins.
+if(CLANG_PLUGIN_SUPPORT)
+  export_executable_symbols_for_plugins(clangd)
----------------
This looks like it's on by default - it should be off at least by default until:
 - support surface of this is understood by maintainers
 - security is understood and safe (command-line arguments are effectively attacker-controlled for clangd)
 - we've verified that this actually works well with important plugins


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3902
 
+  // Load any requested plugins.
+  for (const std::string &Path : Res.getFrontendOpts().Plugins) {
----------------
how does this code behave if CLANG_PLUGIN_SUPPORT is off?


================
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;
----------------
is this threadsafe on all platforms?


================
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();
----------------
we can't support replacement of the main action in clangd. What's the plan there - ignore the plugin?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92155