[clangd] Filter out plugin related flags and move all commandline manipulations into OverlayCDB.

[kadircet]

Summary

Some projects make use of clang plugins when building, but clangd is
not aware of those plugins therefore can't work with the same compile command
arguments.

There were multiple places clangd performed commandline manipulations,
this one also moves them all into OverlayCDB.

[ioeric]

clangd/ClangdUnit.cpp

429

This doesn't seem to be clangd specific; clang-tidy seems to have the same issue. Could we share the filtering logic (e.g. in lib/Tooling/ArgumentsAdjusters.cpp)?

[ilya-biryukov]

LG, just a few comments. Supporting compiler plugins in clangd is non-trivial and we don't support them so far, so filtering out the options make sense.

It feels these helper belong in the Tooling library, that would allow to reuse code between clang-tidy, clangd, etc. Could even be the default behavior, since most of the tools won't handle compiler plugins well, unless compiled with the same exact version of the compiler.
I'd be interested in what other people think, @sammccall, @klimek?

clangd/ClangdUnit.cpp

429

We need a comment mentioning that we are filtering out the plugin options and why we're doing that here

433

Maybe move this code into ClangdServer::getCompileCommand to keep all argument adjustments that we do in clangd in one place.
We add -resource-dir there.

[ilya-biryukov]

clangd/ClangdUnit.cpp

429

+1 to the suggestion.
WDYT about landing it into clangd as a quick workaround and moving into tooling later?
The reason I think it might be better is because that would unblock usage of clangd in Chromium.

[ioeric]

clangd/ClangdUnit.cpp

429

WDYT about landing it into clangd as a quick workaround and moving into tooling later?
The reason I think it might be better is because that would unblock usage of clangd in Chromium.

I don't see why it would take much more effort to move the shared logic into tooling now?

[kadircet]

It seems like the most relevant place in tooling is ArgumentsAdjuster as @ioeric pointed out. Happy to move it to there if @sammccall and @klimek agrees as well.

As a side note ArgumentsAdjusters unfortunately causes a copy of the original command line arguments. Not sure how important this factor is compared to spinning up a compiler instance, just wanted to point it out.

clangd/ClangdUnit.cpp

433

As mentioned in the change description, we also have a different compiler invocation in BackgroundIndex which is not aware of ClangdServer::getCompileCommand

[kadircet]

• Address comments

[ilya-biryukov]

clangd/ClangdUnit.cpp

429

My approximation would be half an hour to finish this and land in clangd and 2-3 hours to land this in tooling, update clangd, clang-tidy, etc.
Not a big difference, though, up to Kadir to actually quantify how hard he thinks this would be.

[klimek]

In D56841#1361395, @kadircet wrote:

It seems like the most relevant place in tooling is ArgumentsAdjuster as @ioeric pointed out. Happy to move it to there if @sammccall and @klimek agrees as well.

As a side note ArgumentsAdjusters unfortunately causes a copy of the original command line arguments. Not sure how important this factor is compared to spinning up a compiler instance, just wanted to point it out.

