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

Support rustc's -Z panic-abort-tests in Cargo #7460

Merged
merged 3 commits into from
Oct 21, 2019
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
11 changes: 11 additions & 0 deletions src/cargo/core/compiler/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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");
}
Expand Down
2 changes: 1 addition & 1 deletion src/cargo/core/compiler/unit_dependencies.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
2 changes: 2 additions & 0 deletions src/cargo/core/features.rs
Original file line number Diff line number Diff line change
Expand Up @@ -340,6 +340,7 @@ pub struct CliUnstable {
pub build_std: Option<Vec<String>>,
pub timings: Option<Vec<String>>,
pub doctest_xcompile: bool,
pub panic_abort_tests: bool,
}

impl CliUnstable {
Expand Down Expand Up @@ -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),
}

Expand Down
115 changes: 83 additions & 32 deletions src/cargo/core/profiles.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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
Expand All @@ -342,25 +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` should not be set for tests/benches, or any of their
// dependencies.
if !unit_for.is_panic_abort_ok() || mode.is_any_test() {
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.
Expand Down Expand Up @@ -880,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 {
Expand All @@ -893,42 +915,67 @@ impl UnitFor {
pub fn new_normal() -> UnitFor {
UnitFor {
build: false,
panic_abort_ok: true,
panic_setting: PanicSetting::ReadProfile,
}
}

/// A unit for a custom build script or its dependencies.
pub fn new_build() -> UnitFor {
UnitFor {
build: true,
panic_abort_ok: false,
// Force build scripts to always use `panic=unwind` for now to
// maximally share dependencies with procedural macros.
panic_setting: PanicSetting::AlwaysUnwind,
}
}

/// A unit for a proc macro or compiler plugin or their dependencies.
pub fn new_compiler() -> UnitFor {
UnitFor {
build: false,
panic_abort_ok: 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_setting: PanicSetting::AlwaysUnwind,
}
}

/// 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,
// 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
},
}
}

/// 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,
panic_abort_ok: self.panic_abort_ok && !for_host,
panic_setting: if for_host {
PanicSetting::AlwaysUnwind
} else {
self.panic_setting
},
}
}

Expand All @@ -938,28 +985,32 @@ impl UnitFor {
self.build
}

/// Returns `true` if this unit is allowed to set the `panic` 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
}
}

Expand Down
4 changes: 2 additions & 2 deletions src/cargo/ops/cargo_compile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -642,7 +642,7 @@ fn generate_targets<'a>(
) -> CargoResult<Vec<Unit<'a>>> {
// 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() {
Copy link
Member Author

Choose a reason for hiding this comment

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

@ehuss I'll want to draw your eye particularly to this line. I sort of blindly made this change assuming that this is the best interpretation nowadays, and it is required to remove the check for mode.is_any_test() in profiles.rs above

// 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
Expand All @@ -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()
Expand Down
14 changes: 14 additions & 0 deletions src/doc/src/reference/unstable.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
6 changes: 4 additions & 2 deletions tests/testsuite/cross_compile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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]
Expand Down
Loading