-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Mark all functions defined in compiler-builtins as nounwind #121605
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Conversation
Would it be more appropriate to apply |
I think it's fine to special case it for now. If we ever want a crate-level attribute then we could revisit this. @bors r+ |
This might improve MIR optimizations. I previously found that Queue looks light, so this might be nice to help perf triage: |
Mark all functions defined in compiler-builtins as nounwind Treat functions in compiler-builtins as nounwind. Suggested in rust-lang/compiler-builtins#578 (comment). A prerequisite for rust-lang#116088 r? `@Amanieu` cc `@RalfJung`
The job Click to see the possible cause of the failure (guessed by this bot)
|
💔 Test failed - checks-actions |
My understanding is that on its own this does nothing since the crate does not set |
Hm no that does not seem right; |
@bors r- (PR got requeued by sync) |
The error is caused by which uses Some options here:
What's the best action here? |
FWIW compiler-builtins CI is currently already broken due to #121552. I would just go with option 3. |
I believe #122580 will make this PR unnecessary. |
The justification I mentioned above still applies: #121605 (comment) |
I think this is not needed any more: rust-lang/compiler-builtins#583 landed, so compiler_builtins is now already working fine with proper c_unwind behavior. So, closing the PR (and the commit should be removed from #116088). |
Treat functions in compiler-builtins as nounwind. Suggested in rust-lang/compiler-builtins#578 (comment).
A prerequisite for #116088
r? @Amanieu
cc @RalfJung