[clangd] Strip mutliple arch options

[kadircet]

Summary

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.

[sammccall]

clang-tools-extra/clangd/CompileCommands.cpp

225

nit: llvm::SmallVector<unsigned> unless you're very sure about the 1

226

nit: fat isn't an acronym here

228

s/host/default/

[kadircet]

clang-tools-extra/clangd/CompileCommands.cpp

225

I think we'll still have just the single filename in the most common case and will only have -arch or multiple filenames rarely. so keeping it.

[kadircet]

• Update comments

clang-tools-extra/clangd/CompileCommands.cpp

void CommandMangler::adjust(std::vector<std::string> &Cmd,
   auto ArgList = OptTable.ParseArgs(
       llvm::makeArrayRef(OriginalArgs).drop_front(), IgnoredCount, IgnoredCount,
       /*FlagsToInclude=*/
       IsCLMode ? (driver::options::CLOption | driver::options::CoreOption)
                : /*everything*/ 0,
       /*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());
-  }
+    Cmd.push_back("--");
+    Cmd.push_back(File.str());
+  }
 
   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] == '='))

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/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"
 
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 namespace clang {
// All inputs but the main file is dropped.
     Mangler.adjust(Args, "baz.cc");
     EXPECT_THAT(llvm::makeArrayRef(Args).take_back(2),
                 ElementsAre("--", "baz.cc"));
     EXPECT_THAT(
         llvm::makeArrayRef(Args).drop_back(2),
         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