-
Notifications
You must be signed in to change notification settings - Fork 707
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
Update struct support in embedded libffi #19468
Conversation
@r30shah Draft PR up for you review |
0a997b7
to
e2ed14f
Compare
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.
Overall change looks good to me except minor nitpicks. I will go through the changes once again tonight - but do not expect any changes that would change the functionality completely.
Hi @AustinWells thanks a lot for the contribution, can I request you to fix the line endings as well (I do understand lot of this are just inherited), but would be good to get them resolved. Also as I commented, this is in a good space and given that we have tested out this with JDK, PR should be marked ready for review (Without that, wouldn't be able to merge) |
@r30shah I've resolved the comments as requested and fixed up the line endings/whitespace. |
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.
Last minor nitpicks - Once you make changes, please squash the commits. I will run the z/OS tests internally before merging this one.
Adds support for passing struct type arguments and receiving struct type returns in libffi - Struct arguments are now properly loaded into registers and the argument area in sys64z.s - Struct returns are now appropriately handled in sys64z.s - Doesn't pad argument buffer in ffi_prep_args to save space. Signed-off-by: Austin Wells <[email protected]>
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.
Changes LGTM - I have started internal z/OS tests with Java8 and Java11. Once those finishes and there are not failures, will go ahead and merge this PR.
Java8 on z/OS Build number : |
Internal tests passes. Merging this one. |
The change enables the excluded tests specific to the default library loading and struct given the related issues have been resolved via ibmruntimes/openj9-openjdk-jdk21#159 and eclipse-openj9#19468. Signed-off-by: ChengJin01 <[email protected]>
The change enables the excluded tests specific to the default library loading and struct given the related issues have been resolved via ibmruntimes/openj9-openjdk-jdk21#159 and eclipse-openj9#19468. Related: [Internal 441](https://github.ibm.com/runtimes/javanext/issues/441) Signed-off-by: ChengJin01 <[email protected]>
The change enables the excluded tests specific to the default library loading and struct given the related issues have been resolved via ibmruntimes/openj9-openjdk-jdk21#159 and eclipse-openj9#19468. Related: [Internal 441](https://github.ibm.com/runtimes/javanext/issues/441) Signed-off-by: ChengJin01 <[email protected]>
Adds support for passing struct type arguments and receiving struct type returns in libffi
- Struct arguments are now properly loaded into registers and the argument area in sys64z.s
- Struct returns are now appropriately handled in sys64z.s
- Doesn't pad argument buffer in ffi_prep_args to save space.