From f37f3aea14ebf0bee9376a5389c8bf8021d83f5f Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Mon, 30 Sep 2019 08:48:43 -0700 Subject: [PATCH 1/3] Support rustc's `-Z panic-abort-tests` in Cargo Recently added in rust-lang/rust#64158 the `-Z panic-abort-tests` flag to the compiler itself will activate a mode in the `test` crate which enables running tests even if they're compiled with `panic=abort`. It effectively runs a test-per-process. This commit brings the same support to Cargo, adding a `-Z panic-abort-tests` flag to Cargo which allows building tests in `panic=abort` mode. While I wanted to be sure to add support for this in Cargo before we stabilize the flag in `rustc`, I don't actually know how we're going to stabilize this here. Today Cargo will automatically switch test targets to `panic=unwind`, and so if we actually were to stabilize this flag then this configuration would break: [profile.dev] panic = 'abort' In that case tests would be compiled with `panic=unwind` (due to how profiles work today) which would clash with crates also being compiled with `panic=abort`. I'm hopeful though that we can perhaps either figure out a solution for this and maybe even integrate it with the ongoing profiles work. --- src/cargo/core/compiler/mod.rs | 11 ++ src/cargo/core/compiler/unit_dependencies.rs | 2 +- src/cargo/core/features.rs | 2 + src/cargo/core/profiles.rs | 31 +++-- src/cargo/ops/cargo_compile.rs | 4 +- src/doc/src/reference/unstable.md | 14 ++ tests/testsuite/test.rs | 129 +++++++++++++++++++ 7 files changed, 182 insertions(+), 11 deletions(-) diff --git a/src/cargo/core/compiler/mod.rs b/src/cargo/core/compiler/mod.rs index 4138ad0db2a..6b159b419ae 100644 --- a/src/cargo/core/compiler/mod.rs +++ b/src/cargo/core/compiler/mod.rs @@ -799,6 +799,17 @@ fn build_base_args<'a, 'cfg>( if test && unit.target.harness() { cmd.arg("--test"); + + // Cargo has historically never compiled `--test` binaries with + // `panic=abort` because the `test` crate itself didn't support it. + // Support is now upstream, however, but requires an unstable flag to be + // passed when compiling the test. We require, in Cargo, an unstable + // flag to pass to rustc, so register that here. Eventually this flag + // will simply not be needed when the behavior is stabilized in the Rust + // compiler itself. + if *panic == PanicStrategy::Abort { + cmd.arg("-Zpanic-abort-tests"); + } } else if test { cmd.arg("--cfg").arg("test"); } diff --git a/src/cargo/core/compiler/unit_dependencies.rs b/src/cargo/core/compiler/unit_dependencies.rs index 1fb61cc5625..62cde69da0b 100644 --- a/src/cargo/core/compiler/unit_dependencies.rs +++ b/src/cargo/core/compiler/unit_dependencies.rs @@ -161,7 +161,7 @@ fn deps_of_roots<'a, 'cfg>(roots: &[Unit<'a>], mut state: &mut State<'a, 'cfg>) // without, once for `--test`). In particular, the lib included for // Doc tests and examples are `Build` mode here. let unit_for = if unit.mode.is_any_test() || state.bcx.build_config.test() { - UnitFor::new_test() + UnitFor::new_test(state.bcx.config) } else if unit.target.is_custom_build() { // This normally doesn't happen, except `clean` aggressively // generates all units. diff --git a/src/cargo/core/features.rs b/src/cargo/core/features.rs index e18b4a70acd..a5e263f409a 100644 --- a/src/cargo/core/features.rs +++ b/src/cargo/core/features.rs @@ -340,6 +340,7 @@ pub struct CliUnstable { pub build_std: Option>, pub timings: Option>, pub doctest_xcompile: bool, + pub panic_abort_tests: bool, } impl CliUnstable { @@ -399,6 +400,7 @@ impl CliUnstable { } "timings" => self.timings = Some(parse_timings(v)), "doctest-xcompile" => self.doctest_xcompile = true, + "panic-abort-tests" => self.panic_abort_tests = true, _ => failure::bail!("unknown `-Z` flag specified: {}", k), } diff --git a/src/cargo/core/profiles.rs b/src/cargo/core/profiles.rs index 5a12f954714..4d96bbffcf6 100644 --- a/src/cargo/core/profiles.rs +++ b/src/cargo/core/profiles.rs @@ -357,9 +357,10 @@ impl Profiles { Some(r) => r, }; let mut profile = maker.get_profile(Some(pkg_id), is_member, unit_for); - // `panic` should not be set for tests/benches, or any of their - // dependencies. - if !unit_for.is_panic_abort_ok() || mode.is_any_test() { + // `panic` may not be set for tests/benches, or any of their + // dependencies, so handle that here if that's what `UnitFor` tells us + // to do. + if !unit_for.is_panic_abort_ok() { profile.panic = PanicStrategy::Unwind; } @@ -901,6 +902,8 @@ impl UnitFor { pub fn new_build() -> UnitFor { UnitFor { build: true, + // Force build scripts to always use `panic=unwind` for now to + // maximally share dependencies with procedural macros. panic_abort_ok: false, } } @@ -909,22 +912,33 @@ impl UnitFor { pub fn new_compiler() -> UnitFor { UnitFor { build: false, + // Force plugins to use `panic=abort` so panics in the compiler do + // not abort the process but instead end with a reasonable error + // message that involves catching the panic in the compiler. panic_abort_ok: false, } } /// A unit for a test/bench target or their dependencies. - pub fn new_test() -> UnitFor { + /// + /// Note that `config` is taken here for unstable CLI features to detect + /// whether `panic=abort` is supported for tests. Historical versions of + /// rustc did not support this, but newer versions do with an unstable + /// compiler flag. + pub fn new_test(config: &Config) -> UnitFor { UnitFor { build: false, - panic_abort_ok: false, + panic_abort_ok: config.cli_unstable().panic_abort_tests, } } /// Creates a variant based on `for_host` setting. /// - /// When `for_host` is true, this clears `panic_abort_ok` in a sticky fashion so - /// that all its dependencies also have `panic_abort_ok=false`. + /// When `for_host` is true, this clears `panic_abort_ok` in a sticky + /// fashion so that all its dependencies also have `panic_abort_ok=false`. + /// This'll help ensure that once we start compiling for the host platform + /// (build scripts, plugins, proc macros, etc) we'll share the same build + /// graph where everything is `panic=unwind`. pub fn with_for_host(self, for_host: bool) -> UnitFor { UnitFor { build: self.build || for_host, @@ -938,7 +952,8 @@ impl UnitFor { self.build } - /// Returns `true` if this unit is allowed to set the `panic` compiler flag. + /// Returns `true` if this unit is allowed to set the `panic=abort` + /// compiler flag. pub fn is_panic_abort_ok(self) -> bool { self.panic_abort_ok } diff --git a/src/cargo/ops/cargo_compile.rs b/src/cargo/ops/cargo_compile.rs index 9895adef2cf..b14557baf2d 100644 --- a/src/cargo/ops/cargo_compile.rs +++ b/src/cargo/ops/cargo_compile.rs @@ -642,7 +642,7 @@ fn generate_targets<'a>( ) -> CargoResult>> { // Helper for creating a `Unit` struct. let new_unit = |pkg: &'a Package, target: &'a Target, target_mode: CompileMode| { - let unit_for = if bcx.build_config.mode.is_any_test() { + let unit_for = if target_mode.is_any_test() { // NOTE: the `UnitFor` here is subtle. If you have a profile // with `panic` set, the `panic` flag is cleared for // tests/benchmarks and their dependencies. If this @@ -661,7 +661,7 @@ fn generate_targets<'a>( // // Forcing the lib to be compiled three times during `cargo // test` is probably also not desirable. - UnitFor::new_test() + UnitFor::new_test(bcx.config) } else if target.for_host() { // Proc macro / plugin should not have `panic` set. UnitFor::new_compiler() diff --git a/src/doc/src/reference/unstable.md b/src/doc/src/reference/unstable.md index b63901637a1..18779290934 100644 --- a/src/doc/src/reference/unstable.md +++ b/src/doc/src/reference/unstable.md @@ -475,3 +475,17 @@ that information for change-detection (if any binary dependency changes, then the crate will be rebuilt). The primary use case is for building the compiler itself, which has implicit dependencies on the standard library that would otherwise be untracked for change-detection. + +### panic-abort-tests + +The `-Z panic-abort-tests` flag will enable nightly support to compile test +harness crates with `-Cpanic=abort`. Without this flag Cargo will compile tests, +and everything they depend on, with `-Cpanic=unwind` because it's the only way +`test`-the-crate knows how to operate. As of [rust-lang/rust#64158], however, +the `test` crate supports `-C panic=abort` with a test-per-process, and can help +avoid compiling crate graphs multiple times. + +It's currently unclear how this feature will be stabilized in Cargo, but we'd +like to stabilize it somehow! + +[rust-lang/rust#64158]: https://github.com/rust-lang/rust/pull/64158 diff --git a/tests/testsuite/test.rs b/tests/testsuite/test.rs index 51223fd01de..c8bf213cbb9 100644 --- a/tests/testsuite/test.rs +++ b/tests/testsuite/test.rs @@ -3872,3 +3872,132 @@ fn cargo_test_doctest_xcompile_no_runner() { ) .run(); } + +#[cargo_test] +fn panic_abort_tests() { + if !is_nightly() { + // -Zpanic-abort-tests in rustc is unstable + return; + } + + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = 'foo' + version = '0.1.0' + + [dependencies] + a = { path = 'a' } + + [profile.dev] + panic = 'abort' + [profile.test] + panic = 'abort' + "#, + ) + .file( + "src/lib.rs", + r#" + #[test] + fn foo() { + a::foo(); + } + "#, + ) + .file("a/Cargo.toml", &basic_lib_manifest("a")) + .file("a/src/lib.rs", "pub fn foo() {}") + .build(); + + p.cargo("test -Z panic-abort-tests -v") + .with_stderr_contains("[..]--crate-name a [..]-C panic=abort[..]") + .with_stderr_contains("[..]--crate-name foo [..]-C panic=abort[..]") + .with_stderr_contains("[..]--crate-name foo [..]-C panic=abort[..]--test[..]") + .masquerade_as_nightly_cargo() + .run(); +} + +#[cargo_test] +fn panic_abort_only_test() { + if !is_nightly() { + // -Zpanic-abort-tests in rustc is unstable + return; + } + + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = 'foo' + version = '0.1.0' + + [dependencies] + a = { path = 'a' } + + [profile.test] + panic = 'abort' + "#, + ) + .file( + "src/lib.rs", + r#" + #[test] + fn foo() { + a::foo(); + } + "#, + ) + .file("a/Cargo.toml", &basic_lib_manifest("a")) + .file("a/src/lib.rs", "pub fn foo() {}") + .build(); + + p.cargo("test -Z panic-abort-tests -v") + .with_stderr_does_not_contain("[..]--crate-name a [..]-C panic=abort[..]") + .with_stderr_contains("[..]--crate-name foo [..]-C panic=abort[..]--test[..]") + .masquerade_as_nightly_cargo() + .run(); +} + +#[cargo_test] +fn panic_abort_invalid() { + if !is_nightly() { + // -Zpanic-abort-tests in rustc is unstable + return; + } + + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = 'foo' + version = '0.1.0' + + [dependencies] + a = { path = 'a' } + + [profile.dev] + panic = 'abort' + "#, + ) + .file( + "src/lib.rs", + r#" + #[test] + fn foo() { + a::foo(); + } + "#, + ) + .file("a/Cargo.toml", &basic_lib_manifest("a")) + .file("a/src/lib.rs", "pub fn foo() {}") + .build(); + + p.cargo("test -Z panic-abort-tests -v") + .masquerade_as_nightly_cargo() + .with_status(101) + .with_stderr_contains("[..]incompatible with this crate's strategy[..]") + .run(); +} From 9cfdf4d8bf1000857c8d8597f48f87f4fc6becd8 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Fri, 18 Oct 2019 06:39:57 -0700 Subject: [PATCH 2/3] Inherit `panic` settings in test/bench profiles We've always ignored the `panic` settings configured in test/bench profiles, so this just continues to ignore them but in a slightly different way. --- src/cargo/core/profiles.rs | 98 ++++++++++++++++++++++++++------------ tests/testsuite/test.rs | 8 ++-- 2 files changed, 70 insertions(+), 36 deletions(-) diff --git a/src/cargo/core/profiles.rs b/src/cargo/core/profiles.rs index 4d96bbffcf6..832d387bf73 100644 --- a/src/cargo/core/profiles.rs +++ b/src/cargo/core/profiles.rs @@ -314,7 +314,7 @@ impl Profiles { mode: CompileMode, profile_kind: ProfileKind, ) -> Profile { - let profile_name = if !self.named_profiles_enabled { + let (profile_name, inherits) = if !self.named_profiles_enabled { // With the feature disabled, we degrade `--profile` back to the // `--release` and `--debug` predicates, and convert back from // ProfileKind::Custom instantiation. @@ -328,9 +328,9 @@ impl Profiles { match mode { CompileMode::Test | CompileMode::Bench => { if release { - "bench" + ("bench", Some("release")) } else { - "test" + ("test", Some("dev")) } } CompileMode::Build @@ -342,26 +342,33 @@ impl Profiles { // ancestor's profile. However, `cargo clean -p` can hit this // path. if release { - "release" + ("release", None) } else { - "dev" + ("dev", None) } } - CompileMode::Doc { .. } => "doc", + CompileMode::Doc { .. } => ("doc", None), } } else { - profile_kind.name() + (profile_kind.name(), None) }; let maker = match self.by_name.get(profile_name) { None => panic!("Profile {} undefined", profile_name), Some(r) => r, }; let mut profile = maker.get_profile(Some(pkg_id), is_member, unit_for); - // `panic` may not be set for tests/benches, or any of their - // dependencies, so handle that here if that's what `UnitFor` tells us - // to do. - if !unit_for.is_panic_abort_ok() { - profile.panic = PanicStrategy::Unwind; + + // Dealing with `panic=abort` and `panic=unwind` requires some special + // treatment. Be sure to process all the various options here. + match unit_for.panic_setting() { + PanicSetting::AlwaysUnwind => profile.panic = PanicStrategy::Unwind, + PanicSetting::ReadProfile => {} + PanicSetting::Inherit => { + if let Some(inherits) = inherits { + let maker = self.by_name.get(inherits).unwrap(); + profile.panic = maker.get_profile(Some(pkg_id), is_member, unit_for).panic; + } + } } // Incremental can be globally overridden. @@ -881,11 +888,25 @@ pub struct UnitFor { /// any of its dependencies. This enables `build-override` profiles for /// these targets. build: bool, - /// This is true if it is *allowed* to set the `panic=abort` flag. Currently - /// this is false for test/bench targets and all their dependencies, and - /// "for_host" units such as proc macro and custom build scripts and their - /// dependencies. - panic_abort_ok: bool, + /// How Cargo processes the `panic` setting or profiles. This is done to + /// handle test/benches inheriting from dev/release, as well as forcing + /// `for_host` units to always unwind. + panic_setting: PanicSetting, +} + +#[derive(Copy, Clone, Debug, Eq, PartialEq, Hash, Ord, PartialOrd)] +enum PanicSetting { + /// Used to force a unit to always be compiled with the `panic=unwind` + /// strategy, notably for build scripts, proc macros, etc. + AlwaysUnwind, + + /// Indicates that this unit will read its `profile` setting and use + /// whatever is configured there. + ReadProfile, + + /// This unit will ignore its `panic` setting in its profile and will + /// instead inherit it from the `dev` or `release` profile, as appropriate. + Inherit, } impl UnitFor { @@ -894,7 +915,7 @@ impl UnitFor { pub fn new_normal() -> UnitFor { UnitFor { build: false, - panic_abort_ok: true, + panic_setting: PanicSetting::ReadProfile, } } @@ -904,7 +925,7 @@ impl UnitFor { build: true, // Force build scripts to always use `panic=unwind` for now to // maximally share dependencies with procedural macros. - panic_abort_ok: false, + panic_setting: PanicSetting::AlwaysUnwind, } } @@ -915,7 +936,7 @@ impl UnitFor { // Force plugins to use `panic=abort` so panics in the compiler do // not abort the process but instead end with a reasonable error // message that involves catching the panic in the compiler. - panic_abort_ok: false, + panic_setting: PanicSetting::AlwaysUnwind, } } @@ -928,7 +949,15 @@ impl UnitFor { pub fn new_test(config: &Config) -> UnitFor { UnitFor { build: false, - panic_abort_ok: config.cli_unstable().panic_abort_tests, + // We're testing out an unstable feature (`-Zpanic-abort-tests`) + // which inherits the panic setting from the dev/release profile + // (basically avoid recompiles) but historical defaults required + // that we always unwound. + panic_setting: if config.cli_unstable().panic_abort_tests { + PanicSetting::Inherit + } else { + PanicSetting::AlwaysUnwind + }, } } @@ -942,7 +971,11 @@ impl UnitFor { pub fn with_for_host(self, for_host: bool) -> UnitFor { UnitFor { build: self.build || for_host, - panic_abort_ok: self.panic_abort_ok && !for_host, + panic_setting: if for_host { + PanicSetting::AlwaysUnwind + } else { + self.panic_setting + }, } } @@ -952,29 +985,32 @@ impl UnitFor { self.build } - /// Returns `true` if this unit is allowed to set the `panic=abort` - /// compiler flag. - pub fn is_panic_abort_ok(self) -> bool { - self.panic_abort_ok + /// Returns how `panic` settings should be handled for this profile + fn panic_setting(self) -> PanicSetting { + self.panic_setting } /// All possible values, used by `clean`. pub fn all_values() -> &'static [UnitFor] { - static ALL: [UnitFor; 3] = [ + static ALL: &[UnitFor] = &[ UnitFor { build: false, - panic_abort_ok: true, + panic_setting: PanicSetting::ReadProfile, }, UnitFor { build: true, - panic_abort_ok: false, + panic_setting: PanicSetting::AlwaysUnwind, + }, + UnitFor { + build: false, + panic_setting: PanicSetting::AlwaysUnwind, }, UnitFor { build: false, - panic_abort_ok: false, + panic_setting: PanicSetting::Inherit, }, ]; - &ALL + ALL } } diff --git a/tests/testsuite/test.rs b/tests/testsuite/test.rs index c8bf213cbb9..76fc1e09252 100644 --- a/tests/testsuite/test.rs +++ b/tests/testsuite/test.rs @@ -3954,14 +3954,13 @@ fn panic_abort_only_test() { .build(); p.cargo("test -Z panic-abort-tests -v") - .with_stderr_does_not_contain("[..]--crate-name a [..]-C panic=abort[..]") - .with_stderr_contains("[..]--crate-name foo [..]-C panic=abort[..]--test[..]") + .with_stderr_contains("warning: `panic` setting is ignored for `test` profile") .masquerade_as_nightly_cargo() .run(); } #[cargo_test] -fn panic_abort_invalid() { +fn panic_abort_test_profile_inherits() { if !is_nightly() { // -Zpanic-abort-tests in rustc is unstable return; @@ -3997,7 +3996,6 @@ fn panic_abort_invalid() { p.cargo("test -Z panic-abort-tests -v") .masquerade_as_nightly_cargo() - .with_status(101) - .with_stderr_contains("[..]incompatible with this crate's strategy[..]") + .with_status(0) .run(); } From 269624f11d87ee4aa9e0aae6c4cafb75a5e39e13 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Mon, 21 Oct 2019 09:37:13 -0700 Subject: [PATCH 3/3] Fix broken tests on nightly --- tests/testsuite/cross_compile.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/testsuite/cross_compile.rs b/tests/testsuite/cross_compile.rs index 8282ca7c572..ae36b1e8e09 100644 --- a/tests/testsuite/cross_compile.rs +++ b/tests/testsuite/cross_compile.rs @@ -203,12 +203,13 @@ fn plugin_deps() { extern crate rustc_driver; extern crate syntax; + extern crate syntax_expand; use rustc_driver::plugin::Registry; use syntax::tokenstream::TokenStream; use syntax::source_map::Span; use syntax::ast::*; - use syntax::ext::base::{ExtCtxt, MacEager, MacResult}; + use syntax_expand::base::{ExtCtxt, MacEager, MacResult}; #[plugin_registrar] pub fn foo(reg: &mut Registry) { @@ -298,13 +299,14 @@ fn plugin_to_the_max() { extern crate rustc_driver; extern crate syntax; + extern crate syntax_expand; extern crate baz; use rustc_driver::plugin::Registry; use syntax::tokenstream::TokenStream; use syntax::source_map::Span; use syntax::ast::*; - use syntax::ext::base::{ExtCtxt, MacEager, MacResult}; + use syntax_expand::base::{ExtCtxt, MacEager, MacResult}; use syntax::ptr::P; #[plugin_registrar]