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 downcall & upcall test suites for Java 17/18/19 #15310

Conversation

ChengJin01
Copy link
Contributor

The changes include all test suites intended for downcall
and upcall in Java17, 18 and 19 which can be enabled
on the following platforms:
1)downcall: all supported platforms
2)upcall: AIX/PPC and Linux/PPC64LE

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

@ChengJin01
Copy link
Contributor Author

ChengJin01 commented Jun 13, 2022

The change also belongs to the issue intended for the upcall work at #15068.

Note:

  1. the test suites cover the downcall & upcall tests in Java 19 and upcall specific tests in Java 17 & 18.
  2. the PR depends on all code included in Design and Implementation of Foreign Linker API: Upcall #15068. So the code there must be merged at first before the test suites.

Reviewer: @tajila, @llxia (for the test framework)
FYI: @DanHeidinga, @pshipton, @gacholio

@tajila
Copy link
Contributor

tajila commented Jun 14, 2022

@llxia Please review these changes

@tajila tajila requested a review from llxia June 14, 2022 13:43
@ChengJin01 ChengJin01 force-pushed the enable_ffi_downcall_upcall_tests_jdk17_18_19 branch from 3082fa4 to 82d08a7 Compare June 15, 2022 18:16
Copy link
Contributor

@llxia llxia left a comment

Choose a reason for hiding this comment

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

test changes look good to me

@tajila
Copy link
Contributor

tajila commented Jun 17, 2022

jenkins test sanity amac jdk17

@tajila
Copy link
Contributor

tajila commented Jun 17, 2022

jenkins test sanity plinux jdk19

@ChengJin01
Copy link
Contributor Author

ChengJin01 commented Jun 17, 2022

The test failures on MacOS/Aarch64 at https://openj9-jenkins.osuosl.org/job/Test_openjdk17_j9_sanity.functional_aarch64_mac_Personal/60/tapResults/
e.g.

FAILED: test_vprintfFromDefaultLibWithVaList
java.util.NoSuchElementException: No value present
	at java.base/java.util.Optional.get(Optional.java:143)
	at org.openj9.test.jep389.valist.VaListTests.test_vprintfFromDefaultLibWithVaList(VaListTests.java:125)

was caused by the default library loading issue already detected previously on macOS in #14487 and was resolved with our own fix in OpenJDK at https://github.com/ibmruntimes/openj9-openjdk-jdk17/pull/112/files#diff-9658a8d1507ae5132a614d4c38ed6589ebab5bed19c3d03f726585aeb49dce29, https://github.com/ibmruntimes/openj9-openjdk-jdk18/pull/57/files#diff-9658a8d1507ae5132a614d4c38ed6589ebab5bed19c3d03f726585aeb49dce29, https://github.com/ibmruntimes/openj9-openjdk-jdk19/pull/5/files#diff-9658a8d1507ae5132a614d4c38ed6589ebab5bed19c3d03f726585aeb49dce29 and https://github.com/ibmruntimes/openj9-openjdk-jdk/pull/457/files#diff-9658a8d1507ae5132a614d4c38ed6589ebab5bed19c3d03f726585aeb49dce29 (not yet merged).

In addition, the tests on JDK19 are invalid given there is no our code in OpenJDK (the FFI specific code was totally removed in JDKnext due to the latest APIs in Java19) and all OpenJDK & OpenJ9 specific code are still under review. So I'll disable all test suites on all supported platforms in playlist.xml for the moment in case any partially merged code mess them up and re-enable them all after all code listed in #15068 for Java17/18/19 are merged if this PR needs to be merged individually.

@ChengJin01 ChengJin01 force-pushed the enable_ffi_downcall_upcall_tests_jdk17_18_19 branch 3 times, most recently from 50228ae to 5df601c Compare June 21, 2022 23:16
@ChengJin01 ChengJin01 force-pushed the enable_ffi_downcall_upcall_tests_jdk17_18_19 branch from 5df601c to d39eb85 Compare July 5, 2022 16:13
@tajila
Copy link
Contributor

tajila commented Jul 12, 2022

jenkins test sanity plinux jdk19

@tajila
Copy link
Contributor

tajila commented Jul 12, 2022

jenkins test sanity amac jdk17

@ChengJin01 ChengJin01 force-pushed the enable_ffi_downcall_upcall_tests_jdk17_18_19 branch 4 times, most recently from 667e81c to a971b29 Compare July 27, 2022 20:00
@ChengJin01
Copy link
Contributor Author

ChengJin01 commented Jul 27, 2022

I just updated the test cases based on the latest changes in OpenJDK and added a couple of tests requested by JIT (working on thunk code on zLinux). I also removed all Valist specific test cases as they are outdated at this point and will be totally replaced with new tests once we have Valist implemented on Power in OpenJDK.

@ChengJin01 ChengJin01 force-pushed the enable_ffi_downcall_upcall_tests_jdk17_18_19 branch 2 times, most recently from 0415284 to d975979 Compare August 17, 2022 19:37
@@ -23,6 +23,8 @@
set(OMR_ENHANCED_WARNINGS OFF)
j9vm_add_library(clinkerffitests SHARED
downcall.c
upcall.c
valist.c
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no file named valist.c in this directory, isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. The test cases for VaList are not ready and will be added once we have VaList support on Power & zLinux which I have been working on. So I just removed valist.c for the moment.

@ChengJin01 ChengJin01 force-pushed the enable_ffi_downcall_upcall_tests_jdk17_18_19 branch from b181220 to b9c0f7f Compare September 8, 2022 03:10
@ChengJin01
Copy link
Contributor Author

I just updated part of tests in UpcallMHWithStructTests.java for JDK17/18/19+ to support struct specific upcall on Linux/s390x given the padding in these tests is not required on Linux/s390x as explained in the PR intended for thunk generation code at #15596 (comment).

@ChengJin01 ChengJin01 force-pushed the enable_ffi_downcall_upcall_tests_jdk17_18_19 branch 2 times, most recently from 9c355a7 to da9331b Compare September 8, 2022 04:12
@ChengJin01
Copy link
Contributor Author

I just updated this PR to add the VaList specific test cases in JDK19+ which is intended for the VaList changes at ibmruntimes/openj9-openjdk-jdk#488 and ibmruntimes/openj9-openjdk-jdk19#27.

@ChengJin01 ChengJin01 force-pushed the enable_ffi_downcall_upcall_tests_jdk17_18_19 branch 2 times, most recently from da64ca6 to 0a70e5e Compare September 14, 2022 22:39
}

