[clangd] Strip mutliple arch options

This patch strips all the arch options in case of multiple ones. As it
results in multiple compiler jobs, which clangd cannot handle.

It doesn't pick any over the others as it is unclear which one the user wants
and defaulting to host architecture seems less surprising. Users also have the
ability to explicitly specify the architecture they want via clangd config
files.

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

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

diff --git a/clang-tools-extra/clangd/CompileCommands.cpp b/clang-tools-extra/clangd/CompileCommands.cpp
index 032bf73..76c92a0 100644 (file)
--- a/clang-tools-extra/clangd/CompileCommands.cpp
+++ b/clang-tools-extra/clangd/CompileCommands.cpp
@@ -222,22 +222,36 @@ void CommandMangler::adjust(std::vector<std::string> &Cmd,
       /*FlagsToExclude=*/driver::options::NoDriverOption |
           (IsCLMode ? 0 : driver::options::CLOption));
 
+  llvm::SmallVector<unsigned, 1> IndicesToDrop;
+  // Having multiple architecture options (e.g. when building fat binaries)
+  // results in multiple compiler jobs, which clangd cannot handle. In such
+  // cases strip all the `-arch` options and fallback to default architecture.
+  // As there are no signals to figure out which one user actually wants. They
+  // can explicitly specify one through `CompileFlags.Add` if need be.
+  unsigned ArchOptCount = 0;
+  for (auto *Input : ArgList.filtered(driver::options::OPT_arch)) {
+    ++ArchOptCount;
+    for (auto I = 0U; I <= Input->getNumValues(); ++I)
+      IndicesToDrop.push_back(Input->getIndex() + I);
+  }
+  // If there is a single `-arch` option, keep it.
+  if (ArchOptCount < 2)
+    IndicesToDrop.clear();
   // 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)) {
     // Drop all the inputs and only add one for the current file.
-    llvm::SmallVector<unsigned, 1> IndicesToDrop;
     for (auto *Input : ArgList.filtered(driver::options::OPT_INPUT))
       IndicesToDrop.push_back(Input->getIndex());
-    llvm::sort(IndicesToDrop);
-    llvm::for_each(llvm::reverse(IndicesToDrop),
-                   // +1 to account for the executable name in Cmd[0] that
-                   // doesn't exist in ArgList.
-                   [&Cmd](unsigned Idx) { Cmd.erase(Cmd.begin() + Idx + 1); });
     Cmd.push_back("--");
     Cmd.push_back(File.str());
   }
+  llvm::sort(IndicesToDrop);
+  llvm::for_each(llvm::reverse(IndicesToDrop),
+                 // +1 to account for the executable name in Cmd[0] that
+                 // doesn't exist in ArgList.
+                 [&Cmd](unsigned Idx) { Cmd.erase(Cmd.begin() + Idx + 1); });
 
   for (auto &Edit : Config::current().CompileFlags.Edits)
     Edit(Cmd);

diff --git a/clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp b/clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
index 175ceb4..d1f9723 100644 (file)
--- a/clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
+++ b/clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
@@ -13,8 +13,10 @@
 
 #include "clang/Tooling/ArgumentsAdjusters.h"
 #include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/StringExtras.h"
+#include "llvm/ADT/StringRef.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Process.h"
@@ -377,6 +379,23 @@ TEST(CommandMangler, InputsAfterDashDash) {
         testing::AllOf(Not(Contains("foo.cc")), Not(Contains("bar.cc"))));
   }
 }
+
+TEST(CommandMangler, StripsMultipleArch) {
+  const auto Mangler = CommandMangler::forTests();
+  std::vector<std::string> Args = {"clang", "-arch", "foo",
+                                   "-arch", "bar",   "/Users/foo.cc"};
+  Mangler.adjust(Args, "/Users/foo.cc");
+  EXPECT_EQ(
+      llvm::count_if(Args, [](llvm::StringRef Arg) { return Arg == "-arch"; }),
+      0);
+
+  // Single arch option is preserved.
+  Args = {"clang", "-arch", "foo", "/Users/foo.cc"};
+  Mangler.adjust(Args, "/Users/foo.cc");
+  EXPECT_EQ(
+      llvm::count_if(Args, [](llvm::StringRef Arg) { return Arg == "-arch"; }),
+      1);
+}
 } // namespace
 } // namespace clangd
 } // namespace clang