[Driver] Do not lose already detected set of toolchain's multilibs while iterating over different library path suffixes and different library versions.

[atanasyan]

Summary

To find the most appropriate library for the given command line flags we iterate over a set of disk paths. Before probe each path the already detected set of multilibs are cleared. If the set of paths contains existing paths which do not satisfy command line flags or do not contain necessary libraries and object files at all we might lose found multilibs.

The patch updates variables which hold detected multilibs if we really find a new multilib matches command line flags.

[jroelofs]

Thanks Simon, this is a much clearer fix.

[jroelofs]

lib/Driver/ToolChains.cpp

1907

Don't we need a

Result.Multilibs.clear();

here, that way Multilibs from different candidate sets don't get used together?

Or maybe I don't fully understand the problem that this (or the other patch) solves...

[atanasyan]

Modify the findMIPSMultilibs() function so it really modifies result set of multilibs only in case of successful searching.

[jroelofs]

LGTM

[atanasyan]

Thanks for review.

Closed by commit rL208523 (authored by @atanasyan)

[a.sidorin]

Patch solves a problem for me.

lib/Driver/ToolChains.h

CollectLibDirsAndTriples(const llvm::Triple &TargetTriple,
                              SmallVectorImpl<StringRef> &BiarchLibDirs,
                              SmallVectorImpl<StringRef> &BiarchTripleAliases);
 
     void ScanLibDirForGCCTriple(const llvm::Triple &TargetArch,
                                 const llvm::opt::ArgList &Args,
                                 const std::string &LibDir,
                                 StringRef CandidateTriple,
                                 bool NeedsBiarchSuffix = false);
-
-    bool findMIPSMultilibs(const llvm::Triple &TargetArch, StringRef Path,
-                           const llvm::opt::ArgList &Args);
-
-    bool findBiarchMultilibs(const llvm::Triple &TargetArch, StringRef Path,
-                             const llvm::opt::ArgList &Args,
-                             bool NeedsBiarchSuffix);
   };
 
   GCCInstallationDetector GCCInstallation;
 
 public:
   Generic_GCC(const Driver &D, const llvm::Triple &Triple,
               const llvm::opt::ArgList &Args);
   ~Generic_GCC();

lib/Driver/ToolChains.cpp

static bool isMipsFP64(const ArgList &Args) {
   return A && A->getOption().matches(options::OPT_mfp64);
 }
 
 static bool isMipsNan2008(const ArgList &Args) {
   Arg *A = Args.getLastArg(options::OPT_mnan_EQ);
   return A && A->getValue() == StringRef("2008");
 }
 
