-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Allow building stage1+ std with panic=abort
#119669
Conversation
(rustbot has picked a reviewer for you, use r? to override) |
I'm very wary of the proliferation and complexity of the |
src/bootstrap/src/lib.rs
Outdated
let mut features = " panic-unwind".to_string(); | ||
fn std_features(&self, target: TargetSelection, stage: u32) -> String { | ||
let mut features = match self.config.rust_panic_strategy_std { | ||
PanicStrategy::Abort if stage > 0 => " panic_immediate_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.
I do not think that we should be doing this, aborting panics are not the same as panic_immediate_abort
; aborting panics are effectively just panics that don't unwind the stack. They still do all the formatting of the panic message and print a backtrace. Immediate-abort panics don't do any formatting and just call core::intrinsics::abort
.
I have thought about ways to make immediate-abort a proper panic strategy, and one could do that albiet poorly by changing the lowering of calls to the panic lang items when the feature is enabled. That doesn't fix all the cfg
s which are most of the maintenance burden.
I'm very happy to see someone working on making our support for panic modes other than unwind better; I don't want to stifle making progress by advising too much caution but I do think the fact that we default to unwind and assume unwinding in so many places has laid a lot of traps that it will take time to fix. |
34134db
to
52c907f
Compare
Given that if std uses |
Is this PR waiting on my review, or is more implementation/bug hunting needed? |
More implementation needed - I need to figure out an approach for the above |
☔ The latest upstream changes (presumably #120803) made this pull request unmergeable. Please resolve the merge conflicts. |
@clubby789 any updates on this? |
Closing this as inactive. Feel free to reöpen this pr or create a new pr if you get the time to work on this. Thanks |
Initial approach to #84766
This adds a
rust.panic-strategy-std
option to bootstrap, which controls how stage1+ std is built. I decided not to apply it to stage 0 std, otherwise I'd need to make the bootstrap compiler inject the correct panic runtime to build stage1 correctly.Draft for now as this I'm not sure about the strategy, and only stage0 and 1 can be built with this option right now.
(All library changes are just fixing warnings that occur when building std with
panic_immediate_abort
)