[clangd] Canonicalize compile flags before applying edits

[kadircet]

Summary

Pushes input for the compile action to the end while separating with a
-- before applying other manglings. This ensures edits that effect only the
arguments that come after them works, like changing parse language via -x.

Fixes https://github.com/clangd/clangd/issues/555.

[sammccall]

clang-tools-extra/clangd/CompileCommands.cpp

213

More than one input is a really annoying error to diagnose, would we want to detect and log it here? Downside is this code runs *really* often.

232–233

These are copies of the rendered arg list, i think many of them we could replace with calls to ArgList.erase before rendering the arg list.
(e.g. syntax-only... if we erased -save-temps and turned off color diagnostics in Compiler.cpp, I think that covers everything)

... but wait, is "rendering the arglist" even possible?

(I want to expand the scope of this patch unneccesarily, but it might be a way to offset the extra costs...)

239

Technically the hasArgNoClaim is stale, we've applied edits to the command since then. Probably fine though, not much use for setting resource-dir. (maybe we should even drop this check)

hasArgNoClaim is probably cheaper than the old scan we were doing, but its impl is crazy: not a simple scan, but also not a simple lookup.

243

OTOH the edits may well set sysroot or isysroot, i'm not sure this is valid :-(

248–249

while here and worrying about efficiency - should we just search for '--' ourselves?
This is an extra copy (due to the ArgsAdjuster interface)

[sammccall]

clang-tools-extra/clangd/CompileCommands.cpp

213

Another option would be to delete all the OPT_INPUTs and append the actual filename (CompileCommand::Filename)

This seems like it would define this class of errors out of existence, maybe?

Layering doesn't make it easy to do that right here, though :-\

[kadircet]

• Rebase

[kadircet]

clang-tools-extra/clangd/CompileCommands.cpp

213

I agree that it is gonna be somewhat noisy. But I think it is gonna effect 2 different workflows:

• User hits a file with compile flags containing multiple inputs, notices clangd doesn't work, tries to resolve the problem and looks at the logs. since they are not working on the file it is not gonna be noisy in this case and really helpful.
• They are just going to ignore clangd being broken and move on, In this case we'll really turn the logs into garbage but I don't think user will care.

So maybe we can make it vlog instead, and ensure it only shows up to users that care?

213

yes this sounds great indeed, send out as a separate follow-up in D106639.

232–233

send out D106562 to get rid of these.

239

keeping the Has behaviour just to minimize the impact, happy to drop the check as a follow-up.

[kadircet]

• Address review comments

[sammccall]

Still LG

clang-tools-extra/clangd/CompileCommands.cpp

248–249

hmm, why isn't this Cmd.insert(It, ToAppend.begin(), ToAppend.end()) instead of the loop, to avoid shuffling the tail elements?

Or in fact Cmd.insert(It, std::make_move_iterator(ToAppend.begin()), std::make_move_iterator(ToAppend.end()));?

255

you reverted the others to Has but keps the ArgList reference here.

[kadircet]

Change occurence of ArgList usage for canonical prefixes to Has

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/Options.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Tooling/ArgumentsAdjusters.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 std::string &S : Cmd)
+    OriginalArgs.push_back(S.c_str());
+  // 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);
+
+  // 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;
     }
     return false;
   };
 
   llvm::erase_if(Cmd, [](llvm::StringRef Elem) {
     return Elem.startswith("--save-temps") || Elem.startswith("-save-temps");
   });
 
   std::vector<std::string> ToAppend;
   if (ResourceDir && !Has("-resource-dir"))
     ToAppend.push_back(("-resource-dir=" + *ResourceDir));
 
   // Don't set `-isysroot` if it is already set or if `--sysroot` is set.
   // `--sysroot` is a superset of the `-isysroot` argument.
-  if (Sysroot && !Has("-isysroot") && !Has("--sysroot")) {
+  if (Sysroot && !Has("--sysroot") && !Has("-isysroot")) {
     ToAppend.push_back("-isysroot");
     ToAppend.push_back(*Sysroot);
   }
 
   if (!ToAppend.empty()) {
     Cmd.insert(llvm::find(Cmd, "--"), std::make_move_iterator(ToAppend.begin()),
                std::make_move_iterator(ToAppend.end()));
   }
 
   if (!Cmd.empty()) {
     bool FollowSymlink = !Has("-no-canonical-prefixes");
     Cmd.front() =
         (FollowSymlink ? ResolvedDrivers : ResolvedDriversNoFollow)
             .get(Cmd.front(), [&, this] {
               return resolveDriver(Cmd.front(), FollowSymlink, ClangPath);
             });
   }
 }
 
 CommandMangler::operator clang::tooling::ArgumentsAdjuster() && {

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/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(ArgStripperTest, OrderDependent) {
   S.strip("-include-pch");
   std::vector<std::string> Args = {"clang", "-include-pch", "foo.pch",
                                    "foo.cc"};
   S.process(Args);
   EXPECT_THAT(Args, ElementsAre("clang", "foo.cc"));
 }
 
 TEST(PrintArgvTest, All) {
-  std::vector<llvm::StringRef> Args = {
-      "one", "two", "thr ee", "f\"o\"ur", "fi\\ve", "$"
-  };
+  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));
 }
 
 } // namespace
 } // namespace clangd
 } // namespace clang