compiletest: improve robustness of LLVM version handling

Author: jieyouxu

src/tools/compiletest/src/common.rs

@@ -167,6 +167,25 @@ pub enum Sanitizer {
     Hwaddress,
 }
 
+#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord)]
+pub struct LlvmVersion {
+    pub major: u32,
+    pub minor: u32,
+    pub patch: u32,
+}
+
+impl LlvmVersion {
+    pub fn new(major: u32, minor: u32, patch: u32) -> LlvmVersion {
+        Self { major, minor, patch }
+    }
+}
+
+impl fmt::Display for LlvmVersion {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> fmt::Result {
+        write!(f, "{}.{}.{}", self.major, self.minor, self.patch)
+    }
+}
+
 /// Configuration for compiletest
 #[derive(Debug, Default, Clone)]
 pub struct Config {
@@ -298,7 +317,7 @@ pub struct Config {
     pub lldb_version: Option<u32>,
 
     /// Version of LLVM
-    pub llvm_version: Option<u32>,
+    pub llvm_version: Option<LlvmVersion>,
 
     /// Is LLVM a system LLVM
     pub system_llvm: bool,

src/tools/compiletest/src/header.rs

@@ -8,7 +8,7 @@ use std::process::Command;
 
 use tracing::*;
 
-use crate::common::{Config, Debugger, FailMode, Mode, PassMode};
+use crate::common::{Config, Debugger, FailMode, LlvmVersion, Mode, PassMode};
 use crate::debuggers::{extract_cdb_version, extract_gdb_version};
 use crate::header::auxiliary::{AuxProps, parse_and_update_aux};
 use crate::header::cfg::{MatchOutcome, parse_cfg_name_directive};
@@ -1113,34 +1113,45 @@ fn parse_normalize_rule(header: &str) -> Option<(String, String)> {
     Some((regex, replacement))
 }
 
-pub fn extract_llvm_version(version: &str) -> Option<u32> {
-    let pat = |c: char| !c.is_ascii_digit() && c != '.';
-    let version_without_suffix = match version.find(pat) {
-        Some(pos) => &version[..pos],
+/// Given an llvm version string that looks like `1.2.3-rc1`, extract key version info into a
+/// [`LlvmVersion`].
+///
+/// Currently panics if the input string is malformed, though we really should not use panic as an
+/// error handling strategy.
+///
+/// FIXME(jieyouxu): improve error handling
+pub fn extract_llvm_version(version: &str) -> LlvmVersion {
+    // The version substring we're interested in usually looks like the `1.2.3`, without any of the
+    // fancy suffix like `-rc1` or `meow`.
+    let version = version.trim();
+    let uninterested = |c: char| !c.is_ascii_digit() && c != '.';
+    let version_without_suffix = match version.split_once(uninterested) {
+        Some((prefix, _suffix)) => prefix,
         None => version,
     };
+
     let components: Vec<u32> = version_without_suffix
         .split('.')
-        .map(|s| s.parse().expect("Malformed version component"))
+        .map(|s| s.parse().expect("llvm version component should consist of only digits"))
         .collect();
-    let version = match *components {
-        [a] => a * 10_000,
-        [a, b] => a * 10_000 + b * 100,
-        [a, b, c] => a * 10_000 + b * 100 + c,
-        _ => panic!("Malformed version"),
-    };
-    Some(version)
+
+    match &components[..] {
+        [major] => LlvmVersion::new(*major, 0, 0),
+        [major, minor] => LlvmVersion::new(*major, *minor, 0),
+        [major, minor, patch] => LlvmVersion::new(*major, *minor, *patch),
+        _ => panic!("malformed llvm version string, expected only 1-3 components: {version}"),
+    }
 }
 
-pub fn extract_llvm_version_from_binary(binary_path: &str) -> Option<u32> {
+pub fn extract_llvm_version_from_binary(binary_path: &str) -> Option<LlvmVersion> {
     let output = Command::new(binary_path).arg("--version").output().ok()?;
     if !output.status.success() {
         return None;
     }
     let version = String::from_utf8(output.stdout).ok()?;
     for line in version.lines() {
         if let Some(version) = line.split("LLVM version ").nth(1) {
-            return extract_llvm_version(version);
+            return Some(extract_llvm_version(version));
         }
     }
     None
@@ -1247,15 +1258,14 @@ pub fn llvm_has_libzstd(config: &Config) -> bool {
     false
 }
 
-/// Takes a directive of the form `"<version1> [- <version2>]"`,
-/// returns the numeric representation of `<version1>` and `<version2>` as
-/// tuple: `(<version1> as u32, <version2> as u32)`.
+/// Takes a directive of the form `"<version1> [- <version2>]"`, returns the numeric representation
+/// of `<version1>` and `<version2>` as tuple: `(<version1>, <version2>)`.
 ///
-/// If the `<version2>` part is omitted, the second component of the tuple
-/// is the same as `<version1>`.
-fn extract_version_range<F>(line: &str, parse: F) -> Option<(u32, u32)>
+/// If the `<version2>` part is omitted, the second component of the tuple is the same as
+/// `<version1>`.
+fn extract_version_range<F, VersionTy: Copy>(line: &str, parse: F) -> Option<(VersionTy, VersionTy)>
 where
-    F: Fn(&str) -> Option<u32>,
+    F: Fn(&str) -> Option<VersionTy>,
 {
     let mut splits = line.splitn(2, "- ").map(str::trim);
     let min = splits.next().unwrap();
@@ -1490,42 +1500,54 @@ fn ignore_llvm(config: &Config, line: &str) -> IgnoreDecision {
         }
     }
     if let Some(actual_version) = config.llvm_version {
-        if let Some(rest) = line.strip_prefix("min-llvm-version:").map(str::trim) {
-            let min_version = extract_llvm_version(rest).unwrap();
-            // Ignore if actual version is smaller the minimum required
-            // version
+        // Note that these `min` versions will check for not just major versions.
+
+        if let Some(version_string) = config.parse_name_value_directive(line, "min-llvm-version") {
+            let min_version = extract_llvm_version(&version_string);
+            // Ignore if actual version is smaller than the minimum required version.
             if actual_version < min_version {
                 return IgnoreDecision::Ignore {
-                    reason: format!("ignored when the LLVM version is older than {rest}"),
+                    reason: format!(
+                        "ignored when the LLVM version {actual_version} is older than {min_version}"
+                    ),
                 };
             }
-        } else if let Some(rest) = line.strip_prefix("min-system-llvm-version:").map(str::trim) {
-            let min_version = extract_llvm_version(rest).unwrap();
+        } else if let Some(version_string) =
+            config.parse_name_value_directive(line, "min-system-llvm-version")
+        {
+            let min_version = extract_llvm_version(&version_string);
             // Ignore if using system LLVM and actual version
             // is smaller the minimum required version
             if config.system_llvm && actual_version < min_version {
                 return IgnoreDecision::Ignore {
-                    reason: format!("ignored when the system LLVM version is older than {rest}"),
+                    reason: format!(
+                        "ignored when the system LLVM version {actual_version} is older than {min_version}"
+                    ),
                 };
             }
-        } else if let Some(rest) = line.strip_prefix("ignore-llvm-version:").map(str::trim) {
+        } else if let Some(version_range) =
+            config.parse_name_value_directive(line, "ignore-llvm-version")
+        {
             // Syntax is: "ignore-llvm-version: <version1> [- <version2>]"
             let (v_min, v_max) =
-                extract_version_range(rest, extract_llvm_version).unwrap_or_else(|| {
-                    panic!("couldn't parse version range: {:?}", rest);
-                });
+                extract_version_range(&version_range, |s| Some(extract_llvm_version(s)))
+                    .unwrap_or_else(|| {
+                        panic!("couldn't parse version range: \"{version_range}\"");
+                    });
             if v_max < v_min {
-                panic!("Malformed LLVM version range: max < min")
+                panic!("malformed LLVM version range where {v_max} < {v_min}")
             }
             // Ignore if version lies inside of range.
             if actual_version >= v_min && actual_version <= v_max {
                 if v_min == v_max {
                     return IgnoreDecision::Ignore {
-                        reason: format!("ignored when the LLVM version is {rest}"),
+                        reason: format!("ignored when the LLVM version is {actual_version}"),
                     };
                 } else {
                     return IgnoreDecision::Ignore {
-                        reason: format!("ignored when the LLVM version is between {rest}"),
+                        reason: format!(
+                            "ignored when the LLVM version is between {v_min} and {v_max}"
+                        ),
                     };
                 }
             }

src/tools/compiletest/src/header/tests.rs

@@ -1,9 +1,11 @@
 use std::io::Read;
 use std::path::Path;
 
-use super::iter_header;
+use super::{
+    EarlyProps, HeadersCache, LlvmVersion, extract_llvm_version, extract_version_range,
+    iter_header, parse_normalize_rule,
+};
 use crate::common::{Config, Debugger, Mode};
-use crate::header::{EarlyProps, HeadersCache, parse_normalize_rule};
 
 fn make_test_description<R: Read>(
     config: &Config,
@@ -407,19 +409,58 @@ fn channel() {
     assert!(!check_ignore(&config, "//@ ignore-stable"));
 }
 
+#[test]
+fn test_extract_llvm_version() {
+    assert_eq!(extract_llvm_version("0"), LlvmVersion::new(0, 0, 0));
+    assert_eq!(extract_llvm_version("0.0"), LlvmVersion::new(0, 0, 0));
+    assert_eq!(extract_llvm_version("0.0.0"), LlvmVersion::new(0, 0, 0));
+    assert_eq!(extract_llvm_version("1"), LlvmVersion::new(1, 0, 0));
+    assert_eq!(extract_llvm_version("1.2"), LlvmVersion::new(1, 2, 0));
+    assert_eq!(extract_llvm_version("1.2.3"), LlvmVersion::new(1, 2, 3));
+    assert_eq!(extract_llvm_version("4git"), LlvmVersion::new(4, 0, 0));
+    assert_eq!(extract_llvm_version("4.5git"), LlvmVersion::new(4, 5, 0));
+    assert_eq!(extract_llvm_version("4.5.6git"), LlvmVersion::new(4, 5, 6));
+    assert_eq!(extract_llvm_version("4.5.6-rc1"), LlvmVersion::new(4, 5, 6));
+    assert_eq!(extract_llvm_version("123.456.789-rc1"), LlvmVersion::new(123, 456, 789));
+}
+
+#[test]
+#[should_panic]
+fn test_llvm_version_invalid_components() {
+    extract_llvm_version("4.x.6");
+}
+
+#[test]
+#[should_panic]
+fn test_llvm_version_invalid_prefix() {
+    extract_llvm_version("meow4.5.6");
+}
+
+#[test]
+#[should_panic]
+fn test_llvm_version_too_many_components() {
+    extract_llvm_version("4.5.6.7");
+}
+
 #[test]
 fn test_extract_version_range() {
-    use super::{extract_llvm_version, extract_version_range};
-
-    assert_eq!(extract_version_range("1.2.3 - 4.5.6", extract_llvm_version), Some((10203, 40506)));
-    assert_eq!(extract_version_range("0   - 4.5.6", extract_llvm_version), Some((0, 40506)));
-    assert_eq!(extract_version_range("1.2.3 -", extract_llvm_version), None);
-    assert_eq!(extract_version_range("1.2.3 - ", extract_llvm_version), None);
-    assert_eq!(extract_version_range("- 4.5.6", extract_llvm_version), None);
-    assert_eq!(extract_version_range("-", extract_llvm_version), None);
-    assert_eq!(extract_version_range(" - 4.5.6", extract_llvm_version), None);
-    assert_eq!(extract_version_range("   - 4.5.6", extract_llvm_version), None);
-    assert_eq!(extract_version_range("0  -", extract_llvm_version), None);
+    let wrapped_extract = |s: &str| Some(extract_llvm_version(s));
+
+    assert_eq!(
+        extract_version_range("1.2.3 - 4.5.6", wrapped_extract),
+        Some((LlvmVersion::new(1, 2, 3), LlvmVersion::new(4, 5, 6)))
+    );
+    assert_eq!(
+        extract_version_range("0   - 4.5.6", wrapped_extract),
+        Some((LlvmVersion::new(0, 0, 0), LlvmVersion::new(4, 5, 6)))
+    );
+    assert_eq!(extract_version_range("1.2.3 -", wrapped_extract), None);
+    assert_eq!(extract_version_range("1.2.3 - ", wrapped_extract), None);
+    assert_eq!(extract_version_range("- 4.5.6", wrapped_extract), None);
+    assert_eq!(extract_version_range("-", wrapped_extract), None);
+    assert_eq!(extract_version_range(" - 4.5.6", wrapped_extract), None);
+    assert_eq!(extract_version_range("   - 4.5.6", wrapped_extract), None);
+    assert_eq!(extract_version_range("0  -", wrapped_extract), None);
 }
 
 #[test]

src/tools/compiletest/src/lib.rs

@@ -228,7 +228,7 @@ pub fn parse_config(args: Vec<String>) -> Config {
         Some(x) => panic!("argument for --color must be auto, always, or never, but found `{}`", x),
     };
     let llvm_version =
-        matches.opt_str("llvm-version").as_deref().and_then(header::extract_llvm_version).or_else(
+        matches.opt_str("llvm-version").as_deref().map(header::extract_llvm_version).or_else(
             || header::extract_llvm_version_from_binary(&matches.opt_str("llvm-filecheck")?),
         );
 

src/tools/compiletest/src/tests.rs

@@ -1,7 +1,6 @@
 use std::ffi::OsString;
 
 use crate::debuggers::{extract_gdb_version, extract_lldb_version};
-use crate::header::extract_llvm_version;
 use crate::is_test;
 
 #[test]
@@ -67,15 +66,3 @@ fn is_test_test() {
     assert!(!is_test(&OsString::from("#a_dog_gif")));
     assert!(!is_test(&OsString::from("~a_temp_file")));
 }
-
-#[test]
-fn test_extract_llvm_version() {
-    assert_eq!(extract_llvm_version("8.1.2-rust"), Some(80102));
-    assert_eq!(extract_llvm_version("9.0.1-rust-1.43.0-dev"), Some(90001));
-    assert_eq!(extract_llvm_version("9.3.1-rust-1.43.0-dev"), Some(90301));
-    assert_eq!(extract_llvm_version("10.0.0-rust"), Some(100000));
-    assert_eq!(extract_llvm_version("11.1.0"), Some(110100));
-    assert_eq!(extract_llvm_version("12.0.0libcxx"), Some(120000));
-    assert_eq!(extract_llvm_version("12.0.0-rc3"), Some(120000));
-    assert_eq!(extract_llvm_version("13.0.0git"), Some(130000));
-}