-
-
Notifications
You must be signed in to change notification settings - Fork 3k
Build-std: panic strategies #17185
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
base: master
Are you sure you want to change the base?
Build-std: panic strategies #17185
Changes from all commits
38e1e62
2ac9da3
c4080d5
fe85a2a
c603013
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -3,7 +3,7 @@ | |||
| use crate::core::compiler::UnitInterner; | ||||
| use crate::core::compiler::unit_dependencies::IsArtifact; | ||||
| use crate::core::compiler::{CompileKind, CompileMode, RustcTargetData, Unit}; | ||||
| use crate::core::profiles::{Profiles, UnitFor}; | ||||
| use crate::core::profiles::{PanicStrategy, Profiles, UnitFor}; | ||||
| use crate::core::resolver::HasDevUnits; | ||||
| use crate::core::resolver::features::{CliFeatures, FeaturesFor, ResolvedFeatures}; | ||||
| use crate::core::{PackageId, PackageSet, Resolve, Workspace}; | ||||
|
|
@@ -15,7 +15,12 @@ use std::path::PathBuf; | |||
|
|
||||
| use super::BuildConfig; | ||||
|
|
||||
| fn std_crates<'a>(crates: &'a [String], default: &'static str, units: &[Unit]) -> HashSet<&'a str> { | ||||
| fn std_crates<'a>( | ||||
| crates: &'a [String], | ||||
| default: &'static str, | ||||
| units: &[Unit], | ||||
| profiles: &Profiles, | ||||
| ) -> HashSet<&'a str> { | ||||
| let mut crates = HashSet::from_iter(crates.iter().map(|s| s.as_str())); | ||||
| // This is a temporary hack until there is a more principled way to | ||||
| // declare dependencies in Cargo.toml. | ||||
|
|
@@ -26,7 +31,7 @@ fn std_crates<'a>(crates: &'a [String], default: &'static str, units: &[Unit]) - | |||
| crates.insert("core"); | ||||
| crates.insert("alloc"); | ||||
| crates.insert("proc_macro"); | ||||
| crates.insert("panic_unwind"); | ||||
| crates.insert("panic_abort"); | ||||
| crates.insert("compiler_builtins"); | ||||
| // Only build libtest if it looks like it is needed (libtest depends on libstd) | ||||
| // If we know what units we're building, we can filter for libtest depending on the jobs. | ||||
|
|
@@ -36,6 +41,13 @@ fn std_crates<'a>(crates: &'a [String], default: &'static str, units: &[Unit]) - | |||
| { | ||||
| crates.insert("test"); | ||||
| } | ||||
| // Conditionally build `panic_unwind` based on the user's profile settings. | ||||
| // NOTE: Base profile is enough here since `panic` profile cannot be | ||||
| // overridden. See `validate_profile_override`. | ||||
| let profile = profiles.base_profile(); | ||||
| if profile.panic == PanicStrategy::Unwind { | ||||
| crates.insert("panic_unwind"); | ||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should be enabled via a feature on |
||||
| } | ||||
|
Comment on lines
+47
to
+50
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This doesn't seem to handle the situation for There's also some complexities when running host tests (proc-macro tests), which I think are also AlwaysUnwind. I'm a little concerned that trying to deal with these complexities will make this optimization difficult to support. For abort-only targets, the unwind crate should be empty and essentially have no cost.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this test exercise that case? https://github.com/rust-lang/cargo/pull/17185/changes#diff-736f9dbd0f15c0f86f99fa4c395a7639c384213e0d5fe33aadab3f411d53de92R1050
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it turned out that the test is incorrect due to:
Today I plan to open a PR in rustc to address #17185 (comment) (load panic runtime with |
||||
| } else if crates.contains("core") { | ||||
| crates.insert("compiler_builtins"); | ||||
| } | ||||
|
|
@@ -63,29 +75,37 @@ pub fn resolve_std<'gctx>( | |||
| // `[dev-dependencies]`. No need for us to generate a `Resolve` which has | ||||
| // those included because we'll never use them anyway. | ||||
| std_ws.set_require_optional_deps(false); | ||||
| let specs = { | ||||
| let profiles = Profiles::new(ws, build_config.requested_profile)?; | ||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just a minor nit, but I worry that this function has the possibility to be expensive to call. Would it be possible to pass it in? |
||||
| let (specs, build_panic_unwind) = { | ||||
| // If there is anything looks like needing std, resolve with it. | ||||
| // If not, we assume only `core` maye be needed, as `core the most fundamental crate. | ||||
| // If not, we assume only `core` may be needed, as `core` is the most fundamental crate. | ||||
| // | ||||
| // This may need a UI overhaul if `build-std` wants to fully support multi-targets. | ||||
| let maybe_std = kinds | ||||
| .iter() | ||||
| .any(|kind| target_data.info(*kind).maybe_support_std()); | ||||
| let mut crates = std_crates(crates, if maybe_std { "std" } else { "core" }, &[]); | ||||
| let mut crates = std_crates( | ||||
| crates, | ||||
| if maybe_std { "std" } else { "core" }, | ||||
| &[], | ||||
| &profiles, | ||||
| ); | ||||
| let build_panic_unwind = crates.contains("panic_unwind"); | ||||
| // `sysroot` is not in the default set because it is optional, but it needs | ||||
| // to be part of the resolve in case we do need it or `libtest`. | ||||
| crates.insert("sysroot"); | ||||
| let specs = Packages::Packages(crates.into_iter().map(Into::into).collect()); | ||||
| specs.to_package_id_specs(&std_ws)? | ||||
| (specs.to_package_id_specs(&std_ws)?, build_panic_unwind) | ||||
| }; | ||||
| let features = match &gctx.cli_unstable().build_std_features { | ||||
| let mut features = match &gctx.cli_unstable().build_std_features { | ||||
| Some(list) => list.clone(), | ||||
| None => vec![ | ||||
| "panic-unwind".to_string(), | ||||
| "backtrace".to_string(), | ||||
| "default".to_string(), | ||||
| ], | ||||
| None => vec!["backtrace".to_string(), "default".to_string()], | ||||
| }; | ||||
|
|
||||
| if build_panic_unwind { | ||||
| features.push("panic-unwind".to_string()); | ||||
| } | ||||
|
|
||||
| let cli_features = CliFeatures::from_command_line( | ||||
| &features, /*all_features*/ false, /*uses_default_features*/ false, | ||||
| )?; | ||||
|
|
@@ -167,7 +187,7 @@ fn generate_roots( | |||
| profiles: &Profiles, | ||||
| target_data: &RustcTargetData<'_>, | ||||
| ) -> CargoResult<()> { | ||||
| let std_ids = std_crates(crates, default, units) | ||||
| let std_ids = std_crates(crates, default, units, profiles) | ||||
| .iter() | ||||
| .map(|crate_name| std_resolve.query(crate_name)) | ||||
| .collect::<CargoResult<Vec<PackageId>>>()?; | ||||
|
|
||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| [package] | ||
| name = "panic_abort" | ||
| version = "0.1.0" | ||
| edition = "2018" | ||
|
|
||
| [dependencies] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| #![feature(panic_abort, panic_runtime)] | ||
| #![panic_runtime] | ||
| #![no_std] | ||
|
|
||
| extern crate panic_abort; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be built by default. The reason the strategies are roots is that rustc won't pick them up unless they're passed in via --extern. We have a section in the build-std=always RFC for this, but it's not next to the other section on panic strategies (sorry!)
This doesn't need to block this PR though as the change to this patch to support that will be quite small, and this function will change a lot in the future anyway when the UI changes.