@Test
public void test_addLongAndLong2FloatsFromStructByUpcallMH() throws Throwable {
Copy link
Contributor

Choose a reason for hiding this comment

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

This test fails on Windows for both HotSpot and OpenJ9. However, the problem seems to be on the "downcall" part of the test as the elements of the struct don't even reach the C function intact (i.e., before we can even do an upcall).

For HotSpot, printing the args reaching the C function (addLongAndLong2FloatsFromStructByUpcallMH) before the upcall:

IN C: addLongAndLong2FloatsFromStruct : arg1=555555555555, arg2.elem1=388697201, arg2.elem2=0.000000, arg2.elem3=11.250000

For OpenJ9, printing the args reaching the C function before the upcall:

IN C: addLongAndLong2FloatsFromStruct : arg1=555555555555, arg2.elem1=388697201, arg2.elem2=0.000000, arg2.elem3=11.250000

Copy link
Contributor Author

@ChengJin01 ChengJin01 Sep 20, 2022

Choose a reason for hiding this comment

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

@0xdaryl, could you help double-check values[1] or ffiArgs[1] for the struct the code of inlInternalDowncallHandlerInvokeNative in downcall at

values[i] = (void *)(U_64)ffiArgs[i];
?
I'd like to know whether or not the passed-in value is correct given the test passes on other platforms.

If the passed-in struct from inlInternalDowncallHandlerInvokeNative is correct but it gets corrupted in the C native function, the problem should originate from libffi on Windows in which case we should disable this test for the moment and will need to create an issue at https://github.com/libffi/libffi/issues or https://sourceware.org/mailman/listinfo/libffi-discuss to request them to fix it up.

Copy link
Contributor Author

@ChengJin01 ChengJin01 Sep 20, 2022

Choose a reason for hiding this comment

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

I suspect the problem was caused by the wrong element type defined for elem1 in stru_Long_2_Floats at downcall.h

typedef struct stru_Long_2_Floats {
	long elem1;  <---------
	float elem2;
	float elem3;
} stru_Long_2_Floats;

in which long is 4 bytes on Windows, which should be replaced with LONG (int64_t). I will update this definition to verify again on Windows to see how it goes.

Copy link
Contributor Author

@ChengJin01 ChengJin01 Sep 21, 2022

Choose a reason for hiding this comment

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

I manually compile a JDK17 build on Windows and verified locally with the updated definition of stru_Long_2_Floats in /runtime/tests/clinkerffi/downcall.h

typedef struct stru_Long_2_Floats {
	LONG elem1;  <-------
	float elem2;
	float elem3;
} stru_Long_2_Floats;

and the test test_addLongAndLong2FloatsFromStructByUpcallMH passed on both OpenJ9 and Hotspot on Windows:

$ java -version
openjdk version "17.0.5-internal" 2022-10-18
OpenJDK Runtime Environment (build 17.0.5-internal+0-adhoc.Administrator.openj9-openjdk-jdk17)
Eclipse OpenJ9 VM (build jep389_419_ffi_upcall_v4_struct_v2_outofline_mh_v9_fin_v8_5-c3c03e474,
 JRE 17 Windows 10 amd64-64-Bit Compressed References 20220920_000000 (JIT enabled, AOT enabled
)
OpenJ9   - c3c03e474
OMR      - b1be89cf5
JCL      - e988b7def79 based on jdk-17.0.5+5)

$ java -version
openjdk version "17.0.2" 2022-01-18
OpenJDK Runtime Environment (build 17.0.2+8-86)
OpenJDK 64-Bit Server VM (build 17.0.2+8-86, mixed mode, sharing)
...
PASSED: test_addLongAndLong2FloatsFromStructByUpcallMH

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@0xdaryl, please double-check the test on your side (note that the test library clinkerffitests.dll needs to be regenerated with the latest downcall.h in this PR)

public class UpcallMHWithStructTests {
private static String osName = System.getProperty("os.name").toLowerCase();
private static String arch = System.getProperty("os.arch").toLowerCase();
private static boolean isLinuxS390x = osName.contains("linux") && arch.equals("s390x");
Copy link
Contributor

Choose a reason for hiding this comment

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

Windows does not require the structure padding either, so all places that check for isLinuxS390x will have to check for Windows as well.

I wonder if it would be better to introduce a requiresStructurePadding boolean here that is initialized to false for Z and Windows rather than adding OS checks for many of the tests. It seems a bit cleaner in my opinion.

Same for all other versions of UpcallMHWithStructTests.java.

Copy link
Contributor Author

@ChengJin01 ChengJin01 Sep 20, 2022

Choose a reason for hiding this comment

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

I will update the tests on Java17/18/19 based against the suggestion above.

Copy link
Contributor Author

@ChengJin01 ChengJin01 Sep 20, 2022

Choose a reason for hiding this comment

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

@0xdaryl, I've updated (and verified locally) the tests in Java17/18/19 by replacing the OS check with isStructPaddingNotRequired for zLinux & Win so as to minimize the changes. Please help check to see whether it works good with your thunk code on Windows.

@tajila
Copy link
Contributor

tajila commented Oct 3, 2022

@ChengJin01 is testing disabled for x86?

@ChengJin01
Copy link
Contributor Author

@tajila
Copy link
Contributor

tajila commented Oct 3, 2022

jenkins test sanity,extended plinux,zlinux,aix,amac jdk19

@ChengJin01 ChengJin01 force-pushed the enable_ffi_downcall_upcall_tests_jdk17_18_19 branch from 5b9e563 to 171327c Compare October 3, 2022 20:10
@ChengJin01
Copy link
Contributor Author

ChengJin01 commented Oct 3, 2022

The downcall & upcall related test suites are enabled in JDK17/18/19+ on AIX & pLinux, zLinux and Aarch64 and VaList tests are only enabled on AIX & pLinux in JDK19+.

@tajila
Copy link
Contributor

tajila commented Oct 4, 2022

jenkins test sanity,extended plinux,zlinux,aix,amac jdk19

@ChengJin01 ChengJin01 force-pushed the enable_ffi_downcall_upcall_tests_jdk17_18_19 branch from 171327c to 35847f6 Compare October 4, 2022 04:30
@ChengJin01
Copy link
Contributor Author

ChengJin01 commented Oct 4, 2022

I just updated a couple of latest native test code for VaList which were missing in this PR.

The changes include all test suites intended for downcall
and upcall in Java17, 18 and 19 which can be enabled
on the following platforms:
1)downcall: all supported platforms
2)upcall: AIX/PPC and Linux/PPC64LE

Signed-off-by: Cheng Jin <jincheng@ca.ibm.com>
@ChengJin01 ChengJin01 force-pushed the enable_ffi_downcall_upcall_tests_jdk17_18_19 branch from 35847f6 to c3b12b5 Compare October 4, 2022 04:34
@tajila
Copy link
Contributor

tajila commented Oct 4, 2022

jenkins test sanity,extended plinux jdk19

@tajila
Copy link
Contributor

tajila commented Oct 4, 2022

jenkins test sanity,extended zlinux,aix,amac jdk19

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants