Skip to content

Update the FFI specific code in JDK21 for JEP442 - #17597

Merged
keithc-ca merged 1 commit into
eclipse-openj9:masterfrom
ChengJin01:ffi_update_code_for_jep442_jdk21_v5
Jul 6, 2023
Merged

Update the FFI specific code in JDK21 for JEP442#17597
keithc-ca merged 1 commit into
eclipse-openj9:masterfrom
ChengJin01:ffi_update_code_for_jep442_jdk21_v5

Conversation

@ChengJin01

Copy link
Copy Markdown

The modifications are based on the latest APIs in JDK21
to support JEP442 as mentioned at #16951.

Signed-off-by: ChengJin01 jincheng@ca.ibm.com

@ChengJin01

ChengJin01 commented Jun 15, 2023

Copy link
Copy Markdown
Author

Note:
[1] The code changes have been verified with the updated test suites in JDK21 on JDK21/JDKnext.
[2] The changes include all code/tests related to the Arena/AddressLayout(introduced in JDK21 to replace OfAddress for pointer) as required at #16951.
[3] There is no updated code in OpenJDK as everything was addressed via ibmruntimes/openj9-openjdk-jdk#598 previously.
[4] There is no code required for the fallback native linker that is literally a libffi-based solution for new architectures without the FFI support which should be enabled by default.
[5] VaList related test cases are removed as there is no support from OpenJDK since JDK21.
[6] Other features (e.g. isTrivial() the union type, etc) will be handled separately via the Jtreg test suites.

@ChengJin01 ChengJin01 added comp:vm project:panama Used to track Project Panama related work jdk21 labels Jun 15, 2023
@ChengJin01
ChengJin01 force-pushed the ffi_update_code_for_jep442_jdk21_v5 branch from 2155993 to 784e7f5 Compare June 15, 2023 00:44
@ChengJin01

Copy link
Copy Markdown
Author

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

@ChengJin01
ChengJin01 requested a review from tajila June 15, 2023 00:45
@ChengJin01

Copy link
Copy Markdown
Author

I will need to double-check the code here to see whether it still works with the new changes in JDK21 at ibmruntimes/openj9-openjdk-jdk21#4 (just merged).

@ChengJin01 ChengJin01 changed the title Update the FFI specific code in JDK21 for JEP442 [WIP] Update the FFI specific code in JDK21 for JEP442 Jun 19, 2023
@ChengJin01

Copy link
Copy Markdown
Author

Probably need to wait till the changes at ibmruntimes/openj9-openjdk-jdk21#4 is merged to the openj9 branch.

@ChengJin01
ChengJin01 force-pushed the ffi_update_code_for_jep442_jdk21_v5 branch from 784e7f5 to 1dbcb97 Compare June 19, 2023 20:18
@ChengJin01

ChengJin01 commented Jun 19, 2023

Copy link
Copy Markdown
Author

As talked to Jason offline, the follow changes in OpenJDK should be merged altogether at first to get code related compiled correctly in JDK21/next:
[1] ibmruntimes/openj9-openjdk-jdk21#5 (depends on #17514 & #17125)
[2] ibmruntimes/openj9-openjdk-jdk#608

@ChengJin01
ChengJin01 force-pushed the ffi_update_code_for_jep442_jdk21_v5 branch from 1dbcb97 to a9d78db Compare June 21, 2023 03:28
@ChengJin01

ChengJin01 commented Jun 21, 2023

Copy link
Copy Markdown
Author

There are a bunch of test failures with padding/alignment detected in OpenJ9 FFI test suites with the newly merged changes (with check on the padding/alignment) in JDK21. I will need to check case by case to see whether it is simply a test issue or anything related to our code that needs to be addressed.

@ChengJin01
ChengJin01 force-pushed the ffi_update_code_for_jep442_jdk21_v5 branch 8 times, most recently from 8c21068 to ecdc2c5 Compare June 26, 2023 15:44
@ChengJin01

Copy link
Copy Markdown
Author

I've went through all failing test cases which was caused by https://github.com/ibmruntimes/openj9-openjdk-jdk21/blob/0a869c9c5496e25f7f89805471071640a4457d68/src/java.base/share/classes/jdk/internal/foreign/abi/AbstractLinker.java#L222C1-L229C6

    // check for trailing padding
    private static void checkGroupSize(GroupLayout gl, long maxUnpaddedOffset) {
        long expectedSize = Utils.alignUp(maxUnpaddedOffset, gl.byteAlignment());
        if (gl.byteSize() != expectedSize) {
            throw new IllegalArgumentException("Layout '" + gl + "' has unexpected size: "
                    + gl.byteSize() + " != " + expectedSize); <------
        }
    }

which the alignment/padding related code has been adjusted in OpenJDK in which case the trailing padding is no longer required in struct except a few cases on AIX (e.g. [double, int]/[double, float]). So there is no more code specific changes in OpenJ9 as padding is ignored by default in downcall.

@ChengJin01

ChengJin01 commented Jun 26, 2023

Copy link
Copy Markdown
Author

The PR has been verified on all supported platforms (no code required in OpenJDK) with the updated test cases and is ready for review.

@ChengJin01
ChengJin01 force-pushed the ffi_update_code_for_jep442_jdk21_v5 branch from ecdc2c5 to 43d2e73 Compare June 27, 2023 16:50
@ChengJin01

Copy link
Copy Markdown
Author

@tajila, any more comment for this PR?

@tajila

tajila commented Jun 29, 2023

Copy link
Copy Markdown
Contributor

jenkins sanity test xlinux,plinux jdk21

@tajila

tajila commented Jun 29, 2023

Copy link
Copy Markdown
Contributor

jenkins test sanity xlinux,plinux jdk21

@ChengJin01

Copy link
Copy Markdown
Author

@ChengJin01

Copy link
Copy Markdown
Author

@tajila, please help run Jenkins for this PR to see whether there is no other issue as #17510 is merged.

The modifications are based on the latest APIs in JDK21
to support JEP442 as mentioned at eclipse-openj9#16951.

Signed-off-by: ChengJin01 <jincheng@ca.ibm.com>
@ChengJin01
ChengJin01 force-pushed the ffi_update_code_for_jep442_jdk21_v5 branch from f1cdebf to 4134c7f Compare July 5, 2023 18:31
@tajila

tajila commented Jul 5, 2023

Copy link
Copy Markdown
Contributor

jenkins test sanity xlinux,plinux jdk21

@keithc-ca
keithc-ca merged commit 5567b8c into eclipse-openj9:master Jul 6, 2023
@ChengJin01
ChengJin01 deleted the ffi_update_code_for_jep442_jdk21_v5 branch October 30, 2023 19:02
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