Add panic=immediate-abort support to Cargo#16041
Conversation
|
r? @weihanglo rustbot has assigned @weihanglo. Use |
| (unstable, multiple_build_scripts, "", "reference/unstable.html#multiple-build-scripts"), | ||
|
|
||
| /// Allows use of panic="immediate-abort". | ||
| (unstable, panic_immediate_abort, "", "reference/unstable.html#panic-immediate-abort"), |
There was a problem hiding this comment.
As the [profile] table is also supported in Cargo configuration, you might want to add both feature gates:
- Unstable feature in Cargo.toml manifest
- Unstable flag in CLI/config.toml
See the first few commits in #12625 for reference
src/doc/src/reference/unstable.md
Outdated
|
|
||
| * Tracking Issue: [#16042](https://github.com/rust-lang/cargo/issues/16042) | ||
|
|
||
| Allow use of the ImmediateAbort panic strategy in a Cargo profile |
There was a problem hiding this comment.
Could we link to rustc book and tracking issue (if any)?
src/cargo/core/profiles.rs
Outdated
| @@ -876,13 +877,15 @@ impl serde::ser::Serialize for Lto { | |||
There was a problem hiding this comment.
Hmm. Traced that serialization is used by unit-graph. Before it is too late, i think we should change this to kebab-case.
There was a problem hiding this comment.
And strip seems wrong as well
cargo/src/cargo/core/profiles.rs
Lines 921 to 922 in 94f6379
There was a problem hiding this comment.
cargo/src/cargo/core/profiles.rs
Lines 607 to 608 in 56bdf49
Hey we don't even specify anything for the entire struct! Probably worth a separate PR/issue to address it.
We're holding 2 things behind: - The toolchain because of rust-lang/rust#146974. We'll wait until rust-lang/cargo#16041 is fixed to update. - The WebAssembly spec because non-trivial changes might be needed to update the interpreter (or at least the test-suite).
| "immediate-abort" => PanicStrategy::ImmediateAbort, | ||
| // This should be validated in TomlProfile::validate | ||
| _ => panic!("Unexpected panic setting `{}`", panic), |
There was a problem hiding this comment.
The validation in there needs to be updated
There was a problem hiding this comment.
I'm afraid I'm lost. What validation? I can't find any validation of the panic setting or a TomlProfile::validate.
There was a problem hiding this comment.
cargo/src/cargo/util/toml/mod.rs
Lines 2552 to 2560 in 56bdf49
|
Generally, we have end-to-end tests for new features (which would have caught #16041 (comment)) and feature gate tests for unstable features. |
There was a problem hiding this comment.
Looks good. Thank you!
Would you mind cleaning the commit history a bit. We usually follow atomic commit pattern and every commit shall pass all tests (not MUST we don't enforce that but encourage).
|
|
||
| if let Some(panic) = &root.panic { | ||
| if panic != "unwind" && panic != "abort" { | ||
| if panic != "unwind" && panic != "abort" && panic != "immediate-abort" { |
There was a problem hiding this comment.
Not sure if we want to make it a warning or configurable lint in the future. It seems too restrictive. Anyway, it is worth its own separate topic.
efd675c to
50697eb
Compare
|
I don't think a PR that's this conceptually simple needs multiple commits, but I'll split it up if you prefer. |
weihanglo
left a comment
There was a problem hiding this comment.
I personally don't mind for this kind of addition. For bugfix I'll be a bit more nitpicky.
Thanks!
Head branch was pushed to by a user without write access
50697eb to
4cb6fe4
Compare
|
The test |
|
Oh I see this test has been rather flaky recently. Ouch. |
|
yeah, I should create an issue and temporarily ignore that |
Update cargo submodule 3 commits in 2394ea6cea8b26d717aa67eb1663a2dbf2d26078..801d9b4981dd07e3aecdca1ab86834c13615737e 2025-10-03 14:13:01 +0000 to 2025-10-04 13:30:15 +0000 - chore: Disabled `reserved_windows_name` test temporaily (rust-lang/cargo#16048) - Add panic=immediate-abort support to Cargo (rust-lang/cargo#16041) - Accessing each build script's `OUT_DIR` (rust-lang/cargo#15891)
|
|
||
| This can be enabled like so: | ||
| ```toml | ||
| cargo-features = ["immediate-abort"] |
There was a problem hiding this comment.
Should be
| cargo-features = ["immediate-abort"] | |
| cargo-features = ["panic-immediate-abort"] |
### What does this PR try to resolve? #16041 (comment)
I recently turned
-Zbuild-std-features=panic_immediate_abortinto-Cpanic=immediate-abortin rust-lang/rust#146317. There was some discussion of the feature on Zulip here: #t-compiler/major changes > Unstably add -Cpanic=immediate-abort compiler-team#909One of the outcomes of this shipping in nightly is that a few use patterns were broken, see rust-lang/rust#146974 and rust-lang/rust#146317 for examples. I think most of the users commenting on these issues are having trouble with how
RUSTFLAGSis propagated through Cargo, and most likely they would have just gotten what they wanted if they could have set a profile option instead. So I'm trying to implement that.