[clangd] Adjust compile commands to be applicable for tooling

[kadircet]

Summary

As can be seen in https://github.com/llvm-mirror/clang/blob/master/lib/Tooling/Tooling.cpp#L385
clang tool invocations adjust commands normally like this. In clangd we have
different code paths for invoking a frontend action(preamble builds, ast builds,
background index, clangd-indexer) they all work on the same GlobalCompilationDatabase
abstraction, but later on are subject to different modifications.

This patch makes sure all of the clangd actions make use of the same compile
commands before invocation.

Enables background-index to work on chromium codebase(since they had dependency
file output in their compile commands).

[kadircet]

• Update the diff, sorry for the confusion

[ioeric]

clangd/GlobalCompilationDatabase.h

116 ↗

(On Diff #189682)

This doesn't seem to be used in this patch (except for tests). Could you include intended uses in the patch so we can understand the problem better?

[hokein]

clangd/GlobalCompilationDatabase.h

116 ↗

(On Diff #189682)

Looks like this patch introduces two changes:

• move the internal adjustArguments to public, I have the same question, any reason doing it? because we can test it?
• add additional Adjusters to adjustArguments

[kadircet]

clangd/GlobalCompilationDatabase.h

116 ↗

(On Diff #189682)

It was only for testing

[kadircet]

• Change testing strategy

[gribozavr]

clangd/GlobalCompilationDatabase.cpp

24

Please don't duplicate code in comments. Explain why it shouldn't generate the dependency files, or drop the comment.

clangd/GlobalCompilationDatabase.h

116 ↗

(On Diff #189682)

This patch also changes getCompileCommand to call adjustArguments.

[ioeric]

clangd/GlobalCompilationDatabase.cpp

25

Please use clang::tooling::combineAdjusters.

[kadircet]

clangd/GlobalCompilationDatabase.cpp

24

I am saying we are stripping "dependecy file related" commands, because clangd is not producing them.
I feel like this provides the cause of the following statement, not replicates it. Becuase the following statement just says strip "dependency file related" commands.

But happy to drop the comment if you insist.

[gribozavr]

clangd/GlobalCompilationDatabase.cpp

24

What about "clangd should not write files to disk, including dependency files requested on the command line."

[kadircet]

• Address comments

clangd/GlobalCompilationDatabase.cpp

 #include "llvm/Support/Path.h"
 
 namespace clang {
 namespace clangd {
 namespace {
 
 void adjustArguments(tooling::CompileCommand &Cmd,
                      llvm::StringRef ResourceDir) {
+  tooling::ArgumentsAdjuster ArgsAdjuster = tooling::combineAdjusters(
+      // clangd should not write files to disk, including dependency files
+      // requested on the command line.
+      tooling::getClangStripDependencyFileAdjuster(),
       // Strip plugin related command line arguments. Clangd does
       // not support plugins currently. Therefore it breaks if
       // compiler tries to load plugins.
-  Cmd.CommandLine =
-      tooling::getStripPluginsAdjuster()(Cmd.CommandLine, Cmd.Filename);
+      tooling::combineAdjusters(tooling::getStripPluginsAdjuster(),
+                                tooling::getClangSyntaxOnlyAdjuster()));
+
+  Cmd.CommandLine = ArgsAdjuster(Cmd.CommandLine, Cmd.Filename);
   // Inject the resource dir.
   // FIXME: Don't overwrite it if it's already there.
   if (!ResourceDir.empty())
     Cmd.CommandLine.push_back(("-resource-dir=" + ResourceDir).str());
 }
 
 std::string getStandardResourceDir() {
   static int Dummy; // Just an address in this process.

unittests/clangd/GlobalCompilationDatabaseTests.cpp

 #include "TestFS.h"
 #include "llvm/ADT/StringExtras.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace clangd {
 namespace {
+using ::testing::AllOf;
+using ::testing::Contains;
 using ::testing::ElementsAre;
 using ::testing::EndsWith;
+using ::testing::Not;
 
 TEST(GlobalCompilationDatabaseTest, FallbackCommand) {
   DirectoryBasedGlobalCompilationDatabase DB(None);
   auto Cmd = DB.getFallbackCommand(testPath("foo/bar.cc"));
   EXPECT_EQ(Cmd.Directory, testPath("foo"));
-  EXPECT_THAT(Cmd.CommandLine, ElementsAre(
-    EndsWith("clang"), testPath("foo/bar.cc")));
+  EXPECT_THAT(Cmd.CommandLine,
+              ElementsAre(EndsWith("clang"), testPath("foo/bar.cc")));
   EXPECT_EQ(Cmd.Output, "");
 
   // .h files have unknown language, so they are parsed liberally as obj-c++.
   Cmd = DB.getFallbackCommand(testPath("foo/bar.h"));
   EXPECT_THAT(Cmd.CommandLine,
               ElementsAre(EndsWith("clang"), "-xobjective-c++-header",
                           testPath("foo/bar.h")));
 }

 
 protected:
   OverlayCDBTest() : Base(llvm::make_unique<BaseCDB>()) {}
   std::unique_ptr<GlobalCompilationDatabase> Base;
 };
 
 TEST_F(OverlayCDBTest, GetCompileCommand) {
   OverlayCDB CDB(Base.get(), {}, std::string(""));
-  EXPECT_EQ(CDB.getCompileCommand(testPath("foo.cc")),
-            Base->getCompileCommand(testPath("foo.cc")));
+  EXPECT_THAT(CDB.getCompileCommand(testPath("foo.cc"))->CommandLine,
+              AllOf(Contains(testPath("foo.cc")), Contains("-DA=1")));
   EXPECT_EQ(CDB.getCompileCommand(testPath("missing.cc")), llvm::None);
 
   auto Override = cmd(testPath("foo.cc"), "-DA=3");
   CDB.setCompileCommand(testPath("foo.cc"), Override);
-  EXPECT_EQ(CDB.getCompileCommand(testPath("foo.cc")), Override);
+  EXPECT_THAT(CDB.getCompileCommand(testPath("foo.cc"))->CommandLine,
+              Contains("-DA=3"));
   EXPECT_EQ(CDB.getCompileCommand(testPath("missing.cc")), llvm::None);
   CDB.setCompileCommand(testPath("missing.cc"), Override);
-  EXPECT_EQ(CDB.getCompileCommand(testPath("missing.cc")), Override);
+  EXPECT_THAT(CDB.getCompileCommand(testPath("missing.cc"))->CommandLine,
+              Contains("-DA=3"));
 }
 
 TEST_F(OverlayCDBTest, GetFallbackCommand) {
   OverlayCDB CDB(Base.get(), {"-DA=4"});
   EXPECT_THAT(CDB.getFallbackCommand(testPath("bar.cc")).CommandLine,
               ElementsAre("clang", "-DA=2", testPath("bar.cc"), "-DA=4"));
 }
 
 TEST_F(OverlayCDBTest, NoBase) {
   OverlayCDB CDB(nullptr, {"-DA=6"}, std::string(""));
   EXPECT_EQ(CDB.getCompileCommand(testPath("bar.cc")), None);
   auto Override = cmd(testPath("bar.cc"), "-DA=5");
   CDB.setCompileCommand(testPath("bar.cc"), Override);
-  EXPECT_EQ(CDB.getCompileCommand(testPath("bar.cc")), Override);
+  EXPECT_THAT(CDB.getCompileCommand(testPath("bar.cc"))->CommandLine,
+              Contains("-DA=5"));
 
   EXPECT_THAT(CDB.getFallbackCommand(testPath("foo.cc")).CommandLine,
               ElementsAre(EndsWith("clang"), testPath("foo.cc"), "-DA=6"));
 }
 
 TEST_F(OverlayCDBTest, Watch) {
   OverlayCDB Inner(nullptr);
   OverlayCDB Outer(&Inner);
 
   std::vector<std::vector<std::string>> Changes;
   auto Sub = Outer.watch([&](const std::vector<std::string> &ChangedFiles) {
     Changes.push_back(ChangedFiles);
   });
 
   Inner.setCompileCommand("A.cpp", tooling::CompileCommand());
   Outer.setCompileCommand("B.cpp", tooling::CompileCommand());
   Inner.setCompileCommand("A.cpp", llvm::None);
   Outer.setCompileCommand("C.cpp", llvm::None);
   EXPECT_THAT(Changes, ElementsAre(ElementsAre("A.cpp"), ElementsAre("B.cpp"),
                                    ElementsAre("A.cpp"), ElementsAre("C.cpp")));
 }
 
+TEST_F(OverlayCDBTest, Adjustments) {
+  OverlayCDB CDB(Base.get(), {}, std::string(""));
+  auto Cmd = CDB.getCompileCommand(testPath("foo.cc")).getValue();
+  // Delete the file name.
+  Cmd.CommandLine.pop_back();
+
+  // Check dependency file commands are dropped.
+  Cmd.CommandLine.push_back("-MF");
+  Cmd.CommandLine.push_back("random-dependency");
+
+  // Check plugin-related commands are dropped.
+  Cmd.CommandLine.push_back("-Xclang");
+  Cmd.CommandLine.push_back("-load");
+  Cmd.CommandLine.push_back("-Xclang");
+  Cmd.CommandLine.push_back("random-plugin");
+
+  Cmd.CommandLine.push_back("-DA=5");
+  Cmd.CommandLine.push_back(Cmd.Filename);
+
+  CDB.setCompileCommand(testPath("foo.cc"), Cmd);
+
+  EXPECT_THAT(CDB.getCompileCommand(testPath("foo.cc"))->CommandLine,
+              AllOf(Contains("-fsyntax-only"), Contains("-DA=5"),
+                    Contains(testPath("foo.cc")), Not(Contains("-MF")),
+                    Not(Contains("random-dependency")),
+                    Not(Contains("-Xclang")), Not(Contains("-load")),
+                    Not(Contains("random-plugin"))));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang