-
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
Remove -Z no-landing-pads flag #70175
Conversation
(rust_highfive has picked a reviewer for you, use r? to override) |
I am in favor of doing this. I think it probably makes sense to start a compiler team FCP here though (I can't personally do that I think). r? @pnkfelix |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Well I have no idea why this test is failing... We are passing in
|
Most likely result of using $ echo 'fn main(){}' > a.rs
$ rustc -Cpanic=abort -Cprefer-dynamic a.rs
error: the linked panic runtime `panic_unwind` is not compiled with this crate's panic strategy `abort`
error: aborting due to previous error |
cc @rust-lang/compiler: @Mark-Simulacrum suggests starting a compiler team FCP for this PR. |
src/librustc_session/session.rs
Outdated
pub fn no_landing_pads(&self) -> bool { | ||
self.opts.debugging_opts.no_landing_pads || self.panic_strategy() == PanicStrategy::Abort | ||
self.panic_strategy() == PanicStrategy::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 wonder if this should be inlined or not.
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.
Won't LTO take care of it regardless?
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 don't mean in terms of performance, only code organization.
The reason these methods exist is because the conditions they check are non-trivial, otherwise options should be checked directly.
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 don't mean in terms of performance, only code organization.
The reason these methods exist is because the conditions they check are non-trivial, otherwise options should be checked directly.
I don't personally agree with eddyb's end assertion; sometimes checking options directly makes sense, but I have seen cases where it can be easier to read the overall logic with helper methods, especially if the names of the helper methods end up encoding something that would otherwise be written as a comment alongside the call site.
Having said that, I don't care enough one way or the other in this case to try to argue that this helper method should stay in place. I just don't want the comment here to, on its own, be treated as precedent for a general rule across the board.
@rfcbot fcp merge |
Team member @pnkfelix has proposed to merge this. The next step is review by the rest of the tagged team members:
No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
Question: Is |
☔ The latest upstream changes (presumably #70692) made this pull request unmergeable. Please resolve the merge conflicts. |
@petrochenkov |
`no-landing-pads` was removed from rust nightly. mozilla/grcov#427 and rust-lang/rust#70175
Follow issue rust-lang/rust#70175
* Update nightly in CI See rust-lang/rust#70175 and rust-lang/cargo#6375 * Bump minimum rust version to 1.43.1 * Add rustdocflags * Bump grcov version
* Update nightly in CI See rust-lang/rust#70175 and rust-lang/cargo#6375 * Bump minimum rust version to 1.43.1 * Add rustdocflags * Bump grcov version
* Update nightly in CI See rust-lang/rust#70175 and rust-lang/cargo#6375 * Bump minimum rust version to 1.43.1 * Add rustdocflags * Bump grcov version
* New makefile targets to shortcut existing functionality * Remove no-landing-pads, see rust-lang/rust#70175 * Run fmt check only on stable * Use makefile target for testing
Remove -Zno-landing-pads flag, because it was removed in rust-lang/rust#70175 Its replacement is panic=abort, which... we already use.
Remove -Zno-landing-pads flag, because it was removed in rust-lang/rust#70175 Its replacement is panic=abort, which... we already use.
Remove -Zno-landing-pads flag, because it was removed in rust-lang/rust#70175 Its replacement is panic=abort, which... we already use.
@Amanieu I can't use
Compiler says:
|
43: Fix compiler flags for ProfileWithGrcov build option r=PaulGrandperrin a=viniul This commit replaces the -Zno-landing-pads with the -C panic=unwind, as the -Zno-landing-pdas is deprecated, see [here](rust-lang/rust#70175). Co-authored-by: Vincent Ulitzsch <[email protected]>
Since #67502,
-Z no-landing-pads
will cause all attempted unwinds to abort since we don't generate atry
/catch
. This previously worked because__rust_try
was located in libpanic_unwind which is always compiled with-C panic=unwind
, but__rust_try
is now directly inline into the crate that usescatch_unwind
.As such,
-Z no-landing-pads
is now mostly useless and people should use-C panic=abort
instead.