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.

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

Merged
merged 1 commit into from
May 27, 2024

Conversation

AustinWells
Copy link
Contributor

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]>

@AustinWells AustinWells marked this pull request as draft May 8, 2024 15:44
@AustinWells
Copy link
Contributor Author

@r30shah Draft PR up for you review

@AustinWells AustinWells force-pushed the master branch 2 times, most recently from 0a997b7 to e2ed14f Compare May 9, 2024 14:50
Copy link
Contributor

@r30shah r30shah left a 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.

runtime/libffi/z/ffi64.c Outdated Show resolved Hide resolved
runtime/libffi/z/ffi64.c Outdated Show resolved Hide resolved
runtime/libffi/z/sysvz64.s Outdated Show resolved Hide resolved
runtime/libffi/z/sysvz64.s Outdated Show resolved Hide resolved
runtime/libffi/z/sysvz64.s Outdated Show resolved Hide resolved
runtime/libffi/z/sysvz64.s Outdated Show resolved Hide resolved
runtime/libffi/z/sysvz64.s Outdated Show resolved Hide resolved
runtime/libffi/z/sysvz64.s Outdated Show resolved Hide resolved
runtime/libffi/z/sysvz64.s Outdated Show resolved Hide resolved
runtime/libffi/z/sysvz64.s Outdated Show resolved Hide resolved
@r30shah
Copy link
Contributor

r30shah commented May 23, 2024

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)

@AustinWells AustinWells marked this pull request as ready for review May 24, 2024 02:50
@AustinWells
Copy link
Contributor Author

@r30shah I've resolved the comments as requested and fixed up the line endings/whitespace.

@AustinWells AustinWells changed the title [WIP] Update struct support in embedded libffi Update struct support in embedded libffi May 24, 2024
Copy link
Contributor

@r30shah r30shah left a 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.

runtime/libffi/z/ffi64.c Outdated Show resolved Hide resolved
runtime/libffi/z/sysvz64.s Outdated Show resolved Hide resolved
runtime/libffi/z/sysvz64.s Outdated Show resolved Hide resolved
runtime/libffi/z/sysvz64.s Outdated Show resolved Hide resolved
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]>
Copy link
Contributor

@r30shah r30shah left a 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.

@r30shah
Copy link
Contributor

r30shah commented May 24, 2024

Java8 on z/OS Build number : build_info.php?build_id=71589
Java 11 on z/OS Build number : job/Pipeline-Build-Test-Personal/22291/

@r30shah
Copy link
Contributor

r30shah commented May 27, 2024

Internal tests passes. Merging this one.

@r30shah r30shah merged commit e61548b into eclipse-openj9:master May 27, 2024
2 checks passed
ChengJin01 added a commit to ChengJin01/openj9 that referenced this pull request May 28, 2024
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]>
ChengJin01 added a commit to ChengJin01/openj9 that referenced this pull request May 28, 2024
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]>
AswathySK pushed a commit to AswathySK/openj9 that referenced this pull request Jun 4, 2024
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants