Skip to content

Commit

Permalink
Rollup merge of #131351 - jieyouxu:yeet-the-valgrind, r=Kobzol
Browse files Browse the repository at this point in the history
Remove valgrind test suite and support from compiletest, bootstrap and opt-dist

The `run-pass-valgrind` test suite is not exercised in CI, and as far as I'm aware nobody runs it (asked in https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/Are.20the.20valgrind.20tests.20even.20used.20by.20anyone.3F). What's remaining of valgrind support in compiletest isn't even properly hooked up with bootstrap.

The existing valgrind logic in compiletest is also straight up questionable, i.e.

https://github.com/rust-lang/rust/blob/1b3b8e7b0265162853c650ead09905bc3cdaeae9/src/tools/compiletest/src/runtest/valgrind.rs#L7-L12

It just runs valgrind tests as `rpass` if no valgrind path is provided to compiletest from bootstrap -- but bootstrap doesn't even pass a valgrind path to compiletest in the first place, so this always ran as `rpass` tests. So what is this even testing?

So if it's not testing anything, let's delete it.

Closes #44816 by deleting the test suite :3

<img src="https://github.com/user-attachments/assets/99525bf7-e85b-40ba-9281-e4e1e275c4e8" width=300 />
  • Loading branch information
workingjubilee authored Oct 7, 2024
2 parents 9c4732a + fa3c25e commit f88bfa3
Show file tree
Hide file tree
Showing 25 changed files with 29 additions and 619 deletions.
6 changes: 0 additions & 6 deletions src/bootstrap/src/core/build_steps/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1394,12 +1394,6 @@ default_test!(Ui { path: "tests/ui", mode: "ui", suite: "ui" });

default_test!(Crashes { path: "tests/crashes", mode: "crashes", suite: "crashes" });

default_test!(RunPassValgrind {
path: "tests/run-pass-valgrind",
mode: "run-pass-valgrind",
suite: "run-pass-valgrind"
});

default_test!(Codegen { path: "tests/codegen", mode: "codegen", suite: "codegen" });

default_test!(CodegenUnits {
Expand Down
2 changes: 0 additions & 2 deletions src/bootstrap/src/core/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -327,7 +327,6 @@ const PATH_REMAP: &[(&str, &[&str])] = &[
"tests/mir-opt",
"tests/pretty",
"tests/run-make",
"tests/run-pass-valgrind",
"tests/rustdoc",
"tests/rustdoc-gui",
"tests/rustdoc-js",
Expand Down Expand Up @@ -852,7 +851,6 @@ impl<'a> Builder<'a> {
test::Tidy,
test::Ui,
test::Crashes,
test::RunPassValgrind,
test::Coverage,
test::CoverageMap,
test::CoverageRun,
Expand Down
8 changes: 0 additions & 8 deletions src/tools/compiletest/src/common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,6 @@ macro_rules! string_enum {
string_enum! {
#[derive(Clone, Copy, PartialEq, Debug)]
pub enum Mode {
RunPassValgrind => "run-pass-valgrind",
Pretty => "pretty",
DebugInfo => "debuginfo",
Codegen => "codegen",
Expand Down Expand Up @@ -207,13 +206,6 @@ pub struct Config {
/// Path to LLVM's bin directory.
pub llvm_bin_dir: Option<PathBuf>,

/// The valgrind path.
pub valgrind_path: Option<String>,

/// Whether to fail if we can't run run-pass-valgrind tests under valgrind
/// (or, alternatively, to silently run them like regular run-pass tests).
pub force_valgrind: bool,

/// The path to the Clang executable to run Clang-based tests with. If
/// `None` then these tests will be ignored.
pub run_clang_based_tests_with: Option<String>,
Expand Down
6 changes: 1 addition & 5 deletions src/tools/compiletest/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,6 @@ pub fn parse_config(args: Vec<String>) -> Config {
.reqopt("", "python", "path to python to use for doc tests", "PATH")
.optopt("", "jsondocck-path", "path to jsondocck to use for doc tests", "PATH")
.optopt("", "jsondoclint-path", "path to jsondoclint to use for doc tests", "PATH")
.optopt("", "valgrind-path", "path to Valgrind executable for Valgrind tests", "PROGRAM")
.optflag("", "force-valgrind", "fail if Valgrind tests cannot be run under Valgrind")
.optopt("", "run-clang-based-tests-with", "path to Clang executable", "PATH")
.optopt("", "llvm-filecheck", "path to LLVM's FileCheck binary", "DIR")
.reqopt("", "src-base", "directory to scan for test files", "PATH")
Expand All @@ -65,7 +63,7 @@ pub fn parse_config(args: Vec<String>) -> Config {
"",
"mode",
"which sort of compile tests to run",
"run-pass-valgrind | pretty | debug-info | codegen | rustdoc \
"pretty | debug-info | codegen | rustdoc \
| rustdoc-json | codegen-units | incremental | run-make | ui \
| js-doc-test | mir-opt | assembly | crashes",
)
Expand Down Expand Up @@ -269,8 +267,6 @@ pub fn parse_config(args: Vec<String>) -> Config {
python: matches.opt_str("python").unwrap(),
jsondocck_path: matches.opt_str("jsondocck-path"),
jsondoclint_path: matches.opt_str("jsondoclint-path"),
valgrind_path: matches.opt_str("valgrind-path"),
force_valgrind: matches.opt_present("force-valgrind"),
run_clang_based_tests_with: matches.opt_str("run-clang-based-tests-with"),
llvm_filecheck: matches.opt_str("llvm-filecheck").map(PathBuf::from),
llvm_bin_dir: matches.opt_str("llvm-bin-dir").map(PathBuf::from),
Expand Down
4 changes: 0 additions & 4 deletions src/tools/compiletest/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,6 @@ fn main() {

let config = Arc::new(parse_config(env::args().collect()));

if config.valgrind_path.is_none() && config.force_valgrind {
panic!("Can't find Valgrind to run Valgrind tests");
}

if !config.has_tidy && config.mode == Mode::Rustdoc {
eprintln!("warning: `tidy` is not installed; diffs will not be generated");
}
Expand Down
38 changes: 4 additions & 34 deletions src/tools/compiletest/src/runtest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,9 @@ use tracing::*;
use crate::common::{
Assembly, Codegen, CodegenUnits, CompareMode, Config, CoverageMap, CoverageRun, Crashes,
DebugInfo, Debugger, FailMode, Incremental, JsDocTest, MirOpt, PassMode, Pretty, RunMake,
RunPassValgrind, Rustdoc, RustdocJson, TestPaths, UI_EXTENSIONS, UI_FIXED, UI_RUN_STDERR,
UI_RUN_STDOUT, UI_STDERR, UI_STDOUT, UI_SVG, UI_WINDOWS_SVG, Ui, expected_output_path,
incremental_dir, output_base_dir, output_base_name, output_testname_unique,
Rustdoc, RustdocJson, TestPaths, UI_EXTENSIONS, UI_FIXED, UI_RUN_STDERR, UI_RUN_STDOUT,
UI_STDERR, UI_STDOUT, UI_SVG, UI_WINDOWS_SVG, Ui, expected_output_path, incremental_dir,
output_base_dir, output_base_name, output_testname_unique,
};
use crate::compute_diff::{write_diff, write_filtered_diff};
use crate::errors::{self, Error, ErrorKind};
Expand All @@ -49,7 +49,6 @@ mod run_make;
mod rustdoc;
mod rustdoc_json;
mod ui;
mod valgrind;
// tidy-alphabet-end

#[cfg(test)]
Expand Down Expand Up @@ -253,7 +252,6 @@ impl<'test> TestCx<'test> {
self.fatal("cannot use should-ice in a test that is not cfail");
}
match self.config.mode {
RunPassValgrind => self.run_valgrind_test(),
Pretty => self.run_pretty_test(),
DebugInfo => self.run_debuginfo_test(),
Codegen => self.run_codegen_test(),
Expand Down Expand Up @@ -1500,8 +1498,7 @@ impl<'test> TestCx<'test> {
Crashes => {
set_mir_dump_dir(&mut rustc);
}
RunPassValgrind | Pretty | DebugInfo | Rustdoc | RustdocJson | RunMake
| CodegenUnits | JsDocTest => {
Pretty | DebugInfo | Rustdoc | RustdocJson | RunMake | CodegenUnits | JsDocTest => {
// do not use JSON output
}
}
Expand Down Expand Up @@ -2655,33 +2652,6 @@ impl<'test> TestCx<'test> {
}
}

// FIXME(jieyouxu): `run_rpass_test` is hoisted out here and not in incremental because
// apparently valgrind test falls back to `run_rpass_test` if valgrind isn't available, which
// seems highly questionable to me.
fn run_rpass_test(&self) {
let emit_metadata = self.should_emit_metadata(self.pass_mode());
let should_run = self.run_if_enabled();
let proc_res = self.compile_test(should_run, emit_metadata);

if !proc_res.status.success() {
self.fatal_proc_rec("compilation failed!", &proc_res);
}

// FIXME(#41968): Move this check to tidy?
if !errors::load_errors(&self.testpaths.file, self.revision).is_empty() {
self.fatal("run-pass tests with expected warnings should be moved to ui/");
}

if let WillExecute::Disabled = should_run {
return;
}

let proc_res = self.exec_compiled_test();
if !proc_res.status.success() {
self.fatal_proc_rec("test run failed!", &proc_res);
}
}

fn aggressive_rm_rf(&self, path: &Path) -> io::Result<()> {
for e in path.read_dir()? {
let entry = e?;
Expand Down
34 changes: 24 additions & 10 deletions src/tools/compiletest/src/runtest/incremental.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,6 @@
use super::{TestCx, WillExecute};
use crate::errors;

// FIXME(jieyouxu): `run_rpass_test` got hoisted out of this because apparently valgrind falls back
// to `run_rpass_test` if valgrind isn't available, which is questionable, but keeping it for
// refactoring changes to preserve current behavior.

impl TestCx<'_> {
pub(super) fn run_incremental_test(&self) {
// Basic plan for a test incremental/foo/bar.rs:
Expand Down Expand Up @@ -73,6 +69,30 @@ impl TestCx<'_> {
}
}

fn run_rpass_test(&self) {
let emit_metadata = self.should_emit_metadata(self.pass_mode());
let should_run = self.run_if_enabled();
let proc_res = self.compile_test(should_run, emit_metadata);

if !proc_res.status.success() {
self.fatal_proc_rec("compilation failed!", &proc_res);
}

// FIXME(#41968): Move this check to tidy?
if !errors::load_errors(&self.testpaths.file, self.revision).is_empty() {
self.fatal("run-pass tests with expected warnings should be moved to ui/");
}

if let WillExecute::Disabled = should_run {
return;
}

let proc_res = self.exec_compiled_test();
if !proc_res.status.success() {
self.fatal_proc_rec("test run failed!", &proc_res);
}
}

fn run_cfail_test(&self) {
let pm = self.pass_mode();
let proc_res = self.compile_test(WillExecute::No, self.should_emit_metadata(pm));
Expand Down Expand Up @@ -115,12 +135,6 @@ impl TestCx<'_> {

let proc_res = self.exec_compiled_test();

// The value our Makefile configures valgrind to return on failure
const VALGRIND_ERR: i32 = 100;
if proc_res.status.code() == Some(VALGRIND_ERR) {
self.fatal_proc_rec("run-fail test isn't valgrind-clean!", &proc_res);
}

let output_to_check = self.get_output(&proc_res);
self.check_correct_failure_status(&proc_res);
self.check_all_error_patterns(&output_to_check, &proc_res, pm);
Expand Down
34 changes: 0 additions & 34 deletions src/tools/compiletest/src/runtest/valgrind.rs

This file was deleted.

1 change: 0 additions & 1 deletion src/tools/opt-dist/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,6 @@ llvm-config = "{llvm_config}"
"tests/incremental",
"tests/mir-opt",
"tests/pretty",
"tests/run-pass-valgrind",
"tests/ui",
"tests/crashes",
];
Expand Down
34 changes: 0 additions & 34 deletions tests/run-pass-valgrind/cast-enum-with-dtor.rs

This file was deleted.

28 changes: 0 additions & 28 deletions tests/run-pass-valgrind/cleanup-auto-borrow-obj.rs

This file was deleted.

5 changes: 0 additions & 5 deletions tests/run-pass-valgrind/cleanup-stdin.rs

This file was deleted.

21 changes: 0 additions & 21 deletions tests/run-pass-valgrind/coerce-match-calls.rs

This file was deleted.

31 changes: 0 additions & 31 deletions tests/run-pass-valgrind/coerce-match.rs

This file was deleted.

Loading

0 comments on commit f88bfa3

Please sign in to comment.