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

Add support for structs in FFI Upcall on z/OS #19937

Merged
merged 1 commit into from
Aug 7, 2024

Conversation

dchopra001
Copy link
Contributor

This PR primarily completes the FFI Upcall feature on z/OS by adding support to handle struct arguments and return types on z/OS. In addition, it adds formatting changes and enabled the FFI Upcall functionality on z/OS.

@dchopra001 dchopra001 force-pushed the ffi_zos_structs branch 2 times, most recently from 5c43fb0 to 6fd35b7 Compare July 30, 2024 15:41
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.

I have started review, before I continue reviewing the rest of the changes posting what I found while skimming through.

Please update the PR with proper commit message explaining what this change is doing with signoff.

runtime/oti/FFIUpcallThunkGenHelpers.hpp Outdated Show resolved Hide resolved
runtime/oti/FFIUpcallThunkGenHelpers.hpp Outdated Show resolved Hide resolved
runtime/vm/mz64/UpcallThunkGen.cpp Show resolved Hide resolved
runtime/vm/mz64/UpcallThunkGen.cpp Show resolved Hide resolved
runtime/vm/mz64/UpcallThunkGen.cpp Outdated Show resolved Hide resolved
runtime/vm/mz64/UpcallThunkGen.cpp Show resolved Hide resolved
runtime/vm/mz64/UpcallThunkGen.cpp Outdated Show resolved Hide resolved
runtime/vm/mz64/UpcallThunkGen.cpp Outdated Show resolved Hide resolved
runtime/vm/mz64/UpcallThunkGen.cpp Outdated Show resolved Hide resolved
runtime/vm/mz64/UpcallThunkGen.cpp Outdated Show resolved Hide resolved
runtime/vm/mz64/UpcallThunkGen.cpp Outdated Show resolved Hide resolved
runtime/vm/mz64/UpcallThunkGen.cpp Outdated Show resolved Hide resolved
runtime/vm/mz64/UpcallThunkGen.cpp Outdated Show resolved Hide resolved
runtime/vm/mz64/UpcallThunkGen.cpp Outdated Show resolved Hide resolved
runtime/vm/mz64/UpcallThunkGen.cpp Outdated Show resolved Hide resolved
runtime/vm/mz64/UpcallThunkGen.cpp Outdated Show resolved Hide resolved
runtime/vm/mz64/UpcallThunkGen.cpp Outdated Show resolved Hide resolved
runtime/vm/mz64/UpcallThunkGen.cpp Outdated Show resolved Hide resolved
@ChengJin01 ChengJin01 added project:panama Used to track Project Panama related work os:zos labels Aug 1, 2024
runtime/vm/mz64/UpcallThunkGen.cpp Outdated Show resolved Hide resolved
runtime/vm/mz64/UpcallThunkGen.cpp Outdated Show resolved Hide resolved
runtime/vm/mz64/UpcallThunkGen.cpp Outdated Show resolved Hide resolved
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.

I think overall looks good to me, one last nitpick.

runtime/vm/mz64/UpcallThunkGen.cpp Show resolved Hide resolved
@r30shah
Copy link
Contributor

r30shah commented Aug 2, 2024

@joransiu / @zl-wang Can I also request your review on this PR as well ?

Copy link
Member

@joransiu joransiu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Two minor nits.

runtime/oti/FFIUpcallThunkGenHelpers.hpp Show resolved Hide resolved
runtime/oti/FFIUpcallThunkGenHelpers.hpp Show resolved Hide resolved
runtime/oti/FFIUpcallThunkGenHelpers.hpp Outdated Show resolved Hide resolved
runtime/oti/FFIUpcallThunkGenHelpers.hpp Outdated Show resolved Hide resolved
runtime/oti/FFIUpcallThunkGenHelpers.hpp Outdated Show resolved Hide resolved
Copy link
Contributor

@zl-wang zl-wang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dchopra001
Copy link
Contributor Author

@r30shah Commits are squashed, so we can run tests now.

@r30shah
Copy link
Contributor

r30shah commented Aug 5, 2024

Jenkins test sanity zlinux jdk21

This commit completes FFI Upcall functionality support
on z/OS by adding support to handle structs as parameters
and return types.

Signed-off-by: Dhruv Chopra <Dhruv.C.Chopra@ibm.com>
@r30shah
Copy link
Contributor

r30shah commented Aug 7, 2024

Jenkins test sanity zlinux jdk21

@r30shah
Copy link
Contributor

r30shah commented Aug 7, 2024

I have launched internal build for IBM Java 8 and also build for z/OS, Will merge this change once this builds on the PR and the one I launched internally finishes.

@r30shah
Copy link
Contributor

r30shah commented Aug 7, 2024

Merging this as JDK21 on Linux on Z test passes (Which will execute some of the changes from this PR through upcall tests) and I see Build job finishes on z/OS.

@r30shah r30shah merged commit 7c8005e into eclipse-openj9:master Aug 7, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
os:zos project:panama Used to track Project Panama related work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants