[PATCH] D107634: [clangd] Strip mutliple arch options

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet created this revision.
kadircet added a reviewer: sammccall.
Herald added subscribers: usaxena95, arphaman, mgrang.
kadircet requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D107634

Files:
  clang-tools-extra/clangd/CompileCommands.cpp
  clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp


Index: clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
+++ 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 @@
         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
Index: clang-tools-extra/clangd/CompileCommands.cpp
===================================================================
--- clang-tools-extra/clangd/CompileCommands.cpp
+++ clang-tools-extra/clangd/CompileCommands.cpp
@@ -222,22 +222,36 @@
       /*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 host 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);


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D107634.364747.patch
Type: text/x-patch
Size: 3753 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20210806/fd04f44a/attachment.bin>