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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
