Fix argument types in jniargtests

[knn-k]

This commit fixes the argument types from jint to jboolean in some of
the JNI functions for jniargtests.

Signed-off-by: KONNO Kazuhiro konno@jp.ibm.com

[knn-k]

jenkins test sanity all jdk11

[knn-k]

jenkins test sanity amac jdk11

[knn-k]

You can find the declarations of those native methods in the Java code side here.

openj9/test/functional/NativeTest/jniargtests/JniArgTests.java

Lines 3699 to 3703 in c0f5771

	native boolean nativeZIZIZIZIZIrZ(boolean arg1, int arg2, boolean arg3, int arg4, boolean arg5, int arg6, boolean arg7, int arg8, boolean arg9, int arg10);
	native boolean nativeIZIZIZIZIZrZ(int arg1, boolean arg2, int arg3, boolean arg4, int arg5, boolean arg6, int arg7, boolean arg8, int arg9, boolean arg10);
	native boolean nativeZZZZZZZZZZrZ(boolean arg1, boolean arg2, boolean arg3, boolean arg4, boolean arg5, boolean arg6, boolean arg7, boolean arg8, boolean arg9, boolean arg10);

[knn-k]

Please ignore the failures with aarch64_mac. I will track them in #14500.

[0xdaryl]

@llxia : please review or delegate

[llxia]

https://openj9-jenkins.osuosl.org/job/Test_openjdk11_j9_sanity.functional_aarch64_linux_Personal/85/ got canceled due to timeout. Relaunching https://openj9-jenkins.osuosl.org/job/Test_openjdk11_j9_sanity.functional_aarch64_linux_Personal/86/

[llxia]

all aarch_linux are offline https://openj9-jenkins.osuosl.org/label/ci.role.test&&sw.os.linux&&hw.arch.aarch64/

[pshipton]

See https://openj9.slack.com/archives/CDS7QE9HB/p1645138685402399
They are very slow to download, and have been timing out trying to get through nightly, acceptance and PR testing. We want to try a restart of the machine.

[knn-k]

I ran jniargtests with this PR on AArch64 Linux locally, and it was successful.

[knn-k]

I also ran sanity.functional on AArch64 Linux as an internal jenkins job: job/Test_openjdk11_j9_sanity.functional_aarch64_linux_Personal/119/

[knn-k]

jenkins test sanity alinux64 jdk11

[knn-k]

I found the type inconsistency in the JNI arguments was introduced by #4569.
I think we need additional comments in the test code if the type inconsistency is intentional.

By the way, you cannot extend to jint when passing jboolean on AArch64 macOS, because their alignment and size on the stack are different.

• jboolean: Native type is unsigned char; 1-byte alignment (Corrected from signed char)
• jint: Native type is int; 4-byte alignment

@ChengJin01 Do you have any comment about this?

[ChengJin01]

@knn-k, The fix at #4569 was used to avoid an unexpected bug that converted boolean to jint in OpenJDK (please see the findings plus explanation starting from #4193 (comment)) which was detected by the SwingSet2 demo program on x86 (the bug was eventually fixed in OpenJDK and replaced back to jboolean later in 2019) . The idea of the fix was to ensure it works for both case (boolean to jboolean or boolean to jint) by extending boolean(1byte) to int(4bytes) in native in OpenJ9 prior to FFI call, in which case it should work on all platforms regardless of endianness. So all the test cases were specially designed to verify the fix with different combination of lineups in terms of argument lists when conversing from boolean to jboolean or jint.

If the test cases are never verified on macOS/Aarch64, there might be missing code in OpenJ9 to address this situation in the files at https://github.com/eclipse-openj9/openj9/pull/4569/files.

In addition, I notice there were only two failing subtests nativeZZZZZZZZZZrZ and nativeIIIIIZZZZZrZ at https://openj9-jenkins.osuosl.org/job/Test_openjdk11_j9_sanity.functional_aarch64_mac_Personal/1/tapResults/, and only two failing arguments arg8 and arg10 while other arguments starting from arg1 to arg7 and arg9 passed.
e.g.

