Enhance the GCC version parsing and comparison logic to handle some more

edge cases and have better behavior. Specifically, we should actually
prefer the general '4.6' version string over the '4.6.1' string, as
'4.6.2' should be able to replace it without breaking rpaths or any
other place that these paths have been embedded. Debian-based
distributions are already using a path structure with symlinks to
achieve in-place upgrades for patch versions. Now our parsing reflects
this and we select the shorter paths instead of the longer paths.

A separate issue was that we would not parse a leading patch version
number even in the presence of a suffix. The above change makes this
more problematic as it would cause a suffix being added to make us treat
the entire thing as patch-version-agnostic, which it isn't. This changes
the logic to distinguish between '4.4.x' and 4.4.1-x', and retain that
the latter has *some* patch number information. Currently, we always
bias toward the shorter and more canonical version strings. If it
becomes important we can add more Debian like rules to produce sequences
such as '4.4.1b' > '4.4.1' > '4.4.1-rc3' > '4.4.1-rc2' > '4.4.1-pre5',
but I'm very doubtful this will ever matter or be desirable.

I've made the tests for this logic a bit more interesting, and added
some specific tests for logic that is now different.

llvm-svn: 143841

clang/lib/Driver/ToolChains.cpp

@@ -1523,34 +1523,78 @@ static LinuxDistro DetectLinuxDistro(llvm::Triple::ArchType Arch) {
 ///
 /// We rely on assumptions about the form and structure of GCC version
 /// numbers: they consist of at most three '.'-separated components, and each
-/// component is a non-negative integer.
+/// component is a non-negative integer except for the last component. For the
+/// last component we are very flexible in order to tolerate release candidates
+/// or 'x' wildcards.
+///
+/// Note that the ordering established among GCCVersions is based on the
+/// preferred version string to use. For example we prefer versions without
+/// a hard-coded patch number to those with a hard coded patch number.
+///
+/// Currently this doesn't provide any logic for textual suffixes to patches in
+/// the way that (for example) Debian's version format does. If that ever
+/// becomes necessary, it can be added.
 struct Linux::GCCVersion {
-  unsigned Major, Minor, Patch;
+  /// \brief The unparsed text of the version.
+  StringRef Text;
+
+  /// \brief The parsed major, minor, and patch numbers.
+  int Major, Minor, Patch;
+
+  /// \brief Any textual suffix on the patch number.
+  StringRef PatchSuffix;
 
   static GCCVersion Parse(StringRef VersionText) {
-    const GCCVersion BadVersion = {0, 0, 0};
+    const GCCVersion BadVersion = { VersionText, -1, -1, -1, "" };
     std::pair<StringRef, StringRef> First = VersionText.split('.');
     std::pair<StringRef, StringRef> Second = First.second.split('.');
 
-    GCCVersion GoodVersion = {0, 0, 0};
-    if (First.first.getAsInteger(10, GoodVersion.Major))
+    GCCVersion GoodVersion = { VersionText, -1, -1, -1, "" };
+    if (First.first.getAsInteger(10, GoodVersion.Major) ||
+        GoodVersion.Major < 0)
       return BadVersion;
-    if (Second.first.getAsInteger(10, GoodVersion.Minor))
+    if (Second.first.getAsInteger(10, GoodVersion.Minor) ||
+        GoodVersion.Minor < 0)
       return BadVersion;
-    // We accept a number, or a string for the patch version, in case there
-    // is a strang suffix, or other mangling: '4.1.x', '4.1.2-rc3'. When it
-    // isn't a number, we just use '0' as the number but accept it.
-    if (Second.first.getAsInteger(10, GoodVersion.Patch))
-      GoodVersion.Patch = 0;
+
+    // First look for a number prefix and parse that if present. Otherwise just
+    // stash the entire patch string in the suffix, and leave the number
+    // unspecified. This covers versions strings such as:
+    //   4.4
+    //   4.4.0
+    //   4.4.x
+    //   4.4.2-rc4
+    //   4.4.x-patched
+    // And retains any patch number it finds.
+    StringRef PatchText = GoodVersion.PatchSuffix = Second.second;
+    if (!PatchText.empty()) {
+      if (unsigned EndNumber = PatchText.find_first_not_of("0123456789")) {
+        // Try to parse the number and any suffix.
+        if (PatchText.slice(0, EndNumber).getAsInteger(10, GoodVersion.Patch) ||
+            GoodVersion.Patch < 0)
+          return BadVersion;
+        GoodVersion.PatchSuffix = PatchText.substr(EndNumber);
+      }
+    }
+
     return GoodVersion;
   }
 
   bool operator<(const GCCVersion &RHS) const {
-    if (Major < RHS.Major) return true;
-    if (Major > RHS.Major) return false;
-    if (Minor < RHS.Minor) return true;
-    if (Minor > RHS.Minor) return false;
-    return Patch < RHS.Patch;
+    if (Major < RHS.Major) return true; if (Major > RHS.Major) return false;
+    if (Minor < RHS.Minor) return true; if (Minor > RHS.Minor) return false;
+
+    // Note that we rank versions with *no* patch specified is better than ones
+    // hard-coding a patch version. Thus if the RHS has no patch, it always
+    // wins, and the LHS only wins if it has no patch and the RHS does have
+    // a patch.
+    if (RHS.Patch == -1) return true;   if (Patch == -1) return false;
+    if (Patch < RHS.Patch) return true; if (Patch > RHS.Patch) return false;
+
+    // Finally, between completely tied version numbers, the version with the
+    // suffix loses as we prefer full releases.
+    if (RHS.PatchSuffix.empty()) return true;
+    return false;
   }
   bool operator>(const GCCVersion &RHS) const { return RHS < *this; }
   bool operator<=(const GCCVersion &RHS) const { return !(*this > RHS); }
@@ -1612,7 +1656,7 @@ Linux::GCCInstallationDetector::GCCInstallationDetector(const Driver &D)
 
   // Loop over the various components which exist and select the best GCC
   // installation available. GCC installs are ranked by version number.
-  GCCVersion BestVersion = {0, 0, 0};
+  GCCVersion BestVersion = GCCVersion::Parse("0.0.0");
   for (unsigned i = 0, ie = Prefixes.size(); i < ie; ++i) {
     if (!llvm::sys::fs::exists(Prefixes[i]))
       continue;
@@ -1716,7 +1760,7 @@ void Linux::GCCInstallationDetector::ScanLibDirForGCCTriple(
          !EC && LI != LE; LI = LI.increment(EC)) {
       StringRef VersionText = llvm::sys::path::filename(LI->path());
       GCCVersion CandidateVersion = GCCVersion::Parse(VersionText);
-      static const GCCVersion MinVersion = { 4, 1, 1 };
+      static const GCCVersion MinVersion = { "4.1.1", 4, 1, 1, "" };
       if (CandidateVersion < MinVersion)
         continue;
       if (CandidateVersion <= BestVersion)

clang/test/Driver/linux-ld.c

@@ -137,3 +137,11 @@
 // CHECK-GCC-VERSION3: "{{.*}}ld{{(.exe)?}}" "--sysroot=[[SYSROOT:[^"]+]]"
 // CHECK-GCC-VERSION3: "{{.*}}/Inputs/gcc_version_parsing3/bin/../lib/gcc/i386-unknown-linux/4.7.99-rc5/crtbegin.o"
 // CHECK-GCC-VERSION3: "-L{{.*}}/Inputs/gcc_version_parsing3/bin/../lib/gcc/i386-unknown-linux/4.7.99-rc5"
+// RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
+// RUN:     -ccc-host-triple i386-unknown-linux -m32 \
+// RUN:     -ccc-install-dir %S/Inputs/gcc_version_parsing4/bin \
+// RUN:     --sysroot=%S/Inputs/basic_linux_tree \
+// RUN:   | FileCheck --check-prefix=CHECK-GCC-VERSION4 %s
+// CHECK-GCC-VERSION4: "{{.*}}ld{{(.exe)?}}" "--sysroot=[[SYSROOT:[^"]+]]"
+// CHECK-GCC-VERSION4: "{{.*}}/Inputs/gcc_version_parsing4/bin/../lib/gcc/i386-unknown-linux/4.7.99/crtbegin.o"
+// CHECK-GCC-VERSION4: "-L{{.*}}/Inputs/gcc_version_parsing4/bin/../lib/gcc/i386-unknown-linux/4.7.99"