-bool Generic_GCC::GCCInstallationDetector::findMIPSMultilibs(
-    const llvm::Triple &TargetTriple, StringRef Path,
-    const llvm::opt::ArgList &Args) {
+struct DetectedMultilibs {
+  /// The set of multilibs that the detected installation supports.
+  MultilibSet Multilibs;
+
+  /// The primary multilib appropriate for the given flags.
+  Multilib SelectedMultilib;
+
+  /// On Biarch systems, this corresponds to the default multilib when
+  /// targeting the non-default multilib. Otherwise, it is empty.
+  llvm::Optional<Multilib> BiarchSibling;
+};
+
+static bool findMIPSMultilibs(const llvm::Triple &TargetTriple, StringRef Path,
+                              const llvm::opt::ArgList &Args,
+                              DetectedMultilibs &Result) {
   // Some MIPS toolchains put libraries and object files compiled
   // using different options in to the sub-directoris which names
   // reflects the flags used for compilation. For example sysroot
   // directory might looks like the following examples:
   //
   // /usr
   //   /lib      <= crt*.o files compiled with '-mips32'
   // /mips16
static bool findMIPSMultilibs(const llvm::Triple &TargetTriple, StringRef Path,
   addMultilibFlag(isSoftFloatABI(Args), "mfloat-abi=soft", Flags);
   addMultilibFlag(!isSoftFloatABI(Args), "mhard-float", Flags);
   addMultilibFlag(!isSoftFloatABI(Args), "mfloat-abi=hard", Flags);
   addMultilibFlag(isMipsEL(TargetArch), "EL", Flags);
   addMultilibFlag(isMipsEB(TargetArch), "EB", Flags);
 
   if (TargetTriple.getEnvironment() == llvm::Triple::Android) {
     // Select Android toolchain. It's the only choice in that case.
-    Multilibs.clear();
-    Multilibs.combineWith(AndroidMipsMultilibs);
-    return Multilibs.select(Flags, SelectedMultilib);
+    if (AndroidMipsMultilibs.select(Flags, Result.SelectedMultilib)) {
+      Result.Multilibs = AndroidMipsMultilibs;
+      return true;
+    }
+    return false;
   }
 
   // Sort candidates. Toolchain that best meets the directories goes first.
   // Then select the first toolchains matches command line flags.
   MultilibSet *candidates[] = { &DebianMipsMultilibs, &FSFMipsMultilibs,
                                 &CSMipsMultilibs };
   std::sort(
       std::begin(candidates), std::end(candidates),
       [](MultilibSet *a, MultilibSet *b) { return a->size() > b->size(); });
   for (const auto &candidate : candidates) {
-    Multilibs.clear();
-    Multilibs.combineWith(*candidate);
-    if (Multilibs.select(Flags, SelectedMultilib)) {
+    if (candidate->select(Flags, Result.SelectedMultilib)) {
       if (candidate == &DebianMipsMultilibs)
-        BiarchSibling = Multilib();
+        Result.BiarchSibling = Multilib();
+      Result.Multilibs = *candidate;
       return true;
     }
   }
 
   return false;
 }
 
-bool Generic_GCC::GCCInstallationDetector::findBiarchMultilibs(
-    const llvm::Triple &TargetTriple, StringRef Path, const ArgList &Args,
-    bool NeedsBiarchSuffix) {
+static bool findBiarchMultilibs(const llvm::Triple &TargetTriple,
+                                StringRef Path, const ArgList &Args,
+                                bool NeedsBiarchSuffix,
+                                DetectedMultilibs &Result) {
 
   // Some versions of SUSE and Fedora on ppc64 put 32-bit libs
   // in what would normally be GCCInstallPath and put the 64-bit
   // libs in a subdirectory named 64. The simple logic we follow is that
   // *if* there is a subdirectory of the right name with crtbegin.o in it,
   // we use that. If not, and if not a biarch triple alias, we look for
   // crtbegin.o without the subdirectory.
 
else
       DefaultIs32Bit = TargetTriple.isArch32Bit();
   }
 
   if (DefaultIs32Bit)
     Default.flag("+m32").flag("-m64");
   else
     Default.flag("-m32").flag("+m64");
 
-  Multilibs.push_back(Default);
-  Multilibs.push_back(Alt64);
-  Multilibs.push_back(Alt32);
+  Result.Multilibs.push_back(Default);
+  Result.Multilibs.push_back(Alt64);
+  Result.Multilibs.push_back(Alt32);
 
-  Multilibs.FilterOut(NonExistent);
+  Result.Multilibs.FilterOut(NonExistent);
 
   Multilib::flags_list Flags;
   addMultilibFlag(TargetTriple.isArch64Bit(), "m64", Flags);
   addMultilibFlag(TargetTriple.isArch32Bit(), "m32", Flags);
 
-  if (!Multilibs.select(Flags, SelectedMultilib))
+  if (!Result.Multilibs.select(Flags, Result.SelectedMultilib))
     return false;
 
-  if (SelectedMultilib == Alt64 || SelectedMultilib == Alt32) {
-    BiarchSibling = Default;
-  }
+  if (Result.SelectedMultilib == Alt64 || Result.SelectedMultilib == Alt32)
+    Result.BiarchSibling = Default;
 
   return true;
 }
 
 void Generic_GCC::GCCInstallationDetector::ScanLibDirForGCCTriple(
     const llvm::Triple &TargetTriple, const ArgList &Args,
     const std::string &LibDir, StringRef CandidateTriple,
     bool NeedsBiarchSuffix) {
for (llvm::sys::fs::directory_iterator LI(LibDir + LibSuffix, EC), LE;
       if (CandidateVersion.Major != -1) // Filter obviously bad entries.
         if (!CandidateGCCInstallPaths.insert(LI->path()).second)
           continue; // Saw this path before; no need to look at it again.
       if (CandidateVersion.isOlderThan(4, 1, 1))
         continue;
       if (CandidateVersion <= Version)
         continue;
 
-      Multilibs.clear();
-      SelectedMultilib = Multilib();
-      BiarchSibling.reset();
+      DetectedMultilibs Detected;
 
       // Debian mips multilibs behave more like the rest of the biarch ones,
       // so handle them there
       if (isMipsArch(TargetArch)) {
-        if (!findMIPSMultilibs(TargetTriple, LI->path(), Args))
+        if (!findMIPSMultilibs(TargetTriple, LI->path(), Args, Detected))
           continue;
       } else if (!findBiarchMultilibs(TargetTriple, LI->path(), Args,
-                                      NeedsBiarchSuffix))
+                                      NeedsBiarchSuffix, Detected)) {
         continue;
+      }
 
+      Multilibs = Detected.Multilibs;
+      SelectedMultilib = Detected.SelectedMultilib;
+      BiarchSibling = Detected.BiarchSibling;
       Version = CandidateVersion;
       GCCTriple.setTriple(CandidateTriple);
       // FIXME: We hack together the directory name here instead of
       // using LI to ensure stable path separators across Windows and
       // Linux.
       GCCInstallPath = LibDir + LibSuffixes[i] + "/" + VersionText.str();
       GCCParentLibPath = GCCInstallPath + InstallSuffixes[i];
       IsValid = true;

test/Driver/linux-header-search.cpp

 // CHECK-UBUNTU-13-04-M32: "{{[^"]*}}clang{{[^"]*}}" "-cc1"
 // CHECK-UBUNTU-13-04-M32: "-triple" "i386-unknown-linux-gnu"
 // CHECK-UBUNTU-13-04-M32: "-isysroot" "[[SYSROOT:[^"]+]]"
 // CHECK-UBUNTU-13-04-M32: "-internal-isystem" "[[SYSROOT]]/usr/lib/gcc/x86_64-linux-gnu/4.7/../../../../include/c++/4.7"
 // CHECK-UBUNTU-13-04-M32: "-internal-isystem" "[[SYSROOT]]/usr/lib/gcc/x86_64-linux-gnu/4.7/../../../../include/c++/4.7/x86_64-linux-gnu/32"
 // CHECK-UBUNTU-13-04-M32: "-internal-isystem" "[[SYSROOT]]/usr/lib/gcc/x86_64-linux-gnu/4.7/../../../../include/c++/4.7/backward"
 // CHECK-UBUNTU-13-04-M32: "-internal-isystem" "[[SYSROOT]]/usr/lib/gcc/x86_64-linux-gnu/4.7/../../../../include/x86_64-linux-gnu/c++/4.7/32"
 //
+// Test Ubuntu/Debian's Ubuntu 14.04 config variant, with -m32
+// and an empty 4.9 directory.
+// RUN: %clang -no-canonical-prefixes %s -### -fsyntax-only 2>&1 \
+// RUN:     -target x86_64-unknown-linux-gnu -m32 \
+// RUN:     --sysroot=%S/Inputs/ubuntu_14.04_multiarch_tree \
+// RUN:   | FileCheck --check-prefix=CHECK-UBUNTU-14-04-M32 %s
+// CHECK-UBUNTU-14-04-M32: "{{[^"]*}}clang{{[^"]*}}" "-cc1"
+// CHECK-UBUNTU-14-04-M32: "-triple" "i386-unknown-linux-gnu"
+// CHECK-UBUNTU-14-04-M32: "-isysroot" "[[SYSROOT:[^"]+]]"
+// CHECK-UBUNTU-14-04-M32: "-internal-isystem" "[[SYSROOT]]/usr/lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8"
+// CHECK-UBUNTU-14-04-M32: "-internal-isystem" "[[SYSROOT]]/usr/lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/x86_64-linux-gnu/32"
+// CHECK-UBUNTU-14-04-M32: "-internal-isystem" "[[SYSROOT]]/usr/lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/backward"
+// CHECK-UBUNTU-14-04-M32: "-internal-isystem" "[[SYSROOT]]/usr/lib/gcc/x86_64-linux-gnu/4.8/../../../../include/x86_64-linux-gnu/c++/4.8/32"
+//
 // Thoroughly exercise the Debian multiarch environment.
 // RUN: %clang -no-canonical-prefixes %s -### -fsyntax-only 2>&1 \
 // RUN:     -target i686-linux-gnu \
 // RUN:     --sysroot=%S/Inputs/debian_multiarch_tree \
 // RUN:   | FileCheck --check-prefix=CHECK-DEBIAN-X86 %s
 // CHECK-DEBIAN-X86: "{{[^"]*}}clang{{[^"]*}}" "-cc1"
 // CHECK-DEBIAN-X86: "-isysroot" "[[SYSROOT:[^"]+]]"
 // CHECK-DEBIAN-X86: "-internal-isystem" "[[SYSROOT]]/usr/lib/gcc/i686-linux-gnu/4.5/../../../../include/c++/4.5"