-
Notifications
You must be signed in to change notification settings - Fork 13k
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
move to the WIP Rust port of compiler-rt #37802
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
@TimNN Right now, compiler-builtins uses Also, I think that I think that we have to update the compiler-rt submodule in the compiler-builtins repo though. To pick up the fixes (on the C side) for the bugs you linked. |
|
I think so as well. So, from what I remember every thing which has a Taking a quick look at the rust compiler-rt sources, I think that the only intrinsics that could currently be problematic are the
Again, taking a look at the rust compiler-rt sources, this would probably not have been strictly necessary, since the affected Note that, as far as I can tell, actually implementing the |
Ok, current steps for dealing with ARM:
I think that's it, right? Perhaps we can go ahead and update to use @japaric I think one thing that'll be very useful is to run your smoke repo on this PR. It's helped detect a number of compiler-rt regressions in the past! I suspect there's at least one non-x86 regression waiting for us in this PR... Do you know if it'd be possible to run smoke against this PR ahead of time? Or perhaps just the nightly after landing? Presumably if everything goes wrong we can just revert. Also it looks like |
If you can hand me binary releases (that include this change) of rustc + cross-std for all the targets that smoke tests then, yeah, it should be possible. Otherwise, trying to bootstrap rustc inside the Travis workers will probably timeout.
This would be the easiest thing to do. |
Ok, sounds good to me |
☔ The latest upstream changes (presumably #37849) made this pull request unmergeable. Please resolve the merge conflicts. |
I think the |
@japaric triage ping on this, do you recall the next steps here? |
@alexcrichton I know that at least rust-lang/compiler-builtins#133 blocks this PR, as using the C code is not enough as it only works on 64 bit non windows platforms (same reason as why the i128 pr had to put them into the crate the first place). Disclaimer though, I'm not 100% sure the PR I've linked above works if its fully integrated into the compiler (as I couldn't test the integration without doing it myself...). |
@est31 thanks! Makes sense to me. |
I've updated the integration metabug in rust-lang/compiler-builtins#66. I'm going to close this PR until all the TODO items in that list are done. |
Couldn't reopen #36992 for some reason so I'm sending a new PR.
r? @alexcrichton