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

[FFI/JDK21] Enable the union support in JDK21 #18291

Merged

Conversation

ChengJin01
Copy link

The changes aim to implement the union type in the FFI specific code in JD21
by mapping the simplified the union type (simulated via the struct) to the
underlying hardware.

Signed-off-by: ChengJin01 [email protected]

@ChengJin01
Copy link
Author

ChengJin01 commented Oct 17, 2023

The implementation was inspired by the libffi document at https://github.com/libffi/libffi/blob/5c6e53db873767cd2266745cebc62551958f5bee/doc/libffi.texi#L543 as to how to set the union type via libffi in native
as follows:

A union can also be emulated using @code{FFI_TYPE_STRUCT}.  In this case, however, 
you must make sure that the size and alignment match the real requirements of the union.

One simple way to do this is to ensue that each element type is laid out.  
Then, give the new structure type a single element; the size of the largest element; 
and the largest alignment seen as well.

Instead of directly changing both the java code and native code to handle the union type which is technically challenging & complex (might need to introduce a new type/symbol to differentiate the union type from other types even in the upcall thunk) and difficult from the maintenance perspective in the future, we adopted a lightweight solution to simulate the union type by laying out an array of primitive types with the same alignment as the union in java, in which case the primitive sequence can be safely mapped to the given GPRs/FPRs (or overflow/reserved area which is determined by the compiler) on the underlying hardware.

@ChengJin01
Copy link
Author

The code has been verified with the newly added union related tests and the jtreg test suite for union at https://github.com/ibmruntimes/openj9-openjdk-jdk21/blob/openj9/test/jdk/java/foreign/nested/TestNested.java on all supported platform except one issue with double related struct on AIX detected at #18287.

@ChengJin01
Copy link
Author

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

@ChengJin01 ChengJin01 requested a review from tajila October 17, 2023 03:55
@ChengJin01 ChengJin01 added comp:vm project:panama Used to track Project Panama related work jdk21 labels Oct 17, 2023
@ChengJin01 ChengJin01 force-pushed the ffi_enable_union_support_jdk21 branch from 6f536b9 to a1f4f24 Compare October 17, 2023 03:56
@ChengJin01 ChengJin01 force-pushed the ffi_enable_union_support_jdk21 branch from a1f4f24 to 4937df2 Compare October 18, 2023 21:35
@ChengJin01 ChengJin01 force-pushed the ffi_enable_union_support_jdk21 branch from 4937df2 to 19ba45c Compare October 19, 2023 15:22
@tajila tajila added this to the Java 21 (0.42) milestone Oct 19, 2023
@ChengJin01 ChengJin01 force-pushed the ffi_enable_union_support_jdk21 branch 3 times, most recently from 48f1d90 to ed7c943 Compare October 20, 2023 20:22
@ChengJin01 ChengJin01 force-pushed the ffi_enable_union_support_jdk21 branch 2 times, most recently from 4df913f to 00d802d Compare October 26, 2023 18:00
@ChengJin01 ChengJin01 force-pushed the ffi_enable_union_support_jdk21 branch 4 times, most recently from 3302ae9 to 0e00e81 Compare October 30, 2023 19:04
@keithc-ca
Copy link
Contributor

An earlier version of this had new test code for unions: What happened to those test additions?

@ChengJin01 ChengJin01 force-pushed the ffi_enable_union_support_jdk21 branch from 0e00e81 to 0205731 Compare October 30, 2023 20:22
@ChengJin01
Copy link
Author

ChengJin01 commented Oct 30, 2023

An earlier version of this had new test code for unions: What happened to those test additions?

This is the first time the union specific test suites were added to verify unions, which were previously placed in jep442 at test/functional/Java21andUp and now entirely moved to test/functional/Java21Only due to the new non-compatible APIs introduced in JDK22 mentioned at #18296 (comment)).

So we will need to copy all these test suites to test/functional/Java22andUp/src/org/openj9/test/jep454 with the updated APIs in JDK22 once this PR for JDK21 is merged.

Copy link
Contributor

@keithc-ca keithc-ca left a comment

