-
Notifications
You must be signed in to change notification settings - Fork 24
Add powerpc64le Linux support #61
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
Conversation
Amanieu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did some testing on powerpc64 qemu (with 2b4700e to add the missing ucontext support). Currently this seems to crash even on the base smoke tests. The issue seems to be that function pointers on powerpc actually point to a 3-word descriptor with pc, r2 and r11 values. See https://rust.godbolt.org/z/c9enqqej7 for an example.
This is an initial review, I will do a more in-depth one once the initial problems are fixed.
Thanks for the review, @Amanieu! From the Compiler Explorer session (https://rust.godbolt.org/z/c9enqqej7, it looks like you compiled with The implementation runs 16 out of 21 unit tests successfully on RedHat. |
|
I see, powerpc64 uses the ELFv1 ABI while powerc64le uses the ELFv2 ABI. You should be able to explicitly handle these with rust-lang/rust#142321. Although for now if might be ok to just |
|
Right, disregard my previous comments on the stack layout, I need to re-read the ABI document in detail. |
|
I've tried to clean up the stack layout for the coroutine here: a783805 Let me know if you think this will work or if I missed anything. |
- change the stack layout as per suggestion, - save/restore the TOC register R2.
78efe8e to
576c081
Compare
|
I've updated the stack layout as you suggested. Thank you! |
|
I spent a few days reworking the code and optimizing the stack layout. This now passes all the tests. Thanks a lot for pushing this forward! The only missing step before this can be merged is that we need a new release of the libc crate that includes the ucontext definition for PPC. |
|
Also I would appreciate if you reviewed my changes to ensure I didn't miss anything. |
|
Also note that since rust-lang/rust#148492, r29 no longer needs to be saved and can just be declared as a clobber. |
|
The changes look great, thank you so much! I really appreciate it! |
|
The dependent PR that adds |
This also fixes various issues so that all the tests pass.
83927d2 to
a5d4d01
Compare
a5d4d01 to
be2b785
Compare
|
Thank you very much, @Amanieu! This patch wouldn’t have been in such great shape without your help and guidance. |
This PR adds support for
powerpc64leLinux.I'm not very familiar with Valgrind, so it’s currently unsupported on powerpc64 in this implementation. Any insights or guidance would be greatly appreciated.
Note: the test has dependency on the
ucontext_tstructure from thelibccrate. PR Add ucontext_t and {get,set,make,swap}context for powerpc64-linux (cont.) has been approved but not yet included in thelibcstable branch and release yet.