From my point of view, it looks like this isn't enough to be a huge thing in tooling. That said, if we can find a nice abstraction to pull out, I'm generally in favor :) (I know, I know, I'm not a big help)

[sammccall]

In D56841#1361395, @kadircet wrote:

It seems like the most relevant place in tooling is ArgumentsAdjuster as @ioeric pointed out. Happy to move it to there if @sammccall and @klimek agrees as well.

As a side note ArgumentsAdjusters unfortunately causes a copy of the original command line arguments. Not sure how important this factor is compared to spinning up a compiler instance, just wanted to point it out.

I do think this should be a standard arguments adjuster available in Tooling.
This is a pattern that's likely to recur (clang-tidy will need the same fix to work with chromium). The fix is small but fiddly enough that it might warrant some improvement over time - I think we want to avoid people cloning their own copies of the fix.
The ArgumentsAdjuster interface is a little unfortunate regarding the copy, but I don't think this is significant in practice.

Once verifying this fixes the issue, we should consider requesting the tooling and clangd patches be cherrypicked to the LLVM-8 branch.
(And possibly do the same for clang-tidy)

[kadircet]

clang-tidy also has the same fix in place, https://reviews.llvm.org/D18806

[ilya-biryukov]

In D56841#1361492, @klimek wrote:

(I know, I know, I'm not a big help)

That's fine, mostly wanted to hear your opinion :-)

That said, if we can find a nice abstraction to pull out, I'm generally in favor :)

Eric suggested putting this into lib/Tooling/ArgumentsAdjusters.cpp, looks like a proper place for this kind of thing.

As a side note ArgumentsAdjusters unfortunately causes a copy of the original command line arguments. Not sure how important this factor is compared to spinning up a compiler instance, just wanted to point it out.

Yeah, running the frontend later would definitely outweigh this inefficiency, so we don't care.
I do agree that's an unfortunate design, though.

clangd/ClangdUnit.cpp

433

Ah, right :-( That's annoying. Does it also manually override -resource-dir? I'd move both into a single place, maybe this function is a better place to do so.

[kadircet]

Moving stripper to tooling.

clangd/ClangdUnit.cpp

433

It turned out to be more complicated actually, for example CodeCompletion also invokes a frontend action and it doesn't go through this function.....

[kadircet]

• Use getStripPluginsAdjuster and move manipulations into OverlayCDB

[ilya-biryukov]

just a few nits

clangd/GlobalCompilationDatabase.cpp

23 ↗

(On Diff #182479)

naming NIT: use adjustArguments

24 ↗

(On Diff #182479)

NIT: use StringRef

136 ↗

(On Diff #182479)

NIT: avoid an unnecessary copy: ResourceDir ? std::move(*ResourceDir) : …

[kadircet]

• Address comments

[kadircet]

clangd/GlobalCompilationDatabase.cpp

24 ↗

(On Diff #182479)

Need a string for the concatenation below, when pushing to vector. Otherwise we get a Twine

[ilya-biryukov]

clangd/GlobalCompilationDatabase.cpp

24 ↗

(On Diff #182479)

Why not call .str() on a Twine?

[kadircet]

Address comments

clangd/GlobalCompilationDatabase.cpp

24 ↗

(On Diff #182479)

Sure it is also an option but didn't think it would look any better, but there you go.

[ilya-biryukov]

LGTM

clangd/GlobalCompilationDatabase.cpp

24 ↗

(On Diff #182479)

Looks kinda ugly, but so efficient... (Not that we care much here)

clangd/ClangdUnit.cpp

: Preamble(std::move(Preamble)), Clang(std::move(Clang)),
       Includes(std::move(Includes)) {
   assert(this->Clang);
   assert(this->Action);
 }
 
 std::unique_ptr<CompilerInvocation>
 buildCompilerInvocation(const ParseInputs &Inputs) {
   std::vector<const char *> ArgStrs;
-  for (const auto &S : Inputs.CompileCommand.CommandLine)
-    ArgStrs.push_back(S.c_str());
+  const auto &CommandLine = Inputs.CompileCommand.CommandLine;
+  for (size_t I = 0, E = CommandLine.size(); I != E; I++) {
+    // According to https://clang.llvm.org/docs/ClangPlugins.html
+    // -Xclang {-load, -plugin, -plugin-arg-<plugin-name>, -add-plugin} -Xclang
+    // <arbitrary-argument>
+    if (I + 4 < E && CommandLine[I] == "-Xclang" &&
+        (CommandLine[I + 1] == "-load" || CommandLine[I + 1] == "-plugin" ||
+         llvm::StringRef(CommandLine[I + 1]).startswith("-plugin-arg-") ||
+         CommandLine[I + 1] == "-add-plugin") &&
+        CommandLine[I + 2] == "-Xclang") {
+      I += 3;
+      continue;
+    }
+    ArgStrs.push_back(CommandLine[I].c_str());
+  }
 
   if (Inputs.FS->setCurrentWorkingDirectory(Inputs.CompileCommand.Directory)) {
     log("Couldn't set working directory when creating compiler invocation.");
     // We proceed anyway, our lit-tests rely on results for non-existing working
     // dirs.
   }
 
   // FIXME(ibiryukov): store diagnostics from CommandLine when we start

unittests/clangd/ClangdTests.cpp

auto Completions = cantFail(runCodeComplete(Server, SourcePath, Code.point(),
                                               clangd::CodeCompleteOptions()))
                          .Completions;
   EXPECT_EQ(CountStats["foo.h"], Before);
   EXPECT_THAT(Completions,
               ElementsAre(Field(&CodeCompletion::Name, "TestSym")));
 }
 #endif
 
+TEST_F(ClangdVFSTest, FlagsWithPlugins) {
+  MockFSProvider FS;
+  ErrorCheckingDiagConsumer DiagConsumer;
+  MockCompilationDatabase CDB;
+  CDB.ExtraClangFlags = {
+      "-Xclang",
+      "-add-plugin",
+      "-Xclang",
+      "random-plugin",
+  };
+  ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+
+  auto FooCpp = testPath("foo.cpp");
+  const auto SourceContents = "int main() { return 0; }";
+  FS.Files[FooCpp] = FooCpp;
+  Server.addDocument(FooCpp, SourceContents);
+  auto Result = dumpASTWithoutMemoryLocs(Server, FooCpp);
+  EXPECT_TRUE(Server.blockUntilIdleForTest()) << "Waiting for diagnostics";
+  EXPECT_NE(Result, "<no-ast>");
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang