Add testing for Fuchsia multilib

[michaelplatings]

Summary

The plan is to change how multilib works under the hood and this test
gives confidence that the changes won't break which multilib variant is
selected.

[michaelplatings]

Update commit message and run arc with clang-format on the PATH

[phosek]

Thanks, some of these are no longer needed which could simplify this change so I sent out D143050 to clean up the Fuchsia driver first.

[michaelplatings]

Rebase

[phosek]

clang/lib/Driver/ToolChains/Fuchsia.cpp

316–317

Could we pass const ArgList &Args instead? I'm concerned that this approach won't scale in the future as we keep on adding more flags.

clang/unittests/Driver/FuchsiaTest.cpp

18–23

Nit: we prefer // for comments.

30–43

While exhaustive, I'm concerned about the scalability of this approach. We plan on adding more multilibs in the future so the number of combinations is going to grow exponentially. A more scalable approach would be to check only the minimal set of combination necessary to achieve full coverage.

[michaelplatings]

A more scalable approach would be to check only the minimal set of combination necessary to achieve full coverage.

In that case the testing you've already got in place in clang/test/Driver/fuchsia.cpp is adequate. There was a potential bug here so I was being extra cautious, but in practice the test added in this change didn't pick up anything that wasn't also caught by the existing tests. I'll abandon this change and remove it from the stack. The change stack now begins at D142893.

clang/lib/Driver/ToolChains/Fuchsia.h

public:
   void AddClangCXXStdlibIncludeArgs(
       const llvm::opt::ArgList &DriverArgs,
       llvm::opt::ArgStringList &CC1Args) const override;
   void AddCXXStdlibLibArgs(const llvm::opt::ArgList &Args,
                            llvm::opt::ArgStringList &CmdArgs) const override;
 
   const char *getDefaultLinker() const override { return "ld.lld"; }
 
+  static void configureMultilibs(MultilibSet &);
+  static void configureMultilibFlags(Multilib::flags_list &, bool Exceptions,
+                                     bool Asan, bool Hwasan, bool Itanium);
+
 protected:
   Tool *buildLinker() const override;
   Tool *buildStaticLibTool() const override;
 };
 
 } // end namespace toolchains
 } // end namespace driver
 } // end namespace clang
 
 #endif // LLVM_CLANG_LIB_DRIVER_TOOLCHAINS_FUCHSIA_H

clang/lib/Driver/ToolChains/Fuchsia.cpp

auto FilePaths = [&](const Multilib &M) -> std::vector<std::string> {
     for (const std::string &Path : getStdlibPaths()) {
       SmallString<128> P(Path);
       llvm::sys::path::append(P, M.gccSuffix());
       FP.push_back(std::string(P.str()));
     }
     return FP;
   };
 
+  configureMultilibs(Multilibs);
+
+  Multilibs.FilterOut([&](const Multilib &M) {
+    std::vector<std::string> RD = FilePaths(M);
+    return llvm::all_of(RD, [&](std::string P) { return !getVFS().exists(P); });
+  });
+
+  Multilib::flags_list Flags;
+  configureMultilibFlags(
+      Flags,
+      Args.hasFlag(options::OPT_fexceptions, options::OPT_fno_exceptions, true),
+      getSanitizerArgs(Args).needsAsanRt(),
+      getSanitizerArgs(Args).needsHwasanRt(),
+      Args.getLastArgValue(options::OPT_fcxx_abi_EQ) == "itanium");
+
+  Multilibs.setFilePathsCallback(FilePaths);
+
+  if (Multilibs.select(Flags, SelectedMultilib))
+    if (!SelectedMultilib.isDefault())
+      if (const auto &PathsCallback = Multilibs.filePathsCallback())
+        for (const auto &Path : PathsCallback(SelectedMultilib))
+          // Prepend the multilib path to ensure it takes the precedence.
+          getFilePaths().insert(getFilePaths().begin(), Path);
+}
+
+void Fuchsia::configureMultilibs(MultilibSet &Multilibs) {
   Multilibs.push_back(Multilib());
   // Use the noexcept variant with -fno-exceptions to avoid the extra overhead.
   Multilibs.push_back(Multilib("noexcept", {}, {}, 1)
                           .flag("-fexceptions")
                           .flag("+fno-exceptions"));
   // ASan has higher priority because we always want the instrumentated version.
   Multilibs.push_back(Multilib("asan", {}, {}, 2)
                           .flag("+fsanitize=address"));
   // Use the asan+noexcept variant with ASan and -fno-exceptions.
   Multilibs.push_back(Multilib("asan+noexcept", {}, {}, 3)
                           .flag("+fsanitize=address")
                           .flag("-fexceptions")
                           .flag("+fno-exceptions"));
   // HWASan has higher priority because we always want the instrumentated
   // version.
   Multilibs.push_back(
       Multilib("hwasan", {}, {}, 4).flag("+fsanitize=hwaddress"));
   // Use the hwasan+noexcept variant with HWASan and -fno-exceptions.
   Multilibs.push_back(Multilib("hwasan+noexcept", {}, {}, 5)
                           .flag("+fsanitize=hwaddress")
                           .flag("-fexceptions")
                           .flag("+fno-exceptions"));
   // Use Itanium C++ ABI for the compat multilib.
   Multilibs.push_back(Multilib("compat", {}, {}, 6).flag("+fc++-abi=itanium"));
+}
 
-  Multilibs.FilterOut([&](const Multilib &M) {
-    std::vector<std::string> RD = FilePaths(M);
-    return llvm::all_of(RD, [&](std::string P) { return !getVFS().exists(P); });
-  });
-
-  Multilib::flags_list Flags;
-  addMultilibFlag(
-      Args.hasFlag(options::OPT_fexceptions, options::OPT_fno_exceptions, true),
-      "fexceptions", Flags);
-  addMultilibFlag(getSanitizerArgs(Args).needsAsanRt(), "fsanitize=address",
-                  Flags);
-  addMultilibFlag(getSanitizerArgs(Args).needsHwasanRt(), "fsanitize=hwaddress",
-                  Flags);
-
-  addMultilibFlag(Args.getLastArgValue(options::OPT_fcxx_abi_EQ) == "itanium",
-                  "fc++-abi=itanium", Flags);
-
-  Multilibs.setFilePathsCallback(FilePaths);
-
-  if (Multilibs.select(Flags, SelectedMultilib))
-    if (!SelectedMultilib.isDefault())
-      if (const auto &PathsCallback = Multilibs.filePathsCallback())
-        for (const auto &Path : PathsCallback(SelectedMultilib))
-          // Prepend the multilib path to ensure it takes the precedence.
-          getFilePaths().insert(getFilePaths().begin(), Path);
+void Fuchsia::configureMultilibFlags(Multilib::flags_list &Flags,
+                                     bool Exceptions, bool Asan, bool Hwasan,
+                                     bool Itanium) {
+  addMultilibFlag(Exceptions, "fexceptions", Flags);
+  addMultilibFlag(!Exceptions, "fno-exceptions", Flags);
+  addMultilibFlag(Asan, "fsanitize=address", Flags);
+  addMultilibFlag(Hwasan, "fsanitize=hwaddress", Flags);
+  addMultilibFlag(Itanium, "fc++-abi=itanium", Flags);
 }
 
 std::string Fuchsia::ComputeEffectiveClangTriple(const ArgList &Args,
                                                  types::ID InputType) const {
   llvm::Triple Triple(ComputeLLVMTriple(Args, InputType));
   return Triple.str();
 }
 

clang/unittests/Driver/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   ${LLVM_TARGETS_TO_BUILD}
   MC
   Option
   Support
   TargetParser
   )
 
 add_clang_unittest(ClangDriverTests
   DistroTest.cpp
   DXCModeTest.cpp
+  FuchsiaTest.cpp
   ToolChainTest.cpp
   ModuleCacheTest.cpp
   MultilibTest.cpp
   SanitizerArgsTest.cpp
   )
 
 clang_target_link_libraries(ClangDriverTests
   PRIVATE
   clangDriver
   clangBasic
   clangFrontend # For TextDiagnosticPrinter.
   )

clang/unittests/Driver/FuchsiaTest.cpp

+//===- unittests/Driver/FuchsiaTest.cpp --- Fuchsia tests ---------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+//
+// Unit tests for Fuchsia
+//
+//===----------------------------------------------------------------------===//
+
+#include "../../lib/Driver/ToolChains/Fuchsia.h"
+#include "gtest/gtest.h"
+
+using namespace clang::driver;
+
+/*
+This test was added prior to changing the behaviour of Multilib.
+The way that Fuchsia used Multilib made it very likely that the change
+would cause it to break so by adding this exhaustive test we avoid that
+possibility.
+*/
+TEST(FuchsiaTest, MultilibVariant) {
+  MultilibSet Multilibs;
+  toolchains::Fuchsia::configureMultilibs(Multilibs);
+
+  std::string Actual;
+
+  for (bool Itanium : {false, true}) {
+    for (bool Hwasan : {false, true}) {
+      for (bool Asan : {false, true}) {
+        for (bool Exceptions : {false, true}) {
+          Multilib::flags_list Flags;
+          toolchains::Fuchsia::configureMultilibFlags(Flags, Exceptions, Asan,
+                                                      Hwasan, Itanium);
+          Multilib Selected;
+          EXPECT_TRUE(Multilibs.select(Flags, Selected));
+          Actual += Selected.gccSuffix() + "\n";
+        }
+      }
+    }
+  }
+  std::string Expected = R"(/noexcept
+
+/asan+noexcept
+/asan
+/hwasan+noexcept
+/hwasan
+/hwasan+noexcept
+/hwasan
+/compat
+/compat
+/compat
+/compat
+/compat
+/compat
+/compat
+/compat
+)";
+  EXPECT_EQ(Expected, Actual);
+}