RFR: 8295044: Implementation of Foreign Function and Memory API (Second Preview) [v2]

[Jorn Vernee]

On Fri, 4 Nov 2022 18:23:17 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> This PR contains the API and implementation changes for JEP-434 [1]. A more detailed description of such changes, to avoid repetitions during the review process, is included as a separate comment.
>> 
>> [1] - https://openjdk.org/jeps/434
>
> Maurizio Cimadamore has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 17 additional commits since the last revision:
> 
>  - Merge branch 'master' into PR_20
>  - Merge branch 'master' into PR_20
>  - Merge pull request #14 from minborg/small-javadoc
>    
>    Update some javadocs
>  - Update some javadocs
>  - Revert some javadoc changes
>  - Merge branch 'master' into PR_20
>  - Fix benchmark and test failure
>  - Merge pull request #13 from minborg/revert-factories
>    
>    Revert MemorySegment factories
>  - Update javadocs after comments
>  - Revert MemorySegment factories
>  - ... and 7 more: https://git.openjdk.org/jdk/compare/e1e4e45b...3d933028

Some preliminary comments about some changes I think are missing from this PR (noticed while I was making a patch for the VM changes)

I will do a more thorough review after the changes from https://github.com/openjdk/panama-foreign/pull/750 are included as well.

src/java.base/share/classes/jdk/internal/foreign/AbstractMemorySegmentImpl.java line 474:

> 472:         long bbAddress = NIO_ACCESS.getBufferAddress(bb);
> 473:         Object base = NIO_ACCESS.getBufferBase(bb);
> 474:         UnmapperProxy unmapper = NIO_ACCESS.unmapper(bb);

Looks like here is also missing the fix that rejects StringCharBuffer: https://github.com/openjdk/panama-foreign/pull/741 I think that is good to include as well.

src/java.base/share/classes/jdk/internal/foreign/abi/BindingSpecializer.java line 477:

> 475:                 case UNBOX_ADDRESS -> emitUnboxAddress();
> 476:                 case DUP -> emitDupBinding();
> 477:                 case CAST -> emitCast((Binding.Cast) binding);

This contains the CAST binding, but not the accompanying VM changes from: https://github.com/openjdk/panama-foreign/pull/720 which removes the now dead code. Preferably both changes go together (and the code removal is pretty trivial, so I suggest including it here)

src/java.base/share/classes/jdk/internal/foreign/abi/BindingSpecializer.java line 491:

> 489:         emitLoad(highLevelType, paramIndex2ParamSlot[paramIndex]);
> 490: 
> 491:         if (shouldAcquire(paramIndex)) {

I can't comment on the actual line below, but this is also missing the fix from: https://github.com/openjdk/panama-foreign/pull/739 (that is a Java-only change as well). I suggest adding that as well.

src/java.base/share/classes/jdk/internal/foreign/abi/x64/windows/CallArranger.java line 165:

> 163:                 assert forArguments : "no stack returns";
> 164:                 // stack
> 165:                 long alignment = Math.max(layout.byteAlignment(), STACK_SLOT_SIZE);

This is also missing part of the changes from: https://github.com/openjdk/panama-foreign/pull/728/ but other changes to the shared code are present. The `layout` parameter is not needed here. (see the changes to this file in the original PR)

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

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