-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Always mark rust and rust-call abi's as unwind #65020
Always mark rust and rust-call abi's as unwind #65020
Conversation
r? @cramertj (rust_highfive has picked a reviewer for you, use r? to override) |
nominating for beta backport once/if this actually lands. |
src/test/ui/extern/issue-64655-allow-unwind-when-calling-panic-directly.rs
Outdated
Show resolved
Hide resolved
src/test/ui/extern/issue-64655-allow-unwind-when-calling-panic-directly.rs
Outdated
Show resolved
Hide resolved
src/test/ui/extern/issue-64655-extern-rust-must-allow-unwind.rs
Outdated
Show resolved
Hide resolved
src/test/ui/extern/issue-64655-extern-rust-must-allow-unwind.rs
Outdated
Show resolved
Hide resolved
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 |
r=me on this, thanks for the clear PR description and the clear comments in the code! |
Funny, I just today talked with @nikomatsakis about this. ;) Yes, this PR is certainly a step in the right direction. I was not expecting my PR to be that controversial; that's the only reason why I did not split out this change to begin with. |
Co-Authored-By: Mazdak Farrokhzad <twingoow@gmail.com> Co-Authored-By: Ralf Jung <post@ralfj.de>
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 |
Some comment refinements inspired by review feedback.
@bors r=alexcrichton |
📌 Commit 71e5f78 has been approved by |
This needs a codegen test to verify that we never generate nounwind for extern calls like those. |
(I will do this in a follow-up PR.) |
Ah, missed another test. @bors r=alexcrichton |
📌 Commit 028f53e has been approved by |
⌛ Testing commit 028f53e with merge d3077a768c8c64752034e2814c42ac030ec4bf25... |
@bors retry yielding to rollup |
…-abi-unwind-issue-64655, r=alexcrichton Always mark rust and rust-call abi's as unwind PR #63909 identified a bug that had been injected by PR #55982. As discussed on #64655 (comment) , we started marking extern items as nounwind, *even* extern items that said they were using "Rust" or "rust-call" ABI. This is a more targeted variant of PR #63909 that fixes the above bug. Fix #64655 ---- I personally suspect we will want PR #63909 to land in the long-term But: * it is not certain that PR #63909 *will* land, * more importantly, PR #63909 almost certainly will not be backported to beta/stable. The identified bug was more severe than I think anyone realized (apart from perhaps @gnzlbg, as noted [here](#63909 (comment))). Thus, I was motivated to write this PR, which fixes *just* the issue with extern rust/rust-call functions, and deliberately avoids injecting further deviation from current behavior (you can see further notes on this in the comments of the code added here).
☀️ Test successful - checks-azure |
nounwind tests and cleanup This is a follow-up to @pnkfelix' rust-lang#65020. In particular it adds some tests as @nagisa asked. It also does a cleanup that the original PR omitted to reduce backporting risks. I hope I finally managed to write an uncontroversial PR in this area. ;) This should not change any behavior, just test it better.
nounwind tests and cleanup This is a follow-up to @pnkfelix' rust-lang#65020. In particular it adds some tests as @nagisa asked. It also does a cleanup that the original PR omitted to reduce backporting risks. I hope I finally managed to write an uncontroversial PR in this area. ;) This should not change any behavior, just test it better.
…king-rust-abi-unwind-issue-64655, r=alexcrichton Always mark rust and rust-call abi's as unwind PR rust-lang#63909 identified a bug that had been injected by PR rust-lang#55982. As discussed on rust-lang#64655 (comment) , we started marking extern items as nounwind, *even* extern items that said they were using "Rust" or "rust-call" ABI. This is a more targeted variant of PR rust-lang#63909 that fixes the above bug. Fix rust-lang#64655 ---- I personally suspect we will want PR rust-lang#63909 to land in the long-term But: * it is not certain that PR rust-lang#63909 *will* land, * more importantly, PR rust-lang#63909 almost certainly will not be backported to beta/stable. The identified bug was more severe than I think anyone realized (apart from perhaps @gnzlbg, as noted [here](rust-lang#63909 (comment))). Thus, I was motivated to write this PR, which fixes *just* the issue with extern rust/rust-call functions, and deliberately avoids injecting further deviation from current behavior (you can see further notes on this in the comments of the code added here).
[beta] backport rollup This includes a bunch of PRs: * Fix redundant semicolon lint interaction with proc macro attributes #64387 * Upgrade async/await to "used" keywords. #64875 * syntax: fix dropping of attribute on first param of non-method assocated fn #64894 * async/await: improve not-send errors #64895 * Silence unreachable code lint from await desugaring #64930 * Always mark rust and rust-call abi's as unwind #65020 * Account for macro invocation in `let mut $pat` diagnostic. #65123 * Ensure that associated `async fn`s have unique fresh param names #65142 * Add troubleshooting section to PGO chapter in rustc book. #65402 * Upgrade GCC to 8.3.0, glibc to 1.17.0 and crosstool-ng to 1.24.0 for dist-armv7-linux #65302 * Optimize `try_expand_impl_trait_type` #65293 * use precalculated dominators in explain_borrow #65172 * Fix ICE #64964 #64989
[beta] backport rollup This includes a bunch of PRs: * Fix redundant semicolon lint interaction with proc macro attributes #64387 * Upgrade async/await to "used" keywords. #64875 * syntax: fix dropping of attribute on first param of non-method assocated fn #64894 * async/await: improve not-send errors #64895 * Silence unreachable code lint from await desugaring #64930 * Always mark rust and rust-call abi's as unwind #65020 * Account for macro invocation in `let mut $pat` diagnostic. #65123 * Ensure that associated `async fn`s have unique fresh param names #65142 * Add troubleshooting section to PGO chapter in rustc book. #65402 * Upgrade GCC to 8.3.0, glibc to 1.17.0 and crosstool-ng to 1.24.0 for dist-armv7-linux #65302 * Optimize `try_expand_impl_trait_type` #65293 * use precalculated dominators in explain_borrow #65172 * Fix ICE #64964 #64989 * [beta] Revert "Auto merge of #62948 - matklad:failable-file-loading, r=petro… #65273 * save-analysis: Don't ICE when resolving qualified type paths in struct members #65353 * save-analysis: Nest tables when processing impl block definitions #65511 * Avoid ICE when checking `Destination` of `break` inside a closure #65518 * Avoid ICE when adjusting bad self ty #65755 * workaround msys2 bug #65762
PR #63909 identified a bug that had been injected by PR #55982. As discussed on #64655 (comment) , we started marking extern items as nounwind, even extern items that said they were using "Rust" or "rust-call" ABI.
This is a more targeted variant of PR #63909 that fixes the above bug.
Fix #64655
I personally suspect we will want PR #63909 to land in the long-term
But:
The identified bug was more severe than I think anyone realized (apart from perhaps @gnzlbg, as noted here).
Thus, I was motivated to write this PR, which fixes just the issue with extern rust/rust-call functions, and deliberately avoids injecting further deviation from current behavior (you can see further notes on this in the comments of the code added here).