Skip to content

Conversation

@Amanieu
Copy link
Member

@Amanieu Amanieu commented Jan 30, 2022

This has 2 effects:

  • It helps LLVM when inlining since it doesn't need to generate landing pads for panic_no_unwind.
  • It makes it sound for a panic handler to unwind even if PanicInfo::can_unwind returns true. This will simply cause another panic once the unwind tries to go past the panic_no_unwind lang item. Eventually this will cause a stack overflow, which is safe.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 30, 2022
@rust-highfive
Copy link
Contributor

r? @lcnr

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 30, 2022
@Amanieu
Copy link
Member Author

Amanieu commented Jan 30, 2022

cc @nbdd0121

@lcnr
Copy link
Contributor

lcnr commented Jan 31, 2022

r? @nagisa maybe

don't know enough here

@rust-highfive rust-highfive assigned nagisa and unassigned lcnr Jan 31, 2022
// unwind. If the panic handler that it invokes unwind then it will simply
// call the panic handler again.
if Some(id) == tcx.lang_items().panic_no_unwind() {
codegen_fn_attrs.flags |= CodegenFnAttrFlags::NEVER_UNWIND;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we use #[rustc_allocator_nounwind] instead? There's nothing allocator-specific for that attribute even though it has allocator in its name.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we did this, I would suggest renaming the attribute.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that this should be an attribute (or possibly a extern "Rust-nounwind" ABI?). I've been away this past week and haven't had time to implement this yet, but it is on my TODO list.

@nagisa
Copy link
Member

nagisa commented Feb 5, 2022

@bors r+

I think its okay if we rely on language items to figure out what attributes we want to add to specific ones since we do that already quite pervasively. I can see us using a dedicated attribute once we have one. Right now what we have is pretty arbitrarily scoped to specific component of the standard library and I'm not entirely sure its a responsibility of this PR to adapt it to be more general.

@Amanieu your description mentions soundness improvements. Do you think this is something that should appear in the relnotes?

@bors
Copy link
Collaborator

bors commented Feb 5, 2022

📌 Commit f738669 has been approved by nagisa

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 5, 2022
@Amanieu
Copy link
Member Author

Amanieu commented Feb 5, 2022

This is only reachable with nounwind functions (extern "C" with #![feature(c_unwind)], Drop with -Z panic-in-drop=abort, #[rustc_allocator_nounwind]) all of which are currently unstable. I don't think this merits appearing in the relnotes.

bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 6, 2022
…askrgr

Rollup of 2 pull requests

Successful merges:

 - rust-lang#90998 (Require const stability attribute on all stable functions that are `const`)
 - rust-lang#93489 (Mark the panic_no_unwind lang item as nounwind)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 4695c21 into rust-lang:master Feb 7, 2022
@rustbot rustbot added this to the 1.60.0 milestone Feb 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants