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

Add {ignore,needs}-{rustc,std}-debug-assertions directive support #131913

Merged
merged 3 commits into from
Nov 8, 2024
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
10 changes: 7 additions & 3 deletions src/bootstrap/src/core/build_steps/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1933,9 +1933,13 @@ NOTE: if you're sure you want to do this, please open an issue as to why. In the

cmd.arg("--json");

if builder.config.rust_debug_assertions_std {
cmd.arg("--with-debug-assertions");
};
if builder.config.rustc_debug_assertions {
cmd.arg("--with-rustc-debug-assertions");
}

if builder.config.std_debug_assertions {
cmd.arg("--with-std-debug-assertions");
}

let mut llvm_components_passed = false;
let mut copts_passed = false;
Expand Down
4 changes: 2 additions & 2 deletions src/bootstrap/src/core/builder/cargo.rs
Original file line number Diff line number Diff line change
Expand Up @@ -833,9 +833,9 @@ impl Builder<'_> {
cargo.env(
profile_var("DEBUG_ASSERTIONS"),
if mode == Mode::Std {
self.config.rust_debug_assertions_std.to_string()
self.config.std_debug_assertions.to_string()
} else {
self.config.rust_debug_assertions.to_string()
self.config.rustc_debug_assertions.to_string()
},
);
cargo.env(
Expand Down
33 changes: 17 additions & 16 deletions src/bootstrap/src/core/config/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -263,8 +263,10 @@ pub struct Config {
pub rust_optimize: RustOptimize,
pub rust_codegen_units: Option<u32>,
pub rust_codegen_units_std: Option<u32>,
pub rust_debug_assertions: bool,
pub rust_debug_assertions_std: bool,

pub rustc_debug_assertions: bool,
pub std_debug_assertions: bool,

pub rust_overflow_checks: bool,
pub rust_overflow_checks_std: bool,
pub rust_debug_logging: bool,
Expand Down Expand Up @@ -1115,9 +1117,9 @@ define_config! {
debug: Option<bool> = "debug",
codegen_units: Option<u32> = "codegen-units",
codegen_units_std: Option<u32> = "codegen-units-std",
debug_assertions: Option<bool> = "debug-assertions",
rustc_debug_assertions: Option<bool> = "debug-assertions",
randomize_layout: Option<bool> = "randomize-layout",
debug_assertions_std: Option<bool> = "debug-assertions-std",
std_debug_assertions: Option<bool> = "debug-assertions-std",
Comment on lines -1118 to +1122
Copy link
Member Author

Choose a reason for hiding this comment

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

I did not change the config.toml user-facing toml name.

overflow_checks: Option<bool> = "overflow-checks",
overflow_checks_std: Option<bool> = "overflow-checks-std",
debug_logging: Option<bool> = "debug-logging",
Expand Down Expand Up @@ -1652,8 +1654,8 @@ impl Config {
let mut llvm_offload = None;
let mut llvm_plugins = None;
let mut debug = None;
let mut debug_assertions = None;
let mut debug_assertions_std = None;
let mut rustc_debug_assertions = None;
let mut std_debug_assertions = None;
let mut overflow_checks = None;
let mut overflow_checks_std = None;
let mut debug_logging = None;
Expand All @@ -1675,8 +1677,8 @@ impl Config {
debug: debug_toml,
codegen_units,
codegen_units_std,
debug_assertions: debug_assertions_toml,
debug_assertions_std: debug_assertions_std_toml,
rustc_debug_assertions: rustc_debug_assertions_toml,
std_debug_assertions: std_debug_assertions_toml,
overflow_checks: overflow_checks_toml,
overflow_checks_std: overflow_checks_std_toml,
debug_logging: debug_logging_toml,
Expand Down Expand Up @@ -1734,8 +1736,8 @@ impl Config {
config.download_ci_rustc_commit(download_rustc, config.llvm_assertions);

debug = debug_toml;
debug_assertions = debug_assertions_toml;
debug_assertions_std = debug_assertions_std_toml;
rustc_debug_assertions = rustc_debug_assertions_toml;
std_debug_assertions = std_debug_assertions_toml;
overflow_checks = overflow_checks_toml;
overflow_checks_std = overflow_checks_std_toml;
debug_logging = debug_logging_toml;
Expand Down Expand Up @@ -2148,14 +2150,13 @@ impl Config {
config.rust_std_features = std_features.unwrap_or(default_std_features);

let default = debug == Some(true);
config.rust_debug_assertions = debug_assertions.unwrap_or(default);
config.rust_debug_assertions_std =
debug_assertions_std.unwrap_or(config.rust_debug_assertions);
config.rustc_debug_assertions = rustc_debug_assertions.unwrap_or(default);
config.std_debug_assertions = std_debug_assertions.unwrap_or(config.rustc_debug_assertions);
config.rust_overflow_checks = overflow_checks.unwrap_or(default);
config.rust_overflow_checks_std =
overflow_checks_std.unwrap_or(config.rust_overflow_checks);

config.rust_debug_logging = debug_logging.unwrap_or(config.rust_debug_assertions);
config.rust_debug_logging = debug_logging.unwrap_or(config.rustc_debug_assertions);

let with_defaults = |debuginfo_level_specific: Option<_>| {
debuginfo_level_specific.or(debuginfo_level).unwrap_or(if debug == Some(true) {
Expand Down Expand Up @@ -3075,8 +3076,8 @@ fn check_incompatible_options_for_ci_rustc(
debug: _,
codegen_units: _,
codegen_units_std: _,
debug_assertions: _,
debug_assertions_std: _,
rustc_debug_assertions: _,
std_debug_assertions: _,
overflow_checks: _,
overflow_checks_std: _,
debuginfo_level: _,
Expand Down
7 changes: 5 additions & 2 deletions src/tools/compiletest/src/common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -236,8 +236,11 @@ pub struct Config {
/// Run ignored tests
pub run_ignored: bool,

/// Whether to run tests with `ignore-debug` header
pub with_debug_assertions: bool,
/// Whether rustc was built with debug assertions.
pub with_rustc_debug_assertions: bool,

/// Whether std was built with debug assertions.
pub with_std_debug_assertions: bool,

/// Only run tests that match these filters
pub filters: Vec<String>,
Expand Down
5 changes: 4 additions & 1 deletion src/tools/compiletest/src/directive-list.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@ const KNOWN_DIRECTIVE_NAMES: &[&str] = &[
"ignore-coverage-map",
"ignore-coverage-run",
"ignore-cross-compile",
"ignore-debug",
"ignore-eabi",
"ignore-emscripten",
"ignore-endian-big",
Expand Down Expand Up @@ -81,13 +80,15 @@ const KNOWN_DIRECTIVE_NAMES: &[&str] = &[
"ignore-powerpc",
"ignore-remote",
"ignore-riscv64",
"ignore-rustc-debug-assertions",
"ignore-s390x",
"ignore-sgx",
"ignore-sparc64",
"ignore-spirv",
"ignore-stable",
"ignore-stage1",
"ignore-stage2",
"ignore-std-debug-assertions",
"ignore-test",
"ignore-thumb",
"ignore-thumbv8m.base-none-eabi",
Expand Down Expand Up @@ -134,6 +135,7 @@ const KNOWN_DIRECTIVE_NAMES: &[&str] = &[
"needs-relocation-model-pic",
"needs-run-enabled",
"needs-rust-lld",
"needs-rustc-debug-assertions",
"needs-sanitizer-address",
"needs-sanitizer-cfi",
"needs-sanitizer-dataflow",
Expand All @@ -146,6 +148,7 @@ const KNOWN_DIRECTIVE_NAMES: &[&str] = &[
"needs-sanitizer-shadow-call-stack",
"needs-sanitizer-support",
"needs-sanitizer-thread",
"needs-std-debug-assertions",
"needs-symlink",
"needs-threads",
"needs-unwind",
Expand Down
11 changes: 8 additions & 3 deletions src/tools/compiletest/src/header/cfg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -202,9 +202,14 @@ pub(super) fn parse_cfg_name_directive<'a>(
message: "when running tests remotely",
}
condition! {
name: "debug",
condition: config.with_debug_assertions,
message: "when running tests with `ignore-debug` header",
name: "rustc-debug-assertions",
condition: config.with_rustc_debug_assertions,
message: "when rustc is built with debug assertions",
}
condition! {
name: "std-debug-assertions",
condition: config.with_std_debug_assertions,
message: "when std is built with debug assertions",
}
condition! {
name: config.debugger.as_ref().map(|d| d.to_str()),
Expand Down
10 changes: 10 additions & 0 deletions src/tools/compiletest/src/header/needs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,16 @@ pub(super) fn handle_needs(
condition: cache.llvm_zstd,
ignore_reason: "ignored if LLVM wasn't build with zstd for ELF section compression",
},
Need {
name: "needs-rustc-debug-assertions",
condition: config.with_rustc_debug_assertions,
ignore_reason: "ignored if rustc wasn't built with debug assertions",
},
Need {
name: "needs-std-debug-assertions",
condition: config.with_std_debug_assertions,
ignore_reason: "ignored if std wasn't built with debug assertions",
},
];

let (name, comment) = match ln.split_once([':', ' ']) {
Expand Down
44 changes: 44 additions & 0 deletions src/tools/compiletest/src/header/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,8 @@ struct ConfigBuilder {
git_hash: bool,
system_llvm: bool,
profiler_runtime: bool,
rustc_debug_assertions: bool,
std_debug_assertions: bool,
}

impl ConfigBuilder {
Expand Down Expand Up @@ -122,6 +124,16 @@ impl ConfigBuilder {
self
}

fn rustc_debug_assertions(&mut self, is_enabled: bool) -> &mut Self {
self.rustc_debug_assertions = is_enabled;
self
}

fn std_debug_assertions(&mut self, is_enabled: bool) -> &mut Self {
self.std_debug_assertions = is_enabled;
self
}

fn build(&mut self) -> Config {
let args = &[
"compiletest",
Expand Down Expand Up @@ -169,6 +181,12 @@ impl ConfigBuilder {
if self.profiler_runtime {
args.push("--profiler-runtime".to_owned());
}
if self.rustc_debug_assertions {
args.push("--with-rustc-debug-assertions".to_owned());
}
if self.std_debug_assertions {
args.push("--with-std-debug-assertions".to_owned());
}

args.push("--rustc-path".to_string());
// This is a subtle/fragile thing. On rust-lang CI, there is no global
Expand Down Expand Up @@ -313,6 +331,32 @@ fn only_target() {
assert!(!check_ignore(&config, "//@ only-64bit"));
}

#[test]
fn rustc_debug_assertions() {
let config: Config = cfg().rustc_debug_assertions(false).build();

assert!(check_ignore(&config, "//@ needs-rustc-debug-assertions"));
assert!(!check_ignore(&config, "//@ ignore-rustc-debug-assertions"));

let config: Config = cfg().rustc_debug_assertions(true).build();

assert!(!check_ignore(&config, "//@ needs-rustc-debug-assertions"));
assert!(check_ignore(&config, "//@ ignore-rustc-debug-assertions"));
}

#[test]
fn std_debug_assertions() {
let config: Config = cfg().std_debug_assertions(false).build();

assert!(check_ignore(&config, "//@ needs-std-debug-assertions"));
assert!(!check_ignore(&config, "//@ ignore-std-debug-assertions"));

let config: Config = cfg().std_debug_assertions(true).build();

assert!(!check_ignore(&config, "//@ needs-std-debug-assertions"));
assert!(check_ignore(&config, "//@ ignore-std-debug-assertions"));
}

#[test]
fn stage() {
let config: Config = cfg().stage_id("stage1-x86_64-unknown-linux-gnu").build();
Expand Down
9 changes: 6 additions & 3 deletions src/tools/compiletest/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,8 @@ pub fn parse_config(args: Vec<String>) -> Config {
.optopt("", "run", "whether to execute run-* tests", "auto | always | never")
.optflag("", "ignored", "run tests marked as ignored")
.optflag("", "has-enzyme", "run tests that require enzyme")
.optflag("", "with-debug-assertions", "whether to run tests with `ignore-debug` header")
.optflag("", "with-rustc-debug-assertions", "whether rustc was built with debug assertions")
.optflag("", "with-std-debug-assertions", "whether std was built with debug assertions")
.optmulti(
"",
"skip",
Expand Down Expand Up @@ -234,7 +235,8 @@ pub fn parse_config(args: Vec<String>) -> Config {

let src_base = opt_path(matches, "src-base");
let run_ignored = matches.opt_present("ignored");
let with_debug_assertions = matches.opt_present("with-debug-assertions");
let with_rustc_debug_assertions = matches.opt_present("with-rustc-debug-assertions");
let with_std_debug_assertions = matches.opt_present("with-std-debug-assertions");
let mode = matches.opt_str("mode").unwrap().parse().expect("invalid mode");
let has_html_tidy = if mode == Mode::Rustdoc {
Command::new("tidy")
Expand Down Expand Up @@ -292,7 +294,8 @@ pub fn parse_config(args: Vec<String>) -> Config {
suite: matches.opt_str("suite").unwrap(),
debugger: None,
run_ignored,
with_debug_assertions,
with_rustc_debug_assertions,
with_std_debug_assertions,
filters,
skip: matches.opt_strs("skip"),
filter_exact: matches.opt_present("exact"),
Expand Down
2 changes: 1 addition & 1 deletion tests/codegen/binary-heap-peek-mut-pop-no-panic.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
//@ compile-flags: -O
//@ ignore-debug
//@ ignore-std-debug-assertions
#![crate_type = "lib"]

use std::collections::binary_heap::PeekMut;
Expand Down
3 changes: 2 additions & 1 deletion tests/codegen/mem-replace-big-type.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@
// known to be `1` after inlining).

//@ compile-flags: -C no-prepopulate-passes -Zinline-mir=no
//@ ignore-debug: precondition checks in ptr::read make them a bad candidate for MIR inlining
//@ ignore-std-debug-assertions
// Reason: precondition checks in ptr::read make them a bad candidate for MIR inlining
Comment on lines -7 to +8
Copy link
Member

Choose a reason for hiding this comment

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

Can't the comment format be the same (without newline) as it was before? Or is it due to the tidy length check?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh it definitely can be, but it gets quite long and I found it a bit easier to read on a different line.

Copy link
Member Author

Choose a reason for hiding this comment

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

(and yes it would hit the tidy line length check lol)

//@ needs-deterministic-layouts

#![crate_type = "lib"]
Expand Down
3 changes: 2 additions & 1 deletion tests/codegen/mem-replace-simple-type.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
//@ compile-flags: -O -C no-prepopulate-passes
//@ only-x86_64 (to not worry about usize differing)
//@ ignore-debug: precondition checks make mem::replace not a candidate for MIR inlining
Copy link
Member

Choose a reason for hiding this comment

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

Just to check: we have some lint or w/e that will fail CI if someone adds a test with ignore-debug as we merge this PR?

Copy link
Member Author

@jieyouxu jieyouxu Oct 20, 2024

Choose a reason for hiding this comment

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

compiletest (currently) has an allowlist of known directives, I've removed ignore-debug from that so compiletest will error if someone writes ignore-debug (should be in the first commit) which will fail locally and in CI. (This is current stopgap solution before fixing how directives are handled)

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

It will fail looking like (I know it's not super user friendly)

Testing stage1 compiletest suite=ui mode=ui (x86_64-pc-windows-msvc)
error: detected unknown compiletest test directive `only-debug` in X:\repos\rust\tests\ui\print_type_sizes\niche-filling.rs:13
errors encountered during EarlyProps parsing: X:\repos\rust\tests\ui\print_type_sizes\niche-filling.rs
thread 'main' panicked at src\tools\compiletest\src\header.rs:68:13:
errors encountered during EarlyProps parsing
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

//@ ignore-std-debug-assertions
// Reason: precondition checks make mem::replace not a candidate for MIR inlining

#![crate_type = "lib"]

Expand Down
2 changes: 1 addition & 1 deletion tests/codegen/slice-reverse.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
//@ compile-flags: -O
//@ only-x86_64
//@ ignore-debug: debug assertions prevent generating shufflevector
//@ ignore-std-debug-assertions (debug assertions prevent generating shufflevector)

#![crate_type = "lib"]

Expand Down
2 changes: 1 addition & 1 deletion tests/codegen/vec-in-place.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
//@ ignore-debug: FIXME: checks for call detect scoped noalias metadata
//@ ignore-std-debug-assertions (FIXME: checks for call detect scoped noalias metadata)
//@ compile-flags: -O -Z merge-functions=disabled
#![crate_type = "lib"]

Expand Down
2 changes: 1 addition & 1 deletion tests/codegen/vec-shrink-panik.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// LLVM 17 realizes double panic is not possible and doesn't generate calls
// to panic_cannot_unwind.
//@ compile-flags: -O
//@ ignore-debug: plain old debug assertions
//@ ignore-std-debug-assertions (plain old debug assertions)
//@ needs-unwind
#![crate_type = "lib"]
#![feature(shrink_to)]
Expand Down
2 changes: 1 addition & 1 deletion tests/codegen/vec-with-capacity.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
//@ compile-flags: -O
//@ ignore-debug
//@ ignore-std-debug-assertions
// (with debug assertions turned on, `assert_unchecked` generates a real assertion)

#![crate_type = "lib"]
Expand Down
2 changes: 1 addition & 1 deletion tests/codegen/vecdeque-drain.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

//@ compile-flags: -O
//@ needs-deterministic-layouts
//@ ignore-debug: FIXME: checks for call detect scoped noalias metadata
//@ ignore-std-debug-assertions (FIXME: checks for call detect scoped noalias metadata)

#![crate_type = "lib"]

Expand Down
2 changes: 1 addition & 1 deletion tests/codegen/vecdeque_no_panic.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// This test checks that `VecDeque::front[_mut]()` and `VecDeque::back[_mut]()` can't panic.

//@ compile-flags: -O
//@ ignore-debug: plain old debug assertions
//@ ignore-std-debug-assertions (plain old debug assertions)

#![crate_type = "lib"]

Expand Down
3 changes: 2 additions & 1 deletion tests/mir-opt/pre-codegen/mem_replace.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// skip-filecheck
//@ compile-flags: -O -C debuginfo=0 -Zmir-opt-level=2 -Zinline-mir
//@ ignore-debug: precondition checks on ptr::read/write are under cfg(debug_assertions)
//@ ignore-std-debug-assertions
// Reason: precondition checks on ptr::read/write are under cfg(debug_assertions)
// EMIT_MIR_FOR_EACH_PANIC_STRATEGY

#![crate_type = "lib"]
Expand Down
2 changes: 1 addition & 1 deletion tests/mir-opt/pre-codegen/ptr_offset.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// skip-filecheck
//@ compile-flags: -O -C debuginfo=0 -Zmir-opt-level=2 -Zinline-mir
//@ ignore-debug: precondition checks are under cfg(debug_assertions)
//@ ignore-std-debug-assertions (precondition checks are under cfg(debug_assertions))
// EMIT_MIR_FOR_EACH_PANIC_STRATEGY

#![crate_type = "lib"]
Expand Down
Loading
Loading