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.

Sign up for GitHub

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

Jump to bottom

Mark all extern functions as nounwind #28358

Merged
merged 3 commits into from
Sep 14, 2015
Merged

Mark all extern functions as nounwind #28358

merged 3 commits into from
Sep 14, 2015

Conversation

dotdash
Copy link
Contributor

@dotdash dotdash commented Sep 11, 2015

This allows to skip the codegen for all the unneeded landing pads, reducing code size across the board by about 2-5%, depending on the crate. Compile times seem to be pretty unaffected though :-/

@rust-highfive
Copy link
Collaborator

r? @huonw

(rust_highfive has picked a reviewer for you, use r? to override)

@alexcrichton
Copy link
Member

Thanks @dotdash! I'm excited to see how this plays out. Some thoughts of mine:

  • This adds #[nounwind], but doesn't end up adding it anywhere, did you mean to attach this to the allocation functions? If not, perhaps this could be left out for now?
  • Could this have some codegen tests? E.g. testing for the nounwind attribute emission, external functions are nounwind by default, and calling a nounwind function with cleanup in scope doesn't use invoke but instead uses call.

@bors
Copy link
Contributor

bors commented Sep 11, 2015

☔ The latest upstream changes (presumably #28306) made this pull request unmergeable. Please resolve the merge conflicts.

@huonw huonw assigned alexcrichton and unassigned huonw Sep 12, 2015
@dotdash dotdash force-pushed the nounwind branch 2 times, most recently from 494e66b to be96d9f Compare September 12, 2015 17:48
@dotdash
Copy link
Contributor Author

dotdash commented Sep 12, 2015

Added codegen tests for the attributes, but not for the call (as discussed on IRC) because we never use invoke an external functions anyway. Also removed the check for nounwind in invoke() because that's for rust functions only, and they don't get marked nounwind yet.

@alexcrichton
Copy link
Member

Looks good to me, thanks! Can you also mark the RaiseException function in both places as being able to unwind as well? Other than that r=me

@dotdash
Copy link
Contributor Author

dotdash commented Sep 13, 2015

@bors r=alexcrichton bd6da30

@bors
Copy link
Contributor

bors commented Sep 14, 2015

⌛ Testing commit bd6da30 with merge 21ba40d...

@bors
Copy link
Contributor

bors commented Sep 14, 2015

💔 Test failed - auto-win-msvc-64-opt

@dotdash
Copy link
Contributor Author

dotdash commented Sep 14, 2015

@bors retry

I'm confused about this error...

@bors
Copy link
Contributor

bors commented Sep 14, 2015

⌛ Testing commit bd6da30 with merge dcf66cb...

@bors
Copy link
Contributor

bors commented Sep 14, 2015

💔 Test failed - auto-win-msvc-64-opt

@dotdash
Mark all extern functions as nounwind
3ef75d5 
Unwinding across an FFI boundary is undefined behaviour, so we can mark
all external function as nounwind. The obvious exception are those
functions that actually perform the unwinding.
@dotdash
Copy link
Contributor Author

dotdash commented Sep 14, 2015

@bors r=alexcrichton 3ef75d5

@bors
Copy link
Contributor

bors commented Sep 14, 2015

⌛ Testing commit 3ef75d5 with merge 2d4ae52...

bors added a commit that referenced this pull request Sep 14, 2015
@bors
Auto merge of #28358 - dotdash:nounwind, r=alexcrichton
2d4ae52 
This allows to skip the codegen for all the unneeded landing pads, reducing code size across the board by about 2-5%, depending on the crate. Compile times seem to be pretty unaffected though :-/
@bors
Copy link
Contributor

bors commented Sep 14, 2015

💔 Test failed - auto-win-msvc-64-opt

@dotdash
Copy link
Contributor Author

dotdash commented Sep 14, 2015

@bors retry

At least it hit a known intermittent failure this time...

@bors
Copy link
Contributor

bors commented Sep 14, 2015

⚡ Previous build results for auto-win-msvc-32-opt are reusable. Rebuilding only auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-64-opt...

@bors
Copy link
Contributor

bors commented Sep 14, 2015

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt

@bors bors merged commit 3ef75d5 into rust-lang:master Sep 14, 2015
@bors bors mentioned this pull request Sep 14, 2015
Merged
@dotdash dotdash deleted the nounwind branch January 15, 2016 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@alexcrichton alexcrichton

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dotdash @rust-highfive @alexcrichton @bors @huonw