Revert "Revert "[clangd] Canonicalize compile flags before applying edits""

[kadircet]

Summary

Set driver mode before parsing arglist.

Depends on D106789.

[sammccall]

clang-tools-extra/clangd/CompileCommands.cpp

209

makeArrayRef(OriginalArgs).slice(1)?

214

include should be IsCLMode ? (CLOption | CoreOption) : /*everything*/0?

215

exclude NoDriverOption | ( IsCLMode ? 0 : CLOption)

clang-tools-extra/clangd/CompileCommands.cpp

 //===--- CompileCommands.cpp ----------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "CompileCommands.h"
 #include "Config.h"
 #include "support/Logger.h"
+#include "support/Trace.h"
+#include "clang/Driver/Driver.h"
 #include "clang/Driver/Options.h"
+#include "clang/Driver/ToolChain.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Tooling/ArgumentsAdjusters.h"
+#include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Option/ArgList.h"
 #include "llvm/Option/Option.h"
 #include "llvm/Support/Allocator.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/FileUtilities.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Program.h"

 CommandMangler CommandMangler::detect() {
   CommandMangler Result;
   Result.ClangPath = detectClangPath();
   Result.ResourceDir = detectStandardResourceDir();
   Result.Sysroot = detectSysroot();
   return Result;
 }
 
-CommandMangler CommandMangler::forTests() {
-  return CommandMangler();
-}
+CommandMangler CommandMangler::forTests() { return CommandMangler(); }
 
 void CommandMangler::adjust(std::vector<std::string> &Cmd) const {
+  trace::Span S("AdjustCompileFlags");
+  auto &OptTable = clang::driver::getDriverOptTable();
+  // OriginalArgs needs to outlive ArgList.
+  llvm::SmallVector<const char *, 16> OriginalArgs;
+  OriginalArgs.reserve(Cmd.size());
+  for (const auto &S : Cmd)
+    OriginalArgs.push_back(S.c_str());
+  bool IsCLMode =
+      !OriginalArgs.empty() &&
+      driver::IsClangCL(driver::getDriverMode(
+          OriginalArgs[0], llvm::makeArrayRef(OriginalArgs).slice(1)));
+  // ParseArgs propagates missig arg/opt counts on error, but preserves
+  // everything it could parse in ArgList. So we just ignore those counts.
+  unsigned IgnoredCount;
+  auto ArgList = OptTable.ParseArgs(
+      OriginalArgs, IgnoredCount, IgnoredCount,
+      /*FlagsToInclude=*/
+      IsCLMode ? (driver::options::CLOption | driver::options::CoreOption)
+               : /*everything*/ 0,
+      /*FlagsToExclude=*/driver::options::NoDriverOption |
+          (IsCLMode ? 0 : driver::options::CLOption));
+
+  // Move the inputs to the end, separated via `--` from flags. This ensures
+  // modifications done in the following steps apply in more cases (like setting
+  // -x, which only affects inputs that come after it).
+  if (!ArgList.hasArgNoClaim(driver::options::OPT__DASH_DASH)) {
+    // In theory there might be more than one input, but clangd can't deal with
+    // them anyway.
+    if (auto *Input = ArgList.getLastArg(driver::options::OPT_INPUT)) {
+      Cmd.insert(Cmd.end(), {"--", Input->getAsString(ArgList)});
+      Cmd.erase(Cmd.begin() + Input->getIndex());
+    }
+  }
+
   for (auto &Edit : Config::current().CompileFlags.Edits)
     Edit(Cmd);
 
   // Check whether the flag exists, either as -flag or -flag=*
   auto Has = [&](llvm::StringRef Flag) {
     for (llvm::StringRef Arg : Cmd) {
       if (Arg.consume_front(Flag) && (Arg.empty() || Arg[0] == '='))
         return true;

clang-tools-extra/clangd/test/did-change-configuration-params.test

 # CHECK-NEXT:      }
 # CHECK-NEXT:    ],
 # CHECK-NEXT:    "uri": "file://{{.*}}/foo.c",
 # CHECK-NEXT:    "version": 0
 # CHECK-NEXT:  }
 #
 # ERR: ASTWorker building file {{.*}}foo.c version 0 with command
 # ERR: [{{.*}}clangd-test2]
-# ERR: clang -c foo.c -Wall -Werror
+# ERR: clang -c -Wall -Werror {{.*}} -- foo.c
 ---
 {"jsonrpc":"2.0","id":5,"method":"shutdown"}
 ---
 {"jsonrpc":"2.0","method":"exit"}
 
 

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

TEST_F(BackgroundIndexTest, Config) {
   }
   // Context provider that installs a configuration mutating foo's command.
   // This causes it to define foo::two instead of foo::one.
   // It also disables indexing of baz entirely.
   BackgroundIndex::Options Opts;
   Opts.ContextProvider = [](PathRef P) {
     Config C;
     if (P.endswith("foo.cpp"))
-      C.CompileFlags.Edits.push_back(
-          [](std::vector<std::string> &Argv) { Argv.push_back("-Done=two"); });
+      C.CompileFlags.Edits.push_back([](std::vector<std::string> &Argv) {
+        Argv = tooling::getInsertArgumentAdjuster("-Done=two")(Argv, "");
+      });
     if (P.endswith("baz.cpp"))
       C.Index.Background = Config::BackgroundPolicy::Skip;
     return Context::current().derive(Config::Key, std::move(C));
   };
   // Create the background index.
   llvm::StringMap<std::string> Storage;
   size_t CacheHits = 0;
   MemoryShardStorage MSS(Storage, CacheHits);

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

 //===-- CompileCommandsTests.cpp ------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "CompileCommands.h"
 #include "Config.h"
 #include "TestFS.h"
 #include "support/Context.h"
 
+#include "clang/Tooling/ArgumentsAdjusters.h"
+#include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Process.h"
 
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
TEST(CommandMangler, ConfigEdits) {
   std::vector<std::string> Cmd = {"clang++", "foo.cc"};
   {
     Config Cfg;
     Cfg.CompileFlags.Edits.push_back([](std::vector<std::string> &Argv) {
       for (auto &Arg : Argv)
         for (char &C : Arg)
           C = llvm::toUpper(C);
     });
-    Cfg.CompileFlags.Edits.push_back(
-        [](std::vector<std::string> &Argv) { Argv.push_back("--hello"); });
+    Cfg.CompileFlags.Edits.push_back([](std::vector<std::string> &Argv) {
+      Argv = tooling::getInsertArgumentAdjuster("--hello")(Argv, "");
+    });
     WithContextValue WithConfig(Config::Key, std::move(Cfg));
     Mangler.adjust(Cmd);
   }
-  // Edits are applied in given order and before other mangling.
-  EXPECT_THAT(Cmd, ElementsAre(_, "FOO.CC", "--hello"));
+  // Edits are applied in given order and before other mangling and they always
+  // go before filename.
+  EXPECT_THAT(Cmd, ElementsAre(_, "--hello", "--", "FOO.CC"));
 }
 
 static std::string strip(llvm::StringRef Arg, llvm::StringRef Argv) {
   llvm::SmallVector<llvm::StringRef> Parts;
   llvm::SplitString(Argv, Parts);
   std::vector<std::string> Args = {Parts.begin(), Parts.end()};
   ArgStripper S;
   S.strip(Arg);

 TEST(PrintArgvTest, All) {
   std::vector<llvm::StringRef> Args = {
       "one", "two", "thr ee", "f\"o\"ur", "fi\\ve", "$"
   };
   const char *Expected = R"(one two "thr ee" "f\"o\"ur" "fi\\ve" $)";
   EXPECT_EQ(Expected, printArgv(Args));
 }
 
+TEST(CommandMangler, InputsAfterDashDash) {
+  const auto Mangler = CommandMangler::forTests();
+  {
+    std::vector<std::string> Args = {"clang", "/Users/foo.cc"};
+    Mangler.adjust(Args);
+    EXPECT_THAT(llvm::makeArrayRef(Args).take_back(2),
+                ElementsAre("--", "/Users/foo.cc"));
+    EXPECT_THAT(llvm::makeArrayRef(Args).drop_back(2),
+                Not(Contains("/Users/foo.cc")));
+  }
+  // In CL mode /U triggers an undef operation, hence `/Users/foo.cc` shouldn't
+  // be interpreted as a file.
+  {
+    std::vector<std::string> Args = {"clang", "--driver-mode=cl", "bar.cc",
+                                     "/Users/foo.cc"};
+    Mangler.adjust(Args);
+    EXPECT_THAT(llvm::makeArrayRef(Args).take_back(2),
+                ElementsAre("--", "bar.cc"));
+    EXPECT_THAT(llvm::makeArrayRef(Args).drop_back(2), Not(Contains("bar.cc")));
+  }
+}
 } // namespace
 } // namespace clangd
 } // namespace clang