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

nridge added a comment. In D92155#2454175 , @sammccall wrote: > I thought `getConfiguration('clangd')` with no scope specified was supposed > to be global (i.e. not a workspace-specific setting). There's a scope you can > pass in, and we're not providing

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

sammccall added a comment. In D92155#2453597 , @psionic12 wrote: > In D92155#2450474 , @nridge wrote: > >> Something just occurred to me: can't clangd arguments also be controlled by >> the untrusted repository by

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

psionic12 added a comment. In D92155#2450474 , @nridge wrote: > In D92155#2419549 , @sammccall wrote: > >> In D92155#2419346 , @psionic12 >> wrote: >> >>> Or, could you help

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

nridge added a comment. In D92155#2419549 , @sammccall wrote: > In D92155#2419346 , @psionic12 wrote: > >> Or, could you help to point out what's the difference between passing a >> plugin path through *clangd* sta

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

psionic12 added a comment. > (One idea we've been playing with is that traversal of the AST usually goes > through TranslationUnitDecls::decls_begin(), which triggers loading from the > preamble. We could add a stateful flag to DeclContext that causes this to > behave like noload_decls_begin in

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

sammccall added a comment. In D92155#2419667 , @psionic12 wrote: >> - main file AST build: Important to run plugins here but it's critical they >> don't traverse the whole preamble (can degrade latency by orders of >> magnitude). > > Well, Sound reasonab

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

psionic12 added a comment. > - code completion: this shouldn't run plugins I believe: latency is critical > and we don't emit any diagnostics Totally agree. > - main file AST build: Important to run plugins here but it's critical they > don't traverse the whole preamble (can degrade latency by

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

psionic12 added a comment. > 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) I don't thinks so, plugins are loaded by using `-

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

sammccall added a comment. Another consideration I haven't mentioned: clangd parses files in a few different modes. - code completion: this shouldn't run plugins I believe: latency is critical and we don't emit any diagnostics - main file AST build: Important to run plugins here but it's critic

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

sammccall added a comment. In 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 co

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

nridge added a comment. > What I'm asking specifically is: this feature has a cost, how important is > supporting it? Are there codebases where these attributes are widely used, > and enforcement at development time is particularly valuable? FWIW, one of the codebases I work on uses a clang plu

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

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

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

sammccall added a comment. TL;DR: clangd has some different concerns than clang, so we really need to approach this as a new feature with some design required (even if a mechanism already exists). The most promising ideas I can think of involve doing the loading at clangd startup based on some

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

psionic12 added a comment. > Right, I understand (a little bit, at least!) what plugins *can* do. What I'm > asking specifically is: this feature has a cost, how important is supporting > it? Are there codebases where these attributes are widely used, and > enforcement at development time is pa

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

psionic12 added inline comments. Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3902 + // Load any requested plugins. + for (const std::string &Path : Res.getFrontendOpts().Plugins) { sammccall wrote: > how does this code behave if CLANG_PLUGIN_SUPPORT

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

sammccall added a comment. In D92155#2418693 , @psionic12 wrote: > In D92155#2418617 , @sammccall wrote: > >> - what are the plugins you want to use with clangd? (i.e what kinds of >> plugins are most important, wh

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

psionic12 added a comment. In D92155#2418617 , @sammccall wrote: > - what are the plugins you want to use with clangd? (i.e what kinds of > plugins are most important, what are they useful for) Any plugins which will report diagnostics, especially custom

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

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.

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

psionic12 added a comment. And I checked the CMakeList.txt for the driver, if `CLANG_PLUGIN_SUPPORT` is off, I guess the "undefined symbol` problem will also happens, I think this maybe one reason that option `CLANG_PLUGIN_SUPPORT ` is as `ON` by default. Repository: rG LLVM Github Monorepo

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

psionic12 added a comment. In D92155#2417758 , @njames93 wrote: > Is it possible to write tests for this, obviously they will only work with > plugin support on the current platform. > Also now that this is loaded from the command line, how will this play

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

njames93 added a comment. Is it possible to write tests for this, obviously they will only work with plugin support on the current platform. Also now that this is loaded from the command line, how will this play nicely with other tools that haven't been built with plugin support? Repository:

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

psionic12 updated this revision to Diff 307763. psionic12 added a comment. Fix some syntax Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92155/new/ https://reviews.llvm.org/D92155 Files: clang-tools-extra/clangd/tool/CMakeLists.txt clang/lib/F

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

psionic12 created this revision. psionic12 added reviewers: njames93, grosser, chapuni, john.brawn, alexfh, sammccall, nridge. Herald added subscribers: cfe-commits, usaxena95, kadircet, arphaman, mgorny. Herald added a project: clang. psionic12 requested review of this revision. Herald added a su