Skip to content

Commit

Permalink
Unrolled build for rust-lang#126370
Browse files Browse the repository at this point in the history
Rollup merge of rust-lang#126370 - Zalathar:normalize, r=oli-obk

compiletest: Stricter parsing of `//@ normalize-*` headers

I noticed some problems with the existing parser for these headers:

- It is extremely lax, and basically ignores everything other than the text between two pairs of double-quote characters.
  - Unlike other name-value headers, it doesn't even check for a colon after the header name, so the test suite contains a mixture of with-colon and without-colon normalization rules.
- If parsing fails, the header is silently ignored.

The latter is especially bad for platform-specific normalization rules, because the lack of normalization probably won't be noticed until the test mysteriously fails in one of the full CI jobs.
  • Loading branch information
rust-timer authored Jun 13, 2024
2 parents 921645c + e09eedb commit b533f6c
Show file tree
Hide file tree
Showing 5 changed files with 74 additions and 61 deletions.
48 changes: 27 additions & 21 deletions src/tools/compiletest/src/header.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ use crate::common::{Config, Debugger, FailMode, Mode, PassMode};
use crate::header::cfg::parse_cfg_name_directive;
use crate::header::cfg::MatchOutcome;
use crate::header::needs::CachedNeedsConditions;
use crate::util::static_regex;
use crate::{extract_cdb_version, extract_gdb_version};

mod cfg;
Expand Down Expand Up @@ -1186,11 +1187,11 @@ impl Config {
}
}

fn parse_custom_normalization(&self, mut line: &str, prefix: &str) -> Option<(String, String)> {
fn parse_custom_normalization(&self, line: &str, prefix: &str) -> Option<(String, String)> {
if parse_cfg_name_directive(self, line, prefix).outcome == MatchOutcome::Match {
let from = parse_normalization_string(&mut line)?;
let to = parse_normalization_string(&mut line)?;
Some((from, to))
let (regex, replacement) = parse_normalize_rule(line)
.unwrap_or_else(|| panic!("couldn't parse custom normalization rule: `{line}`"));
Some((regex, replacement))
} else {
None
}
Expand Down Expand Up @@ -1311,24 +1312,29 @@ fn expand_variables(mut value: String, config: &Config) -> String {
value
}

/// Finds the next quoted string `"..."` in `line`, and extract the content from it. Move the `line`
/// variable after the end of the quoted string.
///
/// # Examples
///
/// ```
/// let mut s = "normalize-stderr-32bit: \"something (32 bits)\" -> \"something ($WORD bits)\".";
/// let first = parse_normalization_string(&mut s);
/// assert_eq!(first, Some("something (32 bits)".to_owned()));
/// assert_eq!(s, " -> \"something ($WORD bits)\".");
/// Parses the regex and replacement values of a `//@ normalize-*` header,
/// in the format:
/// ```text
/// normalize-*: "REGEX" -> "REPLACEMENT"
/// ```
fn parse_normalization_string(line: &mut &str) -> Option<String> {
// FIXME support escapes in strings.
let begin = line.find('"')? + 1;
let end = line[begin..].find('"')? + begin;
let result = line[begin..end].to_owned();
*line = &line[end + 1..];
Some(result)
fn parse_normalize_rule(header: &str) -> Option<(String, String)> {
// FIXME(#126370): A colon after the header name should be mandatory, but
// currently is not, and there are many tests that lack the colon.
// FIXME: Support escaped double-quotes in strings.
let captures = static_regex!(
r#"(?x) # (verbose mode regex)
^
[^:\s]+:?\s* # (header name followed by optional colon)
"(?<regex>[^"]*)" # "REGEX"
\s+->\s+ # ->
"(?<replacement>[^"]*)" # "REPLACEMENT"
$
"#
)
.captures(header)?;
let regex = captures["regex"].to_owned();
let replacement = captures["replacement"].to_owned();
Some((regex, replacement))
}

pub fn extract_llvm_version(version: &str) -> Option<u32> {
Expand Down
66 changes: 36 additions & 30 deletions src/tools/compiletest/src/header/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use std::path::Path;
use std::str::FromStr;

use crate::common::{Config, Debugger, Mode};
use crate::header::{parse_normalization_string, EarlyProps, HeadersCache};
use crate::header::{parse_normalize_rule, EarlyProps, HeadersCache};

use super::iter_header;

Expand Down Expand Up @@ -32,35 +32,41 @@ fn make_test_description<R: Read>(
}

#[test]
fn test_parse_normalization_string() {
let mut s = "normalize-stderr-32bit: \"something (32 bits)\" -> \"something ($WORD bits)\".";
let first = parse_normalization_string(&mut s);
assert_eq!(first, Some("something (32 bits)".to_owned()));
assert_eq!(s, " -> \"something ($WORD bits)\".");

// Nothing to normalize (No quotes)
let mut s = "normalize-stderr-32bit: something (32 bits) -> something ($WORD bits).";
let first = parse_normalization_string(&mut s);
assert_eq!(first, None);
assert_eq!(s, r#"normalize-stderr-32bit: something (32 bits) -> something ($WORD bits)."#);

// Nothing to normalize (Only a single quote)
let mut s = "normalize-stderr-32bit: \"something (32 bits) -> something ($WORD bits).";
let first = parse_normalization_string(&mut s);
assert_eq!(first, None);
assert_eq!(s, "normalize-stderr-32bit: \"something (32 bits) -> something ($WORD bits).");

// Nothing to normalize (Three quotes)
let mut s = "normalize-stderr-32bit: \"something (32 bits)\" -> \"something ($WORD bits).";
let first = parse_normalization_string(&mut s);
assert_eq!(first, Some("something (32 bits)".to_owned()));
assert_eq!(s, " -> \"something ($WORD bits).");

// Nothing to normalize (No quotes, 16-bit)
let mut s = "normalize-stderr-16bit: something (16 bits) -> something ($WORD bits).";
let first = parse_normalization_string(&mut s);
assert_eq!(first, None);
assert_eq!(s, r#"normalize-stderr-16bit: something (16 bits) -> something ($WORD bits)."#);
fn test_parse_normalize_rule() {
let good_data = &[
(
r#"normalize-stderr-32bit: "something (32 bits)" -> "something ($WORD bits)""#,
"something (32 bits)",
"something ($WORD bits)",
),
// FIXME(#126370): A colon after the header name should be mandatory,
// but currently is not, and there are many tests that lack the colon.
(
r#"normalize-stderr-32bit "something (32 bits)" -> "something ($WORD bits)""#,
"something (32 bits)",
"something ($WORD bits)",
),
];

for &(input, expected_regex, expected_replacement) in good_data {
let parsed = parse_normalize_rule(input);
let parsed =
parsed.as_ref().map(|(regex, replacement)| (regex.as_str(), replacement.as_str()));
assert_eq!(parsed, Some((expected_regex, expected_replacement)));
}

let bad_data = &[
r#"normalize-stderr-16bit: something (16 bits) -> something ($WORD bits)"#,
r#"normalize-stderr-32bit: something (32 bits) -> something ($WORD bits)"#,
r#"normalize-stderr-32bit: "something (32 bits) -> something ($WORD bits)"#,
r#"normalize-stderr-32bit: "something (32 bits)" -> "something ($WORD bits)"#,
r#"normalize-stderr-32bit: "something (32 bits)" -> "something ($WORD bits)"."#,
];

for &input in bad_data {
let parsed = parse_normalize_rule(input);
assert_eq!(parsed, None);
}
}

#[derive(Default)]
Expand Down
10 changes: 1 addition & 9 deletions src/tools/compiletest/src/runtest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ use crate::errors::{self, Error, ErrorKind};
use crate::header::TestProps;
use crate::json;
use crate::read2::{read2_abbreviated, Truncated};
use crate::util::{add_dylib_path, copy_dir_all, dylib_env_var, logv, PathBufExt};
use crate::util::{add_dylib_path, copy_dir_all, dylib_env_var, logv, static_regex, PathBufExt};
use crate::ColorConfig;
use colored::Colorize;
use miropt_test_tools::{files_for_miropt_test, MiroptTest, MiroptTestFile};
Expand Down Expand Up @@ -48,14 +48,6 @@ use debugger::DebuggerCommands;
#[cfg(test)]
mod tests;

macro_rules! static_regex {
($re:literal) => {{
static RE: ::std::sync::OnceLock<::regex::Regex> = ::std::sync::OnceLock::new();
RE.get_or_init(|| ::regex::Regex::new($re).unwrap())
}};
}
use static_regex;

const FAKE_SRC_BASE: &str = "fake-test-src-base";

#[cfg(windows)]
Expand Down
3 changes: 2 additions & 1 deletion src/tools/compiletest/src/runtest/coverage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@ use std::process::Command;
use glob::glob;

use crate::common::{UI_COVERAGE, UI_COVERAGE_MAP};
use crate::runtest::{static_regex, Emit, ProcRes, TestCx, WillExecute};
use crate::runtest::{Emit, ProcRes, TestCx, WillExecute};
use crate::util::static_regex;

impl<'test> TestCx<'test> {
fn coverage_dump_path(&self) -> &Path {
Expand Down
8 changes: 8 additions & 0 deletions src/tools/compiletest/src/util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -90,3 +90,11 @@ pub fn copy_dir_all(src: impl AsRef<Path>, dst: impl AsRef<Path>) -> std::io::Re
}
Ok(())
}

macro_rules! static_regex {
($re:literal) => {{
static RE: ::std::sync::OnceLock<::regex::Regex> = ::std::sync::OnceLock::new();
RE.get_or_init(|| ::regex::Regex::new($re).unwrap())
}};
}
pub(crate) use static_regex;

0 comments on commit b533f6c

Please sign in to comment.