Choose a reason for hiding this comment

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

There's also a misspelling of "strings" on line 202.

@ChengJin01
Copy link
Author

ChengJin01 commented Oct 30, 2023

On Linux x64, gcc passes union U64 objects on the stack, contrary to what this would do: The presence of a long or double members is not sufficient to determine that registers will be used...

This is not determined by our implementation in java (the method name might be misleading at this point) but rather by how the underlying libffi simulate the union with the struct (the array of primitive) based on the liffi document for union, whether the union is placed onto the registers or onto the stack. Our job in java simply guarantees that libff correctly handles it against the calling convention on the given platforms.

I will create test cases with your example ( which should be a 8 long array based on our implementation) above to see how it goes with our code.

@ChengJin01 ChengJin01 force-pushed the ffi_enable_union_support_jdk21 branch from 0205731 to 97e42ed Compare October 30, 2023 21:12
@keithc-ca
Copy link
Contributor

This is not determined by our implementation in java (the method name might be misleading at this point) but rather by how the underlying libffi

It's not up to libffi either: it must use the calling convention expected by the function being invoked. If I use gcc to compile a function receiving or returning U64 (from my example above), to call it, the arguments need to be where they are expected to be and there needs to be agreement in how the return value is handled.

@ChengJin01
Copy link
Author

ChengJin01 commented Oct 30, 2023

It's not up to libffi either: it must use the calling convention expected by the function being invoked. If I use gcc to compile a function receiving or returning U64 (from my example above), to call it, the arguments need to be where they are expected to be and there needs to be agreement in how the return value is handled.

With the test case as follows:

downcall.h
typedef union union_64Byte_Double_Long {
	char elem1[64];
	double elem2;
	LONG elem3;
} union_64Byte_Double_Long;

downcall.c
union_64Byte_Double_Long 
verifyUnion_64Byte_Double_Long(union_64Byte_Double_Long arg)
{
	return arg;
}

plus the java code in UnionTests
test.zip

   public static void main(String[] args) throws Throwable {

		test_verifyUnion_64Byte_Double_Long_1();
		test_verifyUnion_64Byte_Double_Long_2();
		test_verifyUnion_64Byte_Double_Long_3();
	}
	public static void test_verifyUnion_64Byte_Double_Long_1() throws Throwable {
		SequenceLayout byteArray = MemoryLayout.sequenceLayout(64, JAVA_BYTE);
		UnionLayout unionLayout = MemoryLayout.unionLayout(byteArray.withName("elem1"),
				JAVA_DOUBLE.withName("elem2"), JAVA_LONG.withName("elem3"));
		FunctionDescriptor fd = FunctionDescriptor.of(unionLayout, unionLayout);
		MemorySegment functionSymbol = nativeLibLookup.find("verifyUnion_64Byte_Double_Long").get();
		MethodHandle mh = linker.downcallHandle(functionSymbol, fd);

		try (Arena arena = Arena.ofConfined()) {
			MemorySegment unionSegmt = arena.allocate(unionLayout);
			unionSegmt.set(JAVA_BYTE, 0L, (byte)1); <-------- union_64Byte_Double_Long .elem1[0]
			unionSegmt.set(JAVA_BYTE, 63L, (byte)2); <-------- union_64Byte_Double_Long .elem1[63]

			MemorySegment result = (MemorySegment)mh.invokeExact((SegmentAllocator)arena, unionSegmt);
			System.out.println("elem1[0] = " + unionSegmt.get(JAVA_BYTE, 0));
			System.out.println("elem1[63] = " + unionSegmt.get(JAVA_BYTE, 63));
		}
	}

	public static void test_verifyUnion_64Byte_Double_Long_2() throws Throwable {
		SequenceLayout byteArray = MemoryLayout.sequenceLayout(64, JAVA_BYTE);
		UnionLayout unionLayout = MemoryLayout.unionLayout(byteArray.withName("elem1"),
				JAVA_DOUBLE.withName("elem2"), JAVA_LONG.withName("elem3"));
		FunctionDescriptor fd = FunctionDescriptor.of(unionLayout, unionLayout);
		VarHandle doubleHandle = unionLayout.varHandle(PathElement.groupElement("elem2"));

		MemorySegment functionSymbol = nativeLibLookup.find("verifyUnion_64Byte_Double_Long").get();
		MethodHandle mh = linker.downcallHandle(functionSymbol, fd);

		try (Arena arena = Arena.ofConfined()) {
			MemorySegment unionSegmt = arena.allocate(unionLayout);
			doubleHandle.set(unionSegmt, 2228.111D); <-------- union_64Byte_Double_Long .elem2

			MemorySegment result = (MemorySegment)mh.invokeExact((SegmentAllocator)arena, unionSegmt);
			System.out.println("elem2 = " + (double)doubleHandle.get(result));
		}
	}

	public static void test_verifyUnion_64Byte_Double_Long_3() throws Throwable {
		SequenceLayout byteArray = MemoryLayout.sequenceLayout(64, JAVA_BYTE);
		UnionLayout unionLayout = MemoryLayout.unionLayout(byteArray.withName("elem1"),
				JAVA_DOUBLE.withName("elem2"), JAVA_LONG.withName("elem3"));
		FunctionDescriptor fd = FunctionDescriptor.of(unionLayout, unionLayout);
		VarHandle longHandle = unionLayout.varHandle(PathElement.groupElement("elem3"));

		MemorySegment functionSymbol = nativeLibLookup.find("verifyUnion_64Byte_Double_Long").get();
		MethodHandle mh = linker.downcallHandle(functionSymbol, fd);

		try (Arena arena = Arena.ofConfined()) {
			MemorySegment unionSegmt = arena.allocate(unionLayout);
			longHandle.set(unionSegmt, 9876543210L); <-------- union_64Byte_Double_Long .elem3

			MemorySegment result = (MemorySegment)mh.invokeExact((SegmentAllocator)arena, unionSegmt);
			System.out.println("elem3 = " + (long)longHandle.get(result));
		}
	}

Here's the result for the union mentioned above with our implementation:

gcc -c -fpic -g downcall.c
gcc -shared -o libclinkerffitests.so downcall.o

../jdk21_openj9_ffi_cmk_x86_64/bin/javac  --enable-preview --source 21 UnionTests.java
../jdk21_openj9_ffi_cmk_x86_64/bin/java  -Djava.library.path=.../test  --enable-preview
--enable-native-access=ALL-UNNAMED -Dforeign.restricted=permit UnionTests

Note: UnionTests.java uses preview features of Java SE 21.
Note: Recompile with -Xlint:preview for details.
elem1[0] = 1
elem1[63] = 2
elem2 = 2228.111
elem3 = 9876543210

plus the debug results for each element of the union as follows:

(1) test_verifyUnion_64Byte_Double_Long_1:
Thread 2 "main" hit Breakpoint 1, verifyUnion_64Byte_Double_Long (arg=...) at downcall.c:4532
4532            return arg;
(gdb) p arg
$1 = {
  elem1 = "\001", '\000' <repeats 62 times>, "\002", <-------------- elem1[0] = 0x1, elem1[63] = 0x2
  elem2 = 4.9406564584124654e-324,
  elem3 = 1
}

(2) test_verifyUnion_64Byte_Double_Long_2:
Thread 2 "main" hit Breakpoint 1, verifyUnion_64Byte_Double_Long (arg=...) at downcall.c:4532
4532            return arg;
(gdb) p arg
$1 = {
  elem1 = "\266\363\375\324\070h\241@", '\000' <repeats 55 times>,
  elem2 = 2228.1109999999999, <---------------- elem2
  elem3 = 4657118082978673590
}

(3) test_verifyUnion_64Byte_Double_Long_3:
Thread 2 "main" hit Breakpoint 1, verifyUnion_64Byte_Double_Long (arg=...) at downcall.c:4532
4532            return arg;
(gdb) p arg
$1 = {
  elem1 = "\352\026\260L\002", '\000' <repeats 58 times>,
  elem2 = 4.8796606997276283e-314,
  elem3 = 9876543210  <------------------- elem3
}

Which means there is no problem in dealing with this union in our code. As I mentioned previously, how to handle union on registers/stack in native is irrelevant to our code but depends on the underlying libffi/compiler. That being said, our job is to simply lay out the expected primitive arrays in java to be correctly handled by libffi/compiler as to how to map to the registers/stack.

@ChengJin01 ChengJin01 force-pushed the ffi_enable_union_support_jdk21 branch from 97e42ed to 5a39519 Compare October 31, 2023 15:28
@keithc-ca
Copy link
Contributor

the method name might be misleading at this point

Perhaps you can find a better, more descriptive name and update the javadoc to suit?

The only effect of the answer from validatePrimTypesOfUnionForGPR() seems to be whether we describe the union to FFI as a struct with one or more elements of type int/long or float/double. Are you saying that the presence of any member that can be passed in a GPR is an indicator that the calling convention will use GPRs even if there are members which could not be passed in a GPR? If so, that surprises me. I would expect that the presence of float or double would take precedence meaning we should use float or double in the description even if int or long members are present.

In the end, I expect passing or returning struct or union types to be exceedingly rare in real world FFM scenarios, so for the most part, the immediate need is to pass the associated tests.

@ChengJin01 ChengJin01 force-pushed the ffi_enable_union_support_jdk21 branch from 5a39519 to 22e78a1 Compare October 31, 2023 18:26
@ChengJin01
Copy link
Author

ChengJin01 commented Oct 31, 2023

Perhaps you can find a better, more descriptive name and update the javadoc to suit?
The only effect of the answer from validatePrimTypesOfUnionForGPR() seems to be whether we describe the union to FFI as a struct with one or more elements of type int/long or float/double.

Yes. I just updated the method names by adding OrStack and the description to reflect how it works with the underlying hardware (by mapping to the expected registers/stack).

Are you saying that the presence of any member that can be passed in a GPR is an indicator that the calling convention will use GPRs even if there are members which could not be passed in a GPR? If so, that surprises me.

Yes (for GPR or stack as it is) and it was verified on all supported platforms in the majority of cases including some platform specific exceptions being addressed in our code.

I would expect that the presence of float or double would take precedence meaning we should use float or double in the description even if int or long members are present.

This was exactly my initial assumption which proved to be entirely wrong in tests. On the contrary, the truth is: it always has to be GPR/stack for union with mixed types (including the floating and non-float primitives, nested structs/unions, etc) even though float/double is present in the union (a float or double array doesn't work in such cases). That's why we have to clarify this in code/description with the best solution to handle all cases.

In the end, I expect passing or returning struct or union types to be exceedingly rare in real world FFM scenarios, so for the most part, the immediate need is to pass the associated tests.

Yes, that's what we've already done in the newly added test suites for union which are sufficient to cover most scenarios with different combinations to ensure the code is verified from different perspectives (normal, nested, mixed types, etc)

The changes aim to implement the union type in the FFI specific
code in JD21 by mapping the simplified the union type (simulated
via the struct) to the underlying hardware.

Signed-off-by: ChengJin01 <[email protected]>
@ChengJin01 ChengJin01 force-pushed the ffi_enable_union_support_jdk21 branch from 22e78a1 to 512606f Compare October 31, 2023 19:58
@keithc-ca
Copy link
Contributor

Were you intending to add back the union test cases for Java22andUp in this pull request, or defer those to later?

@ChengJin01
Copy link
Author

ChengJin01 commented Oct 31, 2023

Were you intending to add back the union test cases for Java22andUp in this pull request, or defer those to later?

We intend to add them to Java22andUp later in a separate PR after this PR gets merged as these test cases (in java) need to be updated accordingly with the new APIs introduced in JDK22 as mentioned at #18296.

@keithc-ca
Copy link
Contributor

Jenkins test sanity aix,osx,win,zlinux jdk17,jdk21

@ChengJin01
Copy link
Author

ChengJin01 commented Nov 1, 2023

@keithc-ca keithc-ca merged commit 0ee3f98 into eclipse-openj9:master Nov 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:vm jdk21 project:panama Used to track Project Panama related work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants