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

Use panic=abort instead of panic=unwind #1825

Merged
merged 1 commit into from
Oct 8, 2017
Merged

Conversation

jrmuizel
Copy link
Collaborator

@jrmuizel jrmuizel commented Oct 6, 2017

This is the same mode as Gecko uses and will get better codegen
once rust-lang/rust#45012 has landed.


This change is Reviewable

This is the same mode as Gecko uses and will get better codegen
once rust-lang/rust#45012 has landed.
@nox
Copy link
Contributor

nox commented Oct 6, 2017

What does that imply for other users of webrender?

@jrmuizel
Copy link
Collaborator Author

jrmuizel commented Oct 6, 2017

They will get worse performance until Rust supports emitting noalias metadata in code that can unwind. Although even then, having to unwind will always constrain the optimizer.

@Gankra
Copy link
Contributor

Gankra commented Oct 6, 2017

I think the top crate wins, so servo will choose to ignore this directive. However this will make wrench use panic=abort.

@Gankra
Copy link
Contributor

Gankra commented Oct 6, 2017

Actually I think servo/gecko won't see this at all, since it's in the workspace, and not the crates.

@nox
Copy link
Contributor

nox commented Oct 6, 2017

@gankro Ok, I thought that this would set panic=abort for any user of this crate, and thus that any panic in WR would have made their program abort. That would have been a blocker in my book, but it seems I was wrong anyway so disregard me.

@Gankra
Copy link
Contributor

Gankra commented Oct 6, 2017

Yeah I totally get it, but it should be fine?

@glennw
Copy link
Member

glennw commented Oct 8, 2017

I confirmed in a local servo build that a panic in WR doesn't abort the process with this change, so I'm assuming that means it's OK to merge.

@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit 6af8c83 has been approved by glennw

@bors-servo
Copy link
Contributor

⌛ Testing commit 6af8c83 with merge 16251d8...

bors-servo pushed a commit that referenced this pull request Oct 8, 2017
Use panic=abort instead of panic=unwind

This is the same mode as Gecko uses and will get better codegen
once rust-lang/rust#45012 has landed.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/1825)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

☀️ Test successful - status-appveyor, status-travis
Approved by: glennw
Pushing 16251d8 to master...

@bors-servo bors-servo merged commit 6af8c83 into servo:master Oct 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants