[clang-tools-extra] 3bf7798 - [clangd] Strip mutliple arch options

[Kadir Cetinkaya via cfe-commits]

Author: Kadir Cetinkaya
Date: 2021-08-06T15:04:04+02:00
New Revision: 3bf77980d934c4aa383e4ea9a9a5de86598bea9b

URL: https://github.com/llvm/llvm-project/commit/3bf77980d934c4aa383e4ea9a9a5de86598bea9b
DIFF: https://github.com/llvm/llvm-project/commit/3bf77980d934c4aa383e4ea9a9a5de86598bea9b.diff

LOG: [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

Added: 
    

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

Removed: 
    


################################################################################
diff  --git a/clang-tools-extra/clangd/CompileCommands.cpp b/clang-tools-extra/clangd/CompileCommands.cpp
index 032bf7354d770..76c92a0bac735 100644
--- 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 175ceb4552740..d1f97233fa672 100644
--- 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