Argument error:  nativeZZZZZZZZZZrZ::arg8 got: 00000000 expected: 00000001
Argument error:  nativeZZZZZZZZZZrZ::arg10 got: 00000000 expected: 00000001
Argument error:  nativeIIIIIZZZZZrZ::arg8 got: 00000000 expected: 00000001
Argument error:  nativeIIIIIZZZZZrZ::arg10 got: 00000000 expected: 00000001

against the test code at /runtime/tests/jniarg/args_10.c

jboolean JNICALL Java_JniArgTests_nativeZZZZZZZZZZrZ(JNIEnv *p_env, jobject p_this, jint arg1, jint arg2, jint arg3, jint arg4, jint arg5, jint arg6, jint arg7, jint arg8, jint arg9, jint arg10)
{

and /test/functional/NativeTest/jniargtests/JniArgTests.java

		retval_boolean = nativeZZZZZZZZZZrZ(test_jboolean[1], test_jboolean[0], test_jboolean[1], test_jboolean[0], test_jboolean[1], test_jboolean[0], test_jboolean[1], test_jboolean[0], test_jboolean[1], test_jboolean[0]);
		if (retval_boolean != test_jboolean[0]) {
			logRetValError("Return value error:  nativeZZZZZZZZZZrZ got: " + retval_boolean + " expected: " + test_jboolean[0]);
		}

My understanding is, if the test doesn't work on macOS/Aarch64, most of test items in these subtests (when conversing from boolean to int should end up with failure out there but there was no failing error from other arguments in the same subtest or from other subtests, such as nativeZIZIZIZIZIrZ, which doesn't make any sense to me.

/test/functional/NativeTest/jniargtests/JniArgTests.java

	final boolean test_jboolean[] = {true, false};

		retval_boolean = nativeZIZIZIZIZIrZ(test_jboolean[0]<----- the fist argument is boolean
test_jint[1], test_jboolean[1], test_jint[2], test_jboolean[0], test_jint[3], test_jboolean[1], test_jint[4], test_jboolean[0], test_jint[5]);
		if (retval_boolean != test_jboolean[0]) {
			logRetValError("Return value error:  nativeZIZIZIZIZIrZ got: " + retval_boolean + " expected: " + test_jboolean[0]);
		}

/runtime/tests/jniarg/args_10.c

const jboolean test_jboolean[] = { JNI_TRUE, JNI_FALSE };  //  runtime/tests/jniarg/cdefs.c

jboolean JNICALL Java_JniArgTests_nativeZIZIZIZIZIrZ(JNIEnv *p_env, jobject p_this, jint arg1, <--- converted from boolean
 jint arg2, jint arg3, jint arg4, jint arg5, jint arg6, jint arg7, jint arg8, jint arg9, jint arg10)
{
	J9JavaVM *javaVM = getJ9JavaVM(p_env);
	PORT_ACCESS_FROM_JAVAVM(javaVM);

	jniTests++;

	if (arg1 != test_jboolean[0]) {
		cFailure_jint(PORTLIB, "nativeZIZIZIZIZIrZ", 1, arg1, test_jboolean[0]);
	}

So I'd suggest to check why other subtests in the test suites still passed on macOS/Aarch64 to understand what really happened to code/tests involved.

[knn-k]

@ChengJin01 The situation is a little bit complicated to describe.

• Bytecode interpreter passes a boolean argument to JNI as four-byte integer argument by the change introduced in Extend booleans to the int type in the native #4569. If the argument type declaration is consistent between the Java code and the native code, the native code on AArch64 macOS assumes the argument is packed at 1-byte boundary. This is against the bytecode interpreter's expectation.
• JIT compiler for AArch64 macOS passes a boolean argument to JNI as a single-byte integer argument in the same way as it does with a byte argument. JIT compiler gives expected results if the argument type declaration is consistent between the Java code and the native code.

Let's look at the nativeIIIIIZZZZZrZ case. The test code declares the type of arg6 - arg10 correctly as jboolean.

openj9/runtime/tests/jniarg/args_10.c

Line 168 in ce1c984

jboolean JNICALL Java_JniArgTests_nativeIIIIIZZZZZrZ(JNIEnv *p_env, jobject p_this, jint arg1, jint arg2, jint arg3, jint arg4, jint arg5, jboolean arg6, jboolean arg7, jboolean arg8, jboolean arg9, jboolean arg10)

AArch64 macOS uses 8 GPRs for passing integer/pointer arguments. The first two GPRs are occupied by JNIEnv* and jobject, and arg7 - arg10 are passed via the C stack in this case. The native code generated by the C compiler expects the placement of arg7 - arg10 in the C stack to be:

• arg7: [x29, 16], where x29 is the frame pointer
• arg8: [x29, 17]
• arg9: [x29, 18]
• arg10: [x29, 19]

The bytecode interpreter, however, places arg7 (boolean 0) as 4-byte data at [x29, 16], arg8 at [x29, 20], etc. The JNI native code loads the 4-byte zeros into arg7 - arg10 as the result, and that causes the checks for arg8 and arg10 fail because they expect boolean 1.

When nativeIIIIIZZZZZrZ is called by direct JNI call from JITed code, the JIT compiler places the arguments for arg7 - arg10 correctly, packed in 4 bytes from [x29, 16].

[knn-k]

Look at nativeIZIZIZIZIZrZ next.
The native code on AArch64 macoS assumes the placement of arg7 - arg10 in the C stack to be:

• arg7 (jint): [x29, 16]
• arg8 (jboolean): [x29, 20]
• arg9 (jint): [x29, 24]
• arg10 (jboolean): [x29, 28]

jint arguments are always placed at the 4-byte boundary. In addition to that, this is a little-endian platform. You can load expected values for arg8 and arg10 using "load byte" instructions. This is why both the bytecode interpreter and the JIT compiler pass this test case.

[knn-k]

Is it better to have this discussion anywhere else? Open a separate issue?

[knn-k]

Is there any problem in fully reverting PR #4569? The original problem with GifImageDecoder has been already fixed in the OpenJDK side.

[ChengJin01]

@knn-k,

1. The fix at Extend booleans to the int type in the native #4569 doesn't change anything but offers the address of boolean on the java stack prior to FFI call to ensure there is no garbage data introduced in other 3 bytes when the conversion from boolean to 'jint` occurs in applications. I have to admit it seems pretty weird that it works on all platforms except macOS/Aarch64. What happens to Linux/Aarch64 or any difference in there as compared to macOS/Aarch64 in the same test cases?

2. Is there any problem in fully reverting PR Extend booleans to the int type in the native #4569? The original problem with GifImageDecoder has been already fixed in the OpenJDK side.

Technically there should be no problem if reverting this PR but it won't totally solve the issue and will end up with unexpected behaviours if the conversion from boolean to jint exists somewhere else in OpenJDK or in any existing user applications (you might notice OpenJDK/Hostpot still works with the bug, as mentioned at #4193 (comment), which means Hotspot already addressed this case in code whether nor not the conversion occurs, even though the bug in OpenJDK was fixed later).

So you could only revert the PR on macOS/Aarch64 for the moment (and disable all the test cases on macOS/Aarch64 as they become useless at this point) while keeping the fix for all other platforms, in which case you might need to come up with a better solution to macOS/Aarch64 if the conversion still occurs on macOS/Aarch64 at that time.

[knn-k]

Thank you for your response, @ChengJin01 .
AArch64 Linux uses the C stack in a different way. The arguments passed via the C stack are always aligned at 8-byte boundary regardless of their sizes: from jboolean (1 byte) to jlong (8 bytes). That is the reason why AArch64 Linux passes those type-inconsistent testcases.
See also my comment in #14500 (comment) .

I am canceling this PR, and going to open a new PR.

[knn-k]

I opened Issue #14574 for further discussion on the type conversion.

[knn-k]

I also opened PR #14575 for reverting the type extension change on AArch64 macOS as suggested.