RFR: 8296477: Foreign linker implementation update following JEP 434 [v2]

[Maurizio Cimadamore]

On Wed, 9 Nov 2022 18:16:59 GMT, Jorn Vernee <jvernee at openjdk.org> wrote:

>> Pull in linker implementation changes, that include non-trivial changes to VM code, from the panama-foreign repo into the main JDK.
>> 
>> This is split off from the main JEP integration to make reviewing easier.
>> 
>> This includes the following patches:
>> 
>> 1. https://github.com/openjdk/panama-foreign/pull/698
>> 2. https://github.com/openjdk/panama-foreign/pull/699
>> 3. (part of) https://github.com/openjdk/panama-foreign/pull/731
>> 4. https://github.com/openjdk/panama-foreign/pull/740
>> 5. https://github.com/openjdk/panama-foreign/pull/746
>> 6. https://github.com/openjdk/panama-foreign/pull/742
>> 7. https://github.com/openjdk/panama-foreign/pull/743
>> 
>> Probably the biggest change to the code comes from replacing `VMReg` - which can not represent offsets into the stack that are not a multiple of the VM's stack slot size (32-bits) - with the new `VMStorage` class, which can describe byte offsets into the stack, as well as having a register mask to indicate only certain register segments.
>> 
>> The only part of 3. that is in this PR is the part that turns the `VMStorage` class in Java into a record.
>> 
>> Please refer to the PR of each individual patch for a more detailed description.
>
> Jorn Vernee has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - Work around x86 failures
>  - Review comments

Java changes look good - added few nits

src/java.base/share/classes/java/lang/foreign/Linker.java line 319:

> 317:          * {@return A linker option used to save portions of the execution state immediately after
> 318:          *          calling a foreign function associated with a downcall method handle,
> 319:          *          before it can be overwritten by the runtime, or read through conventional means}

Suggestion:

         *          before it can be overwritten by the Java runtime, or read through conventional means}

src/java.base/share/classes/java/lang/foreign/Linker.java line 340:

> 338:          * before it can be overwritten by the runtime, or read through conventional means.
> 339:          * <p>
> 340:          * State is captured by a downcall method handle on invocation, by writing it

Suggestion:

         * Execution state is captured by a downcall method handle on invocation, by writing it

src/java.base/share/classes/jdk/internal/foreign/abi/CallingSequence.java line 188:

> 186:     }
> 187: 
> 188:     public int capturedStateMask() {

Isn't this a final static during execution?

src/java.base/share/classes/jdk/internal/foreign/abi/NativeEntryPoint.java line 78:

> 76:     private static void checkType(MethodType methodType, boolean needsReturnBuffer, int savedValueMask) {
> 77:         if (methodType.parameterType(0) != long.class) {
> 78:             throw new IllegalArgumentException("Address expected as first param: " + methodType);

Is throwing IAE correct here? E.g. can the user do anything about it, or does the exception describe more of an internal error? (In that case AssertionError might be better?)

src/java.base/share/classes/jdk/internal/foreign/abi/NativeEntryPoint.java line 83:

> 81:         if ((needsReturnBuffer && methodType.parameterType(checkIdx++) != long.class)
> 82:             || (savedValueMask != 0 && methodType.parameterType(checkIdx) != long.class)) {
> 83:             throw new IllegalArgumentException("return buffer and/or preserved value address expected: " + methodType);

Same here

src/java.base/share/classes/jdk/internal/foreign/abi/aarch64/CallArranger.java line 3:

> 1: /*
> 2:  * Copyright (c) 2020, 2022, Oracle and/or its affiliates. All rights reserved.
> 3:  * Copyright (c) 2019, 2022, Arm Limited. All rights reserved.

Not sure if all copyrights of all changed classes have been tweaked? This might be a more general problem with FFM API.

test/jdk/ProblemList.txt line 484:

> 482: # jdk_foreign
> 483: 
> 484: java/foreign/callarranger/TestAarch64CallArranger.java generic-x86

Should we exclude these tests on 32 bits in the jtreg header (as I think we do for other tests) ?

-------------

PR: https://git.openjdk.org/jdk/pull/11019