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

Enable Jtreg test suites for FFI on Power & zLinux in JDK21 #3

Conversation

ChengJin01
Copy link

The changes add Power & zLinux support to these
Jtreg test suites in JDK21 plus a few compilation
fixes in the test cases to ensure they are executed
correctly on all supported platforms.

Note:
The PR is part of the FFI work at
eclipse-openj9/openj9#16951

Signed-off-by: Cheng Jin jincheng@ca.ibm.com

The changes add Power & zLinux support to these
Jtreg test suites in JDK21 plus a few compilation
fixes in the test cases to ensure they are executed
correctly on all supported platforms.

Note:
The PR is part of the FFI work at
eclipse-openj9/openj9#16951

Signed-off-by: Cheng Jin <jincheng@ca.ibm.com>
@ChengJin01
Copy link
Author

Reviewer: @keithc-ca
FYI: @tajila, @pshipton

@keithc-ca
Copy link
Member

I think we should wait for ed82e91 to be merged into the openj9 branch which will eliminate the need for these changes.

#4 and a backport of ibmruntimes/openj9-openjdk-jdk#608 should land soon.

@@ -64,13 +65,13 @@ public void testInvalidSymbolLookup() {

@Test
public void testVariableSymbolLookup() {
MemorySegment segment = SymbolLookup.loaderLookup().find("c").get().reinterpret(1);
MemorySegment segment = SymbolLookup.loaderLookup().find("c").get().reinterpret(JAVA_INT.byteSize());
Copy link
Member

Choose a reason for hiding this comment

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

I think we just want to remove the call to reinterpret(); see https://github.com/ibmruntimes/openj9-openjdk-jdk/blob/openj9/test/jdk/java/foreign/TestClassLoaderFindNative.java#L68; this file should be the same as in jdknext.

Copy link
Author

Choose a reason for hiding this comment

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

We still need reinterpret()to ensure it works on AIX (BE); otherwise, it will end up with OutOfBoundsException in such case.

@@ -192,7 +192,7 @@ private static void check(int index, MemorySegment ptr, List<Arg> args) {
* address in java on the Big-Endian(BE) platforms such as AIX.
*/
if (isAixOS && (layout instanceof ValueLayout) && (((ValueLayout)layout).carrier() == float.class)) {
MemorySegment doubleSegmt = MemorySegment.ofAddress(ptr, JAVA_DOUBLE.byteSize(), session);
MemorySegment doubleSegmt = MemorySegment.ofAddress(ptr.address()).reinterpret(JAVA_DOUBLE.byteSize());
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

It is hard to determine for the moment given the test suite is not intended for AIX. So I believe it still needed to deal with the conversion on AIX.

@ChengJin01
Copy link
Author

ChengJin01 commented Jun 21, 2023

I'd like to close the PR (which is outdated as compared to the new test suites) given the changed test code in this PR has not yet been verified with the last test changes (just merged) on all supported platform. So we can open another PR with these test changes if they are still required on AIX.

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.

2 participants