[clangd] Use command line adjusters for inserting compile flags

[kadircet]

Summary

This fixes issues with -- in the compile flags.

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

[crr0004]

I was able to replicate the issue on Fedora 33 and cause clangd to fail by adding -- before the file to be compiled.

After applying the patch, the issue is fixed. All the unit tests and relevant test past.

I could go on Windows and replicate the issue and apply the fix as I have a llvm build setup there, but not sure if it is worth the time sink.

[sammccall]

Thanks for fixing!

clang-tools-extra/clangd/CompileCommands.cpp

230

This extra copy is really silly :-(
please move(ToAppend) and make the whole thing conditional on !ToAppend.empty() (even though that'll be pretty much always...). Sadly move(Cmd) won't work.

One day if we feel like shaving a yak, we could add migrate to an ArgsAdjuster interface that takes a mutable CompileCommand&...

clang-tools-extra/clangd/ConfigCompile.cpp

273

Hmm, with this new logic in place, if we "normalized" command lines by moving the filename to the end, we'd resolve https://github.com/clangd/clangd/issues/555 without having to complicate the config file model.

I wonder how hard that is...

274–276

maybe a comment "The point to insert at - usually the end"?
(Since this looks suspiciously like an unhandled error case)

[kadircet]

clang-tools-extra/clangd/ConfigCompile.cpp

273

yes actually that sounds like a good idea. Ofc, but at the cost of one extra command line arg parsing. I don't think we have any other reliable way to detect input argument in the flags.

Within CommandMangler::adjust, we can start by doing the normalization, parsing args considering normalized if it has -- already, and moving filename to the end with a -- before otherwise.

Shouldn't be too hard, can't think of a better place in the pipeline. We can also do this inside OverlayCDB before calling the adjusters, but it feels a little bit less natural.

Happy to send out a patch if you think that's a good idea.

[kadircet]

• Address comments

clang-tools-extra/clangd/CompileCommands.cpp

 #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"
+#include <string>
+#include <vector>
 
 namespace clang {
 namespace clangd {
 namespace {
 
 // Query apple's `xcrun` launcher, which is the source of truth for "how should"
 // clang be invoked on this system.
 llvm::Optional<std::string> queryXcrun(llvm::ArrayRef<llvm::StringRef> Argv) {
void CommandMangler::adjust(std::vector<std::string> &Cmd) const {
   // requested on the command line.
   Cmd = tooling::getClangStripDependencyFileAdjuster()(Cmd, "");
   // Strip plugin related command line arguments. Clangd does
   // not support plugins currently. Therefore it breaks if
   // compiler tries to load plugins.
   Cmd = tooling::getStripPluginsAdjuster()(Cmd, "");
   Cmd = tooling::getClangSyntaxOnlyAdjuster()(Cmd, "");
 
+  std::vector<std::string> ToAppend;
   if (ResourceDir && !Has("-resource-dir"))
-    Cmd.push_back(("-resource-dir=" + *ResourceDir));
+    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")) {
-    Cmd.push_back("-isysroot");
-    Cmd.push_back(*Sysroot);
+    ToAppend.push_back("-isysroot");
+    ToAppend.push_back(*Sysroot);
+  }
+
+  if (!ToAppend.empty()) {
+    Cmd = tooling::getInsertArgumentAdjuster(
+        std::move(ToAppend), tooling::ArgumentInsertPosition::END)(Cmd, "");
   }
 
   if (!Cmd.empty()) {
     bool FollowSymlink = !Has("-no-canonical-prefixes");
     Cmd.front() =
         (FollowSymlink ? ResolvedDrivers : ResolvedDriversNoFollow)
             .get(Cmd.front(), [&, this] {
               return resolveDriver(Cmd.front(), FollowSymlink, ClangPath);
             });
   }
 }
if (matchingRule(Args[Read], CurrentMode, ArgCount)) {
         Args[Write] = std::move(Args[Read]);
       ++Write;
     }
     ++Read;
   }
   Args.resize(Write);
 }
 
-
 std::string printArgv(llvm::ArrayRef<llvm::StringRef> Args) {
   std::string Buf;
   llvm::raw_string_ostream OS(Buf);
   bool Sep = false;
   for (llvm::StringRef Arg : Args) {
     if (Sep)
       OS << ' ';
     Sep = true;

clang-tools-extra/clangd/ConfigCompile.cpp

 #include "llvm/Support/Error.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Format.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Regex.h"
 #include "llvm/Support/SMLoc.h"
 #include "llvm/Support/SourceMgr.h"
+#include <algorithm>
 #include <string>
 
 namespace clang {
 namespace clangd {
 namespace config {
 namespace {
 
 // Returns an empty stringref if Path is not under FragmentDir. Returns Path
if (!F.Remove.empty()) {
       });
     }
 
     if (!F.Add.empty()) {
       std::vector<std::string> Add;
       for (auto &A : F.Add)
         Add.push_back(std::move(*A));
       Out.Apply.push_back([Add(std::move(Add))](const Params &, Config &C) {
         C.CompileFlags.Edits.push_back([Add](std::vector<std::string> &Args) {
-          Args.insert(Args.end(), Add.begin(), Add.end());
+          // The point to insert at. Just append when `--` isn't present.
+          auto It = llvm::find(Args, "--");
+          Args.insert(It, Add.begin(), Add.end());
         });
       });
     }
 
     if (F.CompilationDatabase) {
       llvm::Optional<Config::CDBSearchSpec> Spec;
       if (**F.CompilationDatabase == "Ancestors") {
         Spec.emplace();

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

 
 // 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) {
   auto Mangler = CommandMangler::forTests();
   Mangler.ClangPath = testPath("fake/clang");
   Mangler.ResourceDir = testPath("fake/resources");
   Mangler.Sysroot = testPath("fake/sysroot");
-  std::vector<std::string> Cmd = {"clang++", "-Xclang", "-load", "-Xclang",
-                                  "plugin",  "-MF",     "dep",   "foo.cc"};
+  std::vector<std::string> Cmd = {"clang++", "-Xclang", "-load",
+                                  "-Xclang", "plugin",  "-MF",
+                                  "dep",     "--",      "foo.cc"};
   Mangler.adjust(Cmd);
-  EXPECT_THAT(Cmd, ElementsAre(testPath("fake/clang++"), "foo.cc",
-                               "-fsyntax-only",
+  EXPECT_THAT(Cmd, ElementsAre(testPath("fake/clang++"), "-fsyntax-only",
                                "-resource-dir=" + testPath("fake/resources"),
-                               "-isysroot", testPath("fake/sysroot")));
+                               "-isysroot", testPath("fake/sysroot"), "--",
+                               "foo.cc"));
 }
 
 TEST(CommandMangler, ResourceDir) {
   auto Mangler = CommandMangler::forTests();
   Mangler.ResourceDir = testPath("fake/resources");
   std::vector<std::string> Cmd = {"clang++", "foo.cc"};
   Mangler.adjust(Cmd);
   EXPECT_THAT(Cmd, Contains("-resource-dir=" + testPath("fake/resources")));
TEST(PrintArgvTest, All) {
   };
   const char *Expected = R"(one two "thr ee" "f\"o\"ur" "fi\\ve" $)";
   EXPECT_EQ(Expected, printArgv(Args));
 }
 
 } // namespace
 } // namespace clangd
 } // namespace clang
-

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

 #else
   EXPECT_TRUE(compileAndApply());
 #endif
 }
 
 TEST_F(ConfigCompileTests, CompileCommands) {
   Frag.CompileFlags.Add.emplace_back("-foo");
   Frag.CompileFlags.Remove.emplace_back("--include-directory=");
-  std::vector<std::string> Argv = {"clang", "-I", "bar/", "a.cc"};
+  std::vector<std::string> Argv = {"clang", "-I", "bar/", "--", "a.cc"};
   EXPECT_TRUE(compileAndApply());
   EXPECT_THAT(Conf.CompileFlags.Edits, SizeIs(2));
   for (auto &Edit : Conf.CompileFlags.Edits)
     Edit(Argv);
-  EXPECT_THAT(Argv, ElementsAre("clang", "a.cc", "-foo"));
+  EXPECT_THAT(Argv, ElementsAre("clang", "-foo", "--", "a.cc"));
 }
 
 TEST_F(ConfigCompileTests, CompilationDatabase) {
   Frag.CompileFlags.CompilationDatabase.emplace("None");
   EXPECT_TRUE(compileAndApply());
   EXPECT_EQ(Conf.CompileFlags.CDBSearch.Policy,
             Config::CDBSearchSpec::NoCDBSearch);