Hi, could anyone help to commit this?

In D92155#2450474, @nridge wrote:

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* 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).

Something just occurred to me: can't clangd arguments also be controlled by the untrusted repository by having a .vscode/settings.json file with specificed "clangd.arguments" checked in?

(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 instead...)

Fix grammar
Simplify the test case

Use a vector to collect arguments

Fix grammar

Add tests to check if attributes are attached to AST

• 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 orders of magnitude).

Well, Sound reasonable, but what if getTranslationUnitDecl() is indeed needed instead of using TraverseAST (I didn't came up with an example yet)? In this case I think clangd should allow a plugin to do its job, even if it is performance hostile.

• preamble build: this is tricky, we *rarely* emit diagnostics from this step, and it's very performance-sensitive. It's a separate action from the main file AST build so plugins won't be able to see the results of analysis. I think we shouldn't run plugins here.

I got a question, if "we *rarely* emit diagnostics from this step", what will happen if I open the source file of the header? Can I consider that the diagnostic from the header is not generated by creating PCH (Simply speaking preamble build do not generate diags, and diags from a header file do not generated from preamble build stage)? If so, I think no plugin loading in this step is fine.

• background indexing: again, no reason to run plugins here

Agree, but I got a question, what if a error diagnostic exist in the code, will the index procedure do?

• we need to ensure the traversal scope is already set by the time plugin hooks that may perform traversals (e.g. ASTConsumer::handleTranslationUnit) are called

I think it's hard to implement, and even so a plugin may still need to traversal out of scope, and I think we should let them do that.

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 -fplugin=xxxx.so, this is a flag being parsed and saved in FrontendOptions::Plugins, which is a string vector. As you can see, xxx.so is treat as a string, not a file, vfs didn't load it to memory like normal files (for example main.cpp) . So I think whether VFS is used or not has nothing to do with plugin loading procedure.

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).

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)

ping

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

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)

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.

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 nicely with other tools that haven't been built with plugin support?

Fix some syntax

@aaron.ballman That would be nice if your could help, and Yafei Liu <psionic12@outlook.com> is okay.

Simplify the test case

use VerifyDiagnosticConsumer (-verify) instead of FileCheck for syntax only feature test.
remove illustration in ClangPlugins.rst (which is not very appropriate)

Fix the grammar
Simplify the code logic
Simplify the test case

I just enabled LLVM_ENABLE_DOXYGEN LLVM_BUILD_DOCS, and use make doxygen-clang, it seems no error with my newest patch, is this

fix grammer errors in warning messages and documents

Add more details in ClangPlugins.rst.
Re-design overridden usage checking.
Fix some code style issues.

add final attribute checking