[clangd] Move standard options adaptor to CommandMangler

There is a discrepancy between how clangd processes CDB loaded from
JSON file on disk and pushed via LSP. Thus the same CDB pushed via
LSP protocol may not work as expected. Some difference between these two
paths is expected but we still need to insert driver mode and target from
binary name and expand response files.

Test Plan: check-clang-tools

Differential Revision: https://reviews.llvm.org/D143436

clang-tools-extra/clangd/CompileCommands.cpp

@@ -14,6 +14,7 @@
 #include "clang/Driver/Options.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Tooling/CompilationDatabase.h"
+#include "clang/Tooling/Tooling.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallVector.h"
@@ -27,6 +28,7 @@
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Program.h"
+#include "llvm/TargetParser/Host.h"
 #include <iterator>
 #include <optional>
 #include <string>
@@ -185,6 +187,12 @@ static std::string resolveDriver(llvm::StringRef Driver, bool FollowSymlink,
 
 } // namespace
 
+CommandMangler::CommandMangler() {
+  Tokenizer = llvm::Triple(llvm::sys::getProcessTriple()).isOSWindows()
+                  ? llvm::cl::TokenizeWindowsCommandLine
+                  : llvm::cl::TokenizeGNUCommandLine;
+}
+
 CommandMangler CommandMangler::detect() {
   CommandMangler Result;
   Result.ClangPath = detectClangPath();
@@ -201,9 +209,18 @@ void CommandMangler::operator()(tooling::CompileCommand &Command,
   trace::Span S("AdjustCompileFlags");
   // Most of the modifications below assumes the Cmd starts with a driver name.
   // We might consider injecting a generic driver name like "cc" or "c++", but
-  // a Cmd missing the driver is probably rare enough in practice and errnous.
+  // a Cmd missing the driver is probably rare enough in practice and erroneous.
   if (Cmd.empty())
     return;
+
+  // FS used for expanding response files.
+  // FIXME: ExpandResponseFiles appears not to provide the usual
+  // thread-safety guarantees, as the access to FS is not locked!
+  // For now, use the real FS, which is known to be threadsafe (if we don't
+  // use/change working directory, which ExpandResponseFiles doesn't).
+  auto FS = llvm::vfs::getRealFileSystem();
+  tooling::addExpandedResponseFiles(Cmd, Command.Directory, Tokenizer, *FS);
+
   auto &OptTable = clang::driver::getDriverOptTable();
   // OriginalArgs needs to outlive ArgList.
   llvm::SmallVector<const char *, 16> OriginalArgs;
@@ -212,7 +229,7 @@ void CommandMangler::operator()(tooling::CompileCommand &Command,
     OriginalArgs.push_back(S.c_str());
   bool IsCLMode = driver::IsClangCL(driver::getDriverMode(
       OriginalArgs[0], llvm::ArrayRef(OriginalArgs).slice(1)));
-  // ParseArgs propagates missig arg/opt counts on error, but preserves
+  // ParseArgs propagates missing arg/opt counts on error, but preserves
   // everything it could parse in ArgList. So we just ignore those counts.
   unsigned IgnoredCount;
   // Drop the executable name, as ParseArgs doesn't expect it. This means
@@ -307,12 +324,16 @@ void CommandMangler::operator()(tooling::CompileCommand &Command,
   //    necessary for the system include extractor to identify the file type
   //  - AFTER applying CompileFlags.Edits, because the name of the compiler
   //    that needs to be invoked may come from the CompileFlags->Compiler key
+  //  - BEFORE addTargetAndModeForProgramName(), because gcc doesn't support
+  //    the target flag that might be added.
   //  - BEFORE resolveDriver() because that can mess up the driver path,
   //    e.g. changing gcc to /path/to/clang/bin/gcc
   if (SystemIncludeExtractor) {
     SystemIncludeExtractor(Command, File);
   }
 
+  tooling::addTargetAndModeForProgramName(Cmd, Cmd.front());
+
   // Check whether the flag exists, either as -flag or -flag=*
   auto Has = [&](llvm::StringRef Flag) {
     for (llvm::StringRef Arg : Cmd) {

clang-tools-extra/clangd/CompileCommands.h

@@ -12,6 +12,7 @@
 #include "support/Threading.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/CommandLine.h"
 #include <deque>
 #include <optional>
 #include <string>
@@ -50,10 +51,11 @@ struct CommandMangler {
                   llvm::StringRef TargetFile) const;
 
 private:
-  CommandMangler() = default;
+  CommandMangler();
 
   Memoize<llvm::StringMap<std::string>> ResolvedDrivers;
   Memoize<llvm::StringMap<std::string>> ResolvedDriversNoFollow;
+  llvm::cl::TokenizerCallback Tokenizer;
 };
 
 // Removes args from a command-line in a semantically-aware way.

clang-tools-extra/clangd/GlobalCompilationDatabase.cpp

@@ -244,15 +244,7 @@ static std::unique_ptr<tooling::CompilationDatabase>
 parseJSON(PathRef Path, llvm::StringRef Data, std::string &Error) {
   if (auto CDB = tooling::JSONCompilationDatabase::loadFromBuffer(
           Data, Error, tooling::JSONCommandLineSyntax::AutoDetect)) {
-    // FS used for expanding response files.
-    // FIXME: ExpandResponseFilesDatabase appears not to provide the usual
-    // thread-safety guarantees, as the access to FS is not locked!
-    // For now, use the real FS, which is known to be threadsafe (if we don't
-    // use/change working directory, which ExpandResponseFilesDatabase doesn't).
-    auto FS = llvm::vfs::getRealFileSystem();
-    return tooling::inferTargetAndDriverMode(
-        tooling::inferMissingCompileCommands(
-            expandResponseFiles(std::move(CDB), std::move(FS))));
+    return tooling::inferMissingCompileCommands(std::move(CDB));
   }
   return nullptr;
 }

clang-tools-extra/clangd/unittests/CMakeLists.txt

@@ -157,6 +157,7 @@ clang_target_link_libraries(ClangdTests
   clangLex
   clangSema
   clangSerialization
+  clangTesting
   clangTooling
   clangToolingCore
   clangToolingInclusions

clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp

@@ -11,6 +11,7 @@
 #include "TestFS.h"
 #include "support/Context.h"
 
+#include "clang/Testing/CommandLineArgs.h"
 #include "clang/Tooling/ArgumentsAdjusters.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/STLExtras.h"
@@ -20,6 +21,7 @@
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Process.h"
+#include "llvm/Support/TargetSelect.h"
 
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
@@ -41,15 +43,18 @@ using ::testing::Not;
 // Make use of all features and assert the exact command we get out.
 // Other tests just verify presence/absence of certain args.
 TEST(CommandMangler, Everything) {
+  llvm::InitializeAllTargetInfos(); // As in ClangdMain
+  std::string Target = getAnyTargetForTesting();
   auto Mangler = CommandMangler::forTests();
   Mangler.ClangPath = testPath("fake/clang");
   Mangler.ResourceDir = testPath("fake/resources");
   Mangler.Sysroot = testPath("fake/sysroot");
   tooling::CompileCommand Cmd;
-  Cmd.CommandLine = {"clang++", "--", "foo.cc", "bar.cc"};
+  Cmd.CommandLine = {Target + "-clang++", "--", "foo.cc", "bar.cc"};
   Mangler(Cmd, "foo.cc");
   EXPECT_THAT(Cmd.CommandLine,
-              ElementsAre(testPath("fake/clang++"),
+              ElementsAre(testPath("fake/" + Target + "-clang++"),
+                          "--target=" + Target, "--driver-mode=g++",
                           "-resource-dir=" + testPath("fake/resources"),
                           "-isysroot", testPath("fake/sysroot"), "--",
                           "foo.cc"));
@@ -197,8 +202,26 @@ TEST(CommandMangler, ConfigEdits) {
     Mangler(Cmd, "foo.cc");
   }
   // Edits are applied in given order and before other mangling and they always
-  // go before filename.
-  EXPECT_THAT(Cmd.CommandLine, ElementsAre(_, "--hello", "--", "FOO.CC"));
+  // go before filename. `--driver-mode=g++` here is in lower case because
+  // options inserted by addTargetAndModeForProgramName are not editable,
+  // see discussion in https://reviews.llvm.org/D138546
+  EXPECT_THAT(Cmd.CommandLine,
+              ElementsAre(_, "--driver-mode=g++", "--hello", "--", "FOO.CC"));
+}
+
+TEST(CommandMangler, ExpandedResponseFiles) {
+  SmallString<1024> Path;
+  int FD;
+  ASSERT_FALSE(llvm::sys::fs::createTemporaryFile("args", "", FD, Path));
+  llvm::raw_fd_ostream OutStream(FD, true);
+  OutStream << "-Wall";
+  OutStream.close();
+
+  auto Mangler = CommandMangler::forTests();
+  tooling::CompileCommand Cmd;
+  Cmd.CommandLine = {"clang", ("@" + Path).str(), "foo.cc"};
+  Mangler(Cmd, "foo.cc");
+  EXPECT_THAT(Cmd.CommandLine, ElementsAre(_, "-Wall", "--", "foo.cc"));
 }
 
 static std::string strip(llvm::StringRef Arg, llvm::StringRef Argv) {

clang/include/clang/Testing/CommandLineArgs.h

@@ -38,6 +38,11 @@ std::vector<std::string> getCC1ArgsForTesting(TestLanguage Lang);
 
 StringRef getFilenameForTesting(TestLanguage Lang);
 
+/// Find a target name such that looking for it in TargetRegistry by that name
+/// returns the same target. We expect that there is at least one target
+/// configured with this property.
+std::string getAnyTargetForTesting();
+
 } // end namespace clang
 
 #endif

clang/include/clang/Tooling/Tooling.h

@@ -506,6 +506,12 @@ llvm::Expected<std::string> getAbsolutePath(llvm::vfs::FileSystem &FS,
 void addTargetAndModeForProgramName(std::vector<std::string> &CommandLine,
                                     StringRef InvokedAs);
 
+/// Helper function that expands response files in command line.
+void addExpandedResponseFiles(std::vector<std::string> &CommandLine,
+                              llvm::StringRef WorkingDir,
+                              llvm::cl::TokenizerCallback Tokenizer,
+                              llvm::vfs::FileSystem &FS);
+
 /// Creates a \c CompilerInvocation.
 CompilerInvocation *newInvocation(DiagnosticsEngine *Diagnostics,
                                   ArrayRef<const char *> CC1Args,

clang/lib/Testing/CommandLineArgs.cpp

@@ -7,6 +7,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "clang/Testing/CommandLineArgs.h"
+#include "llvm/MC/TargetRegistry.h"
 #include "llvm/Support/ErrorHandling.h"
 
 namespace clang {
@@ -109,4 +110,18 @@ StringRef getFilenameForTesting(TestLanguage Lang) {
   llvm_unreachable("Unhandled TestLanguage enum");
 }
 
+std::string getAnyTargetForTesting() {
+  for (const auto &Target : llvm::TargetRegistry::targets()) {
+    std::string Error;
+    StringRef TargetName(Target.getName());
+    if (TargetName == "x86-64")
+      TargetName = "x86_64";
+    if (llvm::TargetRegistry::lookupTarget(std::string(TargetName), Error) ==
+        &Target) {
+      return std::string(TargetName);
+    }
+  }
+  return "";
+}
+
 } // end namespace clang

clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp

@@ -7,6 +7,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "clang/Tooling/CompilationDatabase.h"
+#include "clang/Tooling/Tooling.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/ConvertUTF.h"
@@ -48,28 +49,9 @@ public:
 
 private:
   std::vector<CompileCommand> expand(std::vector<CompileCommand> Cmds) const {
-    for (auto &Cmd : Cmds) {
-      bool SeenRSPFile = false;
-      llvm::SmallVector<const char *, 20> Argv;
-      Argv.reserve(Cmd.CommandLine.size());
-      for (auto &Arg : Cmd.CommandLine) {
-        Argv.push_back(Arg.c_str());
-        if (!Arg.empty())
-          SeenRSPFile |= Arg.front() == '@';
-      }
-      if (!SeenRSPFile)
-        continue;
-      llvm::BumpPtrAllocator Alloc;
-      llvm::cl::ExpansionContext ECtx(Alloc, Tokenizer);
-      llvm::Error Err = ECtx.setVFS(FS.get())
-                            .setCurrentDir(Cmd.Directory)
-                            .expandResponseFiles(Argv);
-      if (Err)
-        llvm::errs() << Err;
-      // Don't assign directly, Argv aliases CommandLine.
-      std::vector<std::string> ExpandedArgv(Argv.begin(), Argv.end());
-      Cmd.CommandLine = std::move(ExpandedArgv);
-    }
+    for (auto &Cmd : Cmds)
+      tooling::addExpandedResponseFiles(Cmd.CommandLine, Cmd.Directory,
+                                        Tokenizer, *FS);
     return Cmds;
   }
 

clang/lib/Tooling/Tooling.cpp

@@ -43,6 +43,7 @@
 #include "llvm/Option/OptTable.h"
 #include "llvm/Option/Option.h"
 #include "llvm/Support/Casting.h"
+#include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/FileSystem.h"
@@ -299,6 +300,31 @@ void addTargetAndModeForProgramName(std::vector<std::string> &CommandLine,
   }
 }
 
+void addExpandedResponseFiles(std::vector<std::string> &CommandLine,
+                              llvm::StringRef WorkingDir,
+                              llvm::cl::TokenizerCallback Tokenizer,
+                              llvm::vfs::FileSystem &FS) {
+  bool SeenRSPFile = false;
+  llvm::SmallVector<const char *, 20> Argv;
+  Argv.reserve(CommandLine.size());
+  for (auto &Arg : CommandLine) {
+    Argv.push_back(Arg.c_str());
+    if (!Arg.empty())
+      SeenRSPFile |= Arg.front() == '@';
+  }
+  if (!SeenRSPFile)
+    return;
+  llvm::BumpPtrAllocator Alloc;
+  llvm::cl::ExpansionContext ECtx(Alloc, Tokenizer);
+  llvm::Error Err =
+      ECtx.setVFS(&FS).setCurrentDir(WorkingDir).expandResponseFiles(Argv);
+  if (Err)
+    llvm::errs() << Err;
+  // Don't assign directly, Argv aliases CommandLine.
+  std::vector<std::string> ExpandedArgv(Argv.begin(), Argv.end());
+  CommandLine = std::move(ExpandedArgv);
+}
+
 } // namespace tooling
 } // namespace clang
 
@@ -684,7 +710,7 @@ std::unique_ptr<ASTUnit> buildASTFromCodeWithArgs(
 
   if (!Invocation.run())
     return nullptr;
- 
+
   assert(ASTs.size() == 1);
   return std::move(ASTs[0]);
 }

clang/unittests/Tooling/ToolingTest.cpp

@@ -17,11 +17,11 @@
 #include "clang/Frontend/FrontendAction.h"
 #include "clang/Frontend/FrontendActions.h"
 #include "clang/Frontend/TextDiagnosticBuffer.h"
+#include "clang/Testing/CommandLineArgs.h"
 #include "clang/Tooling/ArgumentsAdjusters.h"
 #include "clang/Tooling/CompilationDatabase.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringRef.h"
-#include "llvm/MC/TargetRegistry.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/TargetSelect.h"
 #include "llvm/TargetParser/Host.h"
@@ -871,28 +871,10 @@ TEST(ClangToolTest, StripPluginsAdjuster) {
   EXPECT_FALSE(HasFlag("-random-plugin"));
 }
 
-namespace {
-/// Find a target name such that looking for it in TargetRegistry by that name
-/// returns the same target. We expect that there is at least one target
-/// configured with this property.
-std::string getAnyTarget() {
-  llvm::InitializeAllTargets();
-  for (const auto &Target : llvm::TargetRegistry::targets()) {
-    std::string Error;
-    StringRef TargetName(Target.getName());
-    if (TargetName == "x86-64")
-      TargetName = "x86_64";
-    if (llvm::TargetRegistry::lookupTarget(std::string(TargetName), Error) ==
-        &Target) {
-      return std::string(TargetName);
-    }
-  }
-  return "";
-}
-}
-
 TEST(addTargetAndModeForProgramName, AddsTargetAndMode) {
-  std::string Target = getAnyTarget();
+  llvm::InitializeAllTargets();
+
+  std::string Target = getAnyTargetForTesting();
   ASSERT_FALSE(Target.empty());
 
   std::vector<std::string> Args = {"clang", "-foo"};
@@ -905,7 +887,8 @@ TEST(addTargetAndModeForProgramName, AddsTargetAndMode) {
 }
 
 TEST(addTargetAndModeForProgramName, PathIgnored) {
-  std::string Target = getAnyTarget();
+  llvm::InitializeAllTargets();
+  std::string Target = getAnyTargetForTesting();
   ASSERT_FALSE(Target.empty());
 
   SmallString<32> ToolPath;
@@ -919,7 +902,8 @@ TEST(addTargetAndModeForProgramName, PathIgnored) {
 }
 
 TEST(addTargetAndModeForProgramName, IgnoresExistingTarget) {
-  std::string Target = getAnyTarget();
+  llvm::InitializeAllTargets();
+  std::string Target = getAnyTargetForTesting();
   ASSERT_FALSE(Target.empty());
 
   std::vector<std::string> Args = {"clang", "-foo", "-target", "something"};
@@ -936,7 +920,8 @@ TEST(addTargetAndModeForProgramName, IgnoresExistingTarget) {
 }
 
 TEST(addTargetAndModeForProgramName, IgnoresExistingMode) {
-  std::string Target = getAnyTarget();
+  llvm::InitializeAllTargets();
+  std::string Target = getAnyTargetForTesting();
   ASSERT_FALSE(Target.empty());
 
   std::vector<std::string> Args = {"clang", "-foo", "--driver-mode=abc"};