Page MenuHomePhabricator

psionic12 (Yafei Liu)
User

Projects

User does not belong to any projects.

User Details

User Since
Oct 23 2020, 1:49 AM (12 w, 5 d)

Recent Activity

Dec 20 2020

psionic12 added a comment to D92006: Refactoring the attribute plugin example to fit the new API.

Hi, could anyone help to commit this?

Dec 20 2020, 7:55 PM · Restricted Project

Dec 14 2020

psionic12 added a comment to D92155: Load plugins when creating a CompilerInvocation..

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?

Dec 14 2020, 6:12 PM · Restricted Project

Dec 9 2020

psionic12 added inline comments to D92006: Refactoring the attribute plugin example to fit the new API.
Dec 9 2020, 6:03 PM · Restricted Project

Dec 8 2020

psionic12 added a comment to D92155: Load plugins when creating a CompilerInvocation..

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

Dec 8 2020, 6:36 PM · Restricted Project
psionic12 updated the diff for D92006: Refactoring the attribute plugin example to fit the new API.

Fix grammar
Simplify the test case

Dec 8 2020, 6:19 PM · Restricted Project

Dec 7 2020

psionic12 added inline comments to D92006: Refactoring the attribute plugin example to fit the new API.
Dec 7 2020, 3:10 AM · Restricted Project
psionic12 updated the diff for D92006: Refactoring the attribute plugin example to fit the new API.

Use a vector to collect arguments

Dec 7 2020, 3:08 AM · Restricted Project
psionic12 added inline comments to D92006: Refactoring the attribute plugin example to fit the new API.
Dec 7 2020, 1:14 AM · Restricted Project

Dec 6 2020

psionic12 updated the diff for D92006: Refactoring the attribute plugin example to fit the new API.

Fix grammar

Dec 6 2020, 11:32 PM · Restricted Project
psionic12 updated the diff for D92006: Refactoring the attribute plugin example to fit the new API.

Add tests to check if attributes are attached to AST

Dec 6 2020, 11:29 PM · Restricted Project

Dec 4 2020

psionic12 requested review of D92647: Refactor codes and tests to make it work in PCH builds.
Dec 4 2020, 3:04 AM · Restricted Project

Nov 27 2020

psionic12 added a comment to D92155: Load plugins when creating a CompilerInvocation..
  • 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.

Nov 27 2020, 1:25 AM · Restricted Project
psionic12 added a comment to D92155: Load plugins when creating a CompilerInvocation..

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

Nov 27 2020, 12:51 AM · Restricted Project

Nov 26 2020

psionic12 added a comment to D92155: Load plugins when creating a CompilerInvocation..

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)

Nov 26 2020, 7:17 PM · Restricted Project
psionic12 added a comment to D92006: Refactoring the attribute plugin example to fit the new API.

ping

Nov 26 2020, 6:23 PM · Restricted Project
psionic12 added a comment to D92155: Load plugins when creating a CompilerInvocation..

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?

Nov 26 2020, 9:16 AM · Restricted Project
psionic12 added inline comments to D92155: Load plugins when creating a CompilerInvocation..
Nov 26 2020, 8:11 AM · Restricted Project
psionic12 added a comment to D92155: Load plugins when creating a CompilerInvocation..
  • what are the plugins you want to use with clangd? (i.e what kinds of plugins are most important, what are they useful for)
Nov 26 2020, 8:00 AM · Restricted Project
psionic12 added a comment to D92155: Load plugins when creating a CompilerInvocation..

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.

Nov 26 2020, 12:53 AM · Restricted Project
psionic12 added a comment to D92155: Load plugins when creating a CompilerInvocation..

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?

Nov 26 2020, 12:40 AM · Restricted Project

Nov 25 2020

psionic12 updated the diff for D92155: Load plugins when creating a CompilerInvocation..

Fix some syntax

Nov 25 2020, 11:06 PM · Restricted Project
psionic12 requested review of D92155: Load plugins when creating a CompilerInvocation..
Nov 25 2020, 11:03 PM · Restricted Project

Nov 23 2020

psionic12 abandoned D91239: Update attribute example to fit the new Annotation API.
Nov 23 2020, 11:14 PM · Restricted Project
psionic12 requested review of D92006: Refactoring the attribute plugin example to fit the new API.
Nov 23 2020, 11:13 PM · Restricted Project

Nov 19 2020

psionic12 added a comment to D91047: Add a call super attribute plugin example.

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

Nov 19 2020, 6:08 PM · Restricted Project

Nov 18 2020

psionic12 updated the diff for D91047: Add a call super attribute plugin example.

Simplify the test case

Nov 18 2020, 6:41 PM · Restricted Project

Nov 17 2020

psionic12 added inline comments to D91047: Add a call super attribute plugin example.
Nov 17 2020, 3:54 AM · Restricted Project
psionic12 added inline comments to D91047: Add a call super attribute plugin example.
Nov 17 2020, 3:52 AM · Restricted Project
psionic12 updated the diff for D91047: Add a call super attribute plugin example.

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

Nov 17 2020, 3:43 AM · Restricted Project

Nov 15 2020

psionic12 added inline comments to D91047: Add a call super attribute plugin example.
Nov 15 2020, 7:55 PM · Restricted Project
psionic12 updated the diff for D91047: Add a call super attribute plugin example.

Fix the grammar
Simplify the code logic
Simplify the test case

Nov 15 2020, 7:38 PM · Restricted Project

Nov 12 2020

psionic12 added inline comments to rGd093401a2617: [NFC] Remove string parameter of annotation attribute from AST childs..
Nov 12 2020, 7:56 PM
psionic12 added inline comments to rGd093401a2617: [NFC] Remove string parameter of annotation attribute from AST childs..
Nov 12 2020, 7:48 PM
psionic12 added a comment to D91047: Add a call super attribute plugin example.

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

Nov 12 2020, 7:38 PM · Restricted Project
psionic12 added inline comments to D91047: Add a call super attribute plugin example.
Nov 12 2020, 6:52 PM · Restricted Project
psionic12 updated the diff for D91047: Add a call super attribute plugin example.

fix grammer errors in warning messages and documents

Nov 12 2020, 6:49 PM · Restricted Project

Nov 11 2020

psionic12 added inline comments to D91239: Update attribute example to fit the new Annotation API.
Nov 11 2020, 12:25 AM · Restricted Project
psionic12 requested review of D91239: Update attribute example to fit the new Annotation API.
Nov 11 2020, 12:17 AM · Restricted Project

Nov 10 2020

psionic12 added inline comments to D91047: Add a call super attribute plugin example.
Nov 10 2020, 4:16 AM · Restricted Project
psionic12 updated the diff for D91047: Add a call super attribute plugin example.

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

Nov 10 2020, 4:02 AM · Restricted Project
psionic12 added inline comments to D91047: Add a call super attribute plugin example.
Nov 10 2020, 1:59 AM · Restricted Project

Nov 9 2020

psionic12 added inline comments to D91047: Add a call super attribute plugin example.
Nov 9 2020, 6:56 PM · Restricted Project
psionic12 added inline comments to D91047: Add a call super attribute plugin example.
Nov 9 2020, 1:38 AM · Restricted Project
psionic12 added a reviewer for D91047: Add a call super attribute plugin example: john.brawn.
Nov 9 2020, 1:28 AM · Restricted Project
psionic12 updated the diff for D91047: Add a call super attribute plugin example.

add final attribute checking

Nov 9 2020, 1:23 AM · Restricted Project

Nov 8 2020

psionic12 added a reviewer for D91047: Add a call super attribute plugin example: erichkeane.
Nov 8 2020, 10:57 PM · Restricted Project
psionic12 requested review of D91047: Add a call super attribute plugin example.
Nov 8 2020, 10:40 PM · Restricted Project