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

Increment counter correctly in FFI Upcall on Z #16326

Merged
merged 1 commit into from Nov 16, 2022

Conversation

dchopra001
Copy link
Contributor

When getArgPointer is invoked while an upcall is performed the gprIndex must be incremented if hidden parameters have to be accounted for. This commit implements the suggested change.

Fixes: #16214

Signed-off-by: Dhruv Chopra Dhruv.C.Chopra@ibm.com

@dchopra001
Copy link
Contributor Author

Tagging @r30shah for review.

@ChengJin01 FYI

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.

Looks good to me. We should update comment to reflect that, for the complex return type GPR2 contains address of the return value buffer.

@r30shah
Copy link
Contributor

r30shah commented Nov 15, 2022

@dchopra001 I would assume we you have verified the changes with FFI tests, right?
@joransiu Can you please review and merge this change?

@dchopra001
Copy link
Contributor Author

@dchopra001 I would assume we you have verified the changes with FFI tests, right?

Yes, all the tests in TestUpcallStack.java now pass from my local testing.

@ChengJin01 is there any PR testing we can run to verify the change? Or alternatively did you want to also verify if this fixes the issue on Z?

@ChengJin01
Copy link
Contributor

@ChengJin01 is there any PR testing we can run to verify the change? Or alternatively did you want to also verify if this fixes the issue on Z?

Please make sure the following upcall specific Jtreg test suites at https://github.com/ibmruntimes/openj9-openjdk-jdk19/tree/openj9/test/jdk/java/foreign pass without any issue given the problem here was captured by Jtreg test suites.

https://github.com/ibmruntimes/openj9-openjdk-jdk19/blob/openj9/test/jdk/java/foreign/TestUpcallScope.java
https://github.com/ibmruntimes/openj9-openjdk-jdk19/blob/openj9/test/jdk/java/foreign/TestUpcallAsync.java
https://github.com/ibmruntimes/openj9-openjdk-jdk19/blob/openj9/test/jdk/java/foreign/TestUpcallHighArity.java
https://github.com/ibmruntimes/openj9-openjdk-jdk19/blob/openj9/test/jdk/java/foreign/TestUpcallScope.java
https://github.com/ibmruntimes/openj9-openjdk-jdk19/blob/openj9/test/jdk/java/foreign/TestUpcallStructScope.java

@dchopra001
Copy link
Contributor Author

Please make sure the following upcall specific Jtreg test suites at https://github.com/ibmruntimes/openj9-openjdk-jdk19/tree/openj9/test/jdk/java/foreign pass without any issue given the problem here was captured by Jtreg test suites.

I ran each of the listed tests twice and saw no failures. So I think we should be good to merge this.

@joransiu
Copy link
Member

We should update comment to reflect that, for the complex return type GPR2 contains address of the return value buffer.

@dchopra001 : Do you want to update the comment as per @r30shah's suggestion?

When getArgPointer is invoked while an upcall is performed
the gprIndex must be incremented if hidden parameters have
to be accounted for. This commit implements the suggested
change.

Fixes: eclipse-openj9#16214

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

dchopra001 commented Nov 15, 2022

@dchopra001 : Do you want to update the comment as per @r30shah's suggestion?

Sorry forgot to address that. The comments should be there now in de300b8

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.

LGTM, good to merge.

@joransiu
Copy link
Member

jenkins test sanity zlinux jdk19

@joransiu joransiu merged commit 4c499f8 into eclipse-openj9:master Nov 16, 2022
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.

[Jtreg/FFI] Invalid struct offset returned from getArgPointer() in upcall on zLinux
4 participants