Skip to content
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

std: Ensure OOM is classified as nounwind #51041

Merged
merged 1 commit into from
May 26, 2018

Conversation

alexcrichton
Copy link
Member

OOM can't unwind today, and historically it's been optimized as if it can't
unwind. This accidentally regressed with recent changes to the OOM handler, so
this commit adds in a codegen test to assert that everything gets optimized away
after the OOM function is approrpiately classified as nounwind

Closes #50925

OOM can't unwind today, and historically it's been optimized as if it can't
unwind. This accidentally regressed with recent changes to the OOM handler, so
this commit adds in a codegen test to assert that everything gets optimized away
after the OOM function is approrpiately classified as nounwind

Closes rust-lang#50925
@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

(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 May 24, 2018
// *in Rust code* may unwind. Foreign items like `extern "C" {
// fn foo(); }` are assumed not to unwind **unless** they have
// a `#[unwind]` attribute.
} else if !cx.tcx.is_foreign_item(id) {
Copy link
Member

Choose a reason for hiding this comment

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

Is this going to cause similar issues to #48251?

Copy link
Member

Choose a reason for hiding this comment

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

Oh nevermind, seems like we've already been doing this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yeah this was accidentally overwriting the custom attributes we have so I just moved this around to make the attributes (unstable) take priority

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented May 25, 2018

📌 Commit f674537 has been approved by nikomatsakis

@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 May 25, 2018
@bors
Copy link
Contributor

bors commented May 26, 2018

⌛ Testing commit f674537 with merge b4247d4...

bors added a commit that referenced this pull request May 26, 2018
std: Ensure OOM is classified as `nounwind`

OOM can't unwind today, and historically it's been optimized as if it can't
unwind. This accidentally regressed with recent changes to the OOM handler, so
this commit adds in a codegen test to assert that everything gets optimized away
after the OOM function is approrpiately classified as nounwind

Closes #50925
@bors
Copy link
Contributor

bors commented May 26, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing b4247d4 to master...

@bors bors merged commit f674537 into rust-lang:master May 26, 2018
@bors bors mentioned this pull request May 26, 2018
@glandium
Copy link
Contributor

Why? Unwinding did work. #50880 (comment)

@nikic
Copy link
Contributor

nikic commented May 26, 2018

@glandium I think the idea here is not that unwinding can't work, but rather that we should preserve a way to keep these optimizations for the people who don't make use of unwinding OOM.

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants