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