Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convert run-make/coverage-reports tests to use a custom compiletest mode #112300

Merged
merged 12 commits into from
Jun 30, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/bootstrap/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -686,6 +686,7 @@ impl<'a> Builder<'a> {
test::Tidy,
test::Ui,
test::RunPassValgrind,
test::RunCoverage,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't put much thought into the relative ordering of these new suites, so their placement is somewhat arbitrary.

test::MirOpt,
test::Codegen,
test::CodegenUnits,
Expand All @@ -694,6 +695,7 @@ impl<'a> Builder<'a> {
test::Debuginfo,
test::UiFullDeps,
test::Rustdoc,
test::RunCoverageRustdoc,
test::Pretty,
test::Crate,
test::CrateLibrustc,
Expand Down
20 changes: 16 additions & 4 deletions src/bootstrap/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1319,6 +1319,13 @@ host_test!(RunMakeFullDeps {

default_test!(Assembly { path: "tests/assembly", mode: "assembly", suite: "assembly" });

host_test!(RunCoverage { path: "tests/run-coverage", mode: "run-coverage", suite: "run-coverage" });
host_test!(RunCoverageRustdoc {
path: "tests/run-coverage-rustdoc",
mode: "run-coverage",
suite: "run-coverage-rustdoc"
});

Comment on lines +1322 to +1328
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not fully clear on the differences between default_test! and host_test!, but since the tests are skipped on cross-compile targets anyway I assume it doesn't matter much right now.

(It should be possible to run coverage tests on cross-compile targets, but the cross-runner would need to be taught how to send back the resulting .profraw files to be processed by the host.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also don't have strong feelings on the name of the extra suite; I was wavering between run-coverage-doctest, run-coverage-doctests, and run-coverage-rustdoc.

(If there are ever other run-coverage tests with extra tool dependencies, it would probably be fine to group them all into one extra suite with a more generic name.)

// For the mir-opt suite we do not use macros, as we need custom behavior when blessing.
#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)]
pub struct MirOpt {
Expand Down Expand Up @@ -1503,6 +1510,7 @@ note: if you're sure you want to do this, please open an issue as to why. In the
|| (mode == "ui" && is_rustdoc)
|| mode == "js-doc-test"
|| mode == "rustdoc-json"
|| suite == "run-coverage-rustdoc"
{
cmd.arg("--rustdoc-path").arg(builder.rustdoc(compiler));
}
Expand All @@ -1516,7 +1524,7 @@ note: if you're sure you want to do this, please open an issue as to why. In the
.arg(builder.ensure(tool::JsonDocLint { compiler: json_compiler, target }));
}

if mode == "run-make" {
if mode == "run-make" || mode == "run-coverage" {
let rust_demangler = builder
.ensure(tool::RustDemangler {
compiler,
Expand Down Expand Up @@ -1703,17 +1711,21 @@ note: if you're sure you want to do this, please open an issue as to why. In the
add_link_lib_path(vec![llvm_libdir.trim().into()], &mut cmd);
}

// Only pass correct values for these flags for the `run-make` suite as it
// requires that a C++ compiler was configured which isn't always the case.
if !builder.config.dry_run() && matches!(suite, "run-make" | "run-make-fulldeps") {
if !builder.config.dry_run()
Zalathar marked this conversation as resolved.
Show resolved Hide resolved
&& (matches!(suite, "run-make" | "run-make-fulldeps") || mode == "run-coverage")
{
// The llvm/bin directory contains many useful cross-platform
// tools. Pass the path to run-make tests so they can use them.
// (The run-coverage tests also need these tools to process
// coverage reports.)
let llvm_bin_path = llvm_config
.parent()
.expect("Expected llvm-config to be contained in directory");
assert!(llvm_bin_path.is_dir());
cmd.arg("--llvm-bin-dir").arg(llvm_bin_path);
}

if !builder.config.dry_run() && matches!(suite, "run-make" | "run-make-fulldeps") {
Comment on lines +1726 to +1728
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new mode/suites don't need LLD, so I've split this off into a separate conditional that only checks for the run-make suites.

// If LLD is available, add it to the PATH
if builder.config.lld_enabled {
let lld_install_root =
Expand Down
3 changes: 3 additions & 0 deletions src/tools/compiletest/src/common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ string_enum! {
JsDocTest => "js-doc-test",
MirOpt => "mir-opt",
Assembly => "assembly",
RunCoverage => "run-coverage",
}
}

Expand Down Expand Up @@ -626,6 +627,7 @@ pub const UI_EXTENSIONS: &[&str] = &[
UI_STDERR_64,
UI_STDERR_32,
UI_STDERR_16,
UI_COVERAGE,
];
pub const UI_STDERR: &str = "stderr";
pub const UI_STDOUT: &str = "stdout";
Expand All @@ -635,6 +637,7 @@ pub const UI_RUN_STDOUT: &str = "run.stdout";
pub const UI_STDERR_64: &str = "64bit.stderr";
pub const UI_STDERR_32: &str = "32bit.stderr";
pub const UI_STDERR_16: &str = "16bit.stderr";
pub const UI_COVERAGE: &str = "coverage";

/// Absolute path to the directory where all output for all tests in the given
/// `relative_dir` group should reside. Example:
Expand Down
48 changes: 40 additions & 8 deletions src/tools/compiletest/src/header.rs
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ pub struct TestProps {
// customized normalization rules
pub normalize_stdout: Vec<(String, String)>,
pub normalize_stderr: Vec<(String, String)>,
pub failure_status: i32,
pub failure_status: Option<i32>,
// For UI tests, allows compiler to exit with arbitrary failure status
pub dont_check_failure_status: bool,
// Whether or not `rustfix` should apply the `CodeSuggestion`s of this test and compile the
Expand Down Expand Up @@ -257,7 +257,7 @@ impl TestProps {
check_test_line_numbers_match: false,
normalize_stdout: vec![],
normalize_stderr: vec![],
failure_status: -1,
failure_status: None,
dont_check_failure_status: false,
run_rustfix: false,
rustfix_only_machine_applicable: false,
Expand Down Expand Up @@ -428,7 +428,7 @@ impl TestProps {
.parse_name_value_directive(ln, FAILURE_STATUS)
.and_then(|code| code.trim().parse::<i32>().ok())
{
self.failure_status = code;
self.failure_status = Some(code);
}

config.set_name_directive(
Expand Down Expand Up @@ -491,11 +491,8 @@ impl TestProps {
});
}

if self.failure_status == -1 {
self.failure_status = 1;
}
if self.should_ice {
self.failure_status = 101;
self.failure_status = Some(101);
}

if config.mode == Mode::Incremental {
Expand Down Expand Up @@ -615,10 +612,25 @@ pub fn line_directive<'line>(
}

fn iter_header<R: Read>(testfile: &Path, rdr: R, it: &mut dyn FnMut(Option<&str>, &str, usize)) {
iter_header_extra(testfile, rdr, &[], it)
}

fn iter_header_extra(
testfile: &Path,
rdr: impl Read,
extra_directives: &[&str],
it: &mut dyn FnMut(Option<&str>, &str, usize),
) {
if testfile.is_dir() {
return;
}

// Process any extra directives supplied by the caller (e.g. because they
// are implied by the test mode), with a dummy line number of 0.
for directive in extra_directives {
it(None, directive, 0);
}

let comment = if testfile.extension().map(|e| e == "rs") == Some(true) { "//" } else { "#" };

let mut rdr = BufReader::new(rdr);
Expand Down Expand Up @@ -897,7 +909,27 @@ pub fn make_test_description<R: Read>(
let mut ignore_message = None;
let mut should_fail = false;

iter_header(path, src, &mut |revision, ln, line_number| {
let extra_directives: &[&str] = match config.mode {
// The run-coverage tests are treated as having these extra directives,
// without needing to specify them manually in every test file.
// (Some of the comments below have been copied over from
// `tests/run-make/coverage-reports/Makefile`, which no longer exists.)
Mode::RunCoverage => {
&[
"needs-profiler-support",
// FIXME(mati865): MinGW GCC miscompiles compiler-rt profiling library but with Clang it works
// properly. Since we only have GCC on the CI ignore the test for now.
"ignore-windows-gnu",
// FIXME(pietroalbini): this test currently does not work on cross-compiled
// targets because remote-test is not capable of sending back the *.profraw
// files generated by the LLVM instrumentation.
"ignore-cross-compile",
Comment on lines +920 to +926
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for including these FIXMEs here, that will make it easier to understand in the future when they can be removed!

]
}
_ => &[],
};

iter_header_extra(path, src, extra_directives, &mut |revision, ln, line_number| {
if revision.is_some() && revision != cfg {
return;
}
Expand Down
4 changes: 3 additions & 1 deletion src/tools/compiletest/src/header/needs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ pub(super) fn handle_needs(
},
Need {
name: "needs-profiler-support",
condition: std::env::var_os("RUSTC_PROFILER_SUPPORT").is_some(),
condition: cache.profiler_support,
ignore_reason: "ignored when profiler support is disabled",
},
Need {
Expand Down Expand Up @@ -195,6 +195,7 @@ pub(super) struct CachedNeedsConditions {
sanitizer_memtag: bool,
sanitizer_shadow_call_stack: bool,
sanitizer_safestack: bool,
profiler_support: bool,
xray: bool,
rust_lld: bool,
i686_dlltool: bool,
Expand Down Expand Up @@ -232,6 +233,7 @@ impl CachedNeedsConditions {
sanitizer_memtag: util::MEMTAG_SUPPORTED_TARGETS.contains(target),
sanitizer_shadow_call_stack: util::SHADOWCALLSTACK_SUPPORTED_TARGETS.contains(target),
sanitizer_safestack: util::SAFESTACK_SUPPORTED_TARGETS.contains(target),
profiler_support: std::env::var_os("RUSTC_PROFILER_SUPPORT").is_some(),
xray: util::XRAY_SUPPORTED_TARGETS.contains(target),

// For tests using the `needs-rust-lld` directive (e.g. for `-Zgcc-ld=lld`), we need to find
Expand Down
Loading