[foreign-abi] RFR: 8254260: Consider splitting binding recipe operators that serve a dual role

[Maurizio Cimadamore]

On Thu, 8 Oct 2020 19:12:48 GMT, Jorn Vernee <jvernee at openjdk.org> wrote:

> Hi,
> 
> This patch splits the binding recipe operators that currently serve a dual purpose into 2 operators each:
> - MOVE -> VM_STORE and VM_LOAD
> - DEREFERENCE -> BUFFER_STORE and BUFFER_LOAD
> - CONVERT_ADDRESS -> BOX_ADDRESS and UNBOX_ADDRESS
> 
> Note that I also added a TO_SEGMENT operator which converts a MemoryAddress into a MemorySegment. This was previously
> done as part of the COPY operator, but only for upcalls, so I had to split out this functionality so that COPY could
> have only one interpretation as well. This will also enable some more optimizations down the line, such as merely
> wrapping a by-value struct passed as a pointer in a MemorySegment, instead of having to make a copy.  This also
> regularized the use of allocators by the operators. Now an allocator is always passed as an argument, and that's what
> is used to do allocations, instead of relying implicitly on MemorySegment::allocateNative. (The index of the allocator
> argument is now also always passed when specializing an operator, to indicate the dependency, and so that operator
> impls don't have to 'guess' which argument index is the allocator (see for instance the implementation of
> Copy::specialize).  The rest of the patch is more or less a mechanical renaming, and updating the tests/CallArrangers
> to use the different operator names.  Thanks, Jorn

Looks a great cleanup. Left some comments.
I also ran all tests here and everything looks good.

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/Binding.java line 303:

> 301:
> 302:     // FIXME should register with scope
> 303:     private static MemorySegment toSegment(MemoryAddress operand, long size) {

This seems to be only used inside the `ToSegment` binding, so, perhaps this could should be moved there (also because
this is an interpreter function, not a binding factory - e.g. the overload with the `toSegment` binding factory is
confusing)

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/Binding.java line 363:

> 361:
> 362:     @Override
> 363:     public boolean equals(Object o) {

General question - where do we use equals/hash? Just for testing purposes right?

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/ProgrammableInvoker.java line 344:

> 342:                                 Map<VMStorage, Integer> retIndexMap) throws Throwable {
> 343:         boolean hasBufferCopies = bufferCopySize != 0;
> 344:         NativeScope scope = hasBufferCopies ? NativeScope.boundedScope(bufferCopySize) : null;

If Allocator had a close() method, this could be simplified a little?

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/ProgrammableInvoker.java line 219:

> 217:         MethodHandle specializedHandle = leafHandle; // initial
> 218:
> 219:         int argInsertPos = -1;

The more we tweak this routine, the more I'm convinced we should just spin specialized bytecode :-) - it's becoming
increasingly hard to follow what's happening here. (not for this review - more longer term)

test/jdk/java/foreign/NativeTestHelper.java line 36:

> 34:
> 35:     public static boolean isIntegral(MemoryLayout layout) {
> 36:         return kind(layout).isIntegral();

Whoops - sorry - I guess I fixed the API and have not run the tests?

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/Binding.java line 230:

> 228:
> 229:     enum Tag {
> 230:         VM_STORE,

Should these be called REG_STORE and REG_LOAD? Ah, I guess we also have stack, right? Ok, I agree with the name - these
are "moves" that only the VM can perform, makes sense.

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/Binding.java line 591:

> 589:      * The [type] must be one of byte, short, char, int, long, float, or double
> 590:      */
> 591:     public static class BuffferStore extends Dereference {

Typo, there are 3 `f`

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/Binding.java line 622:

> 620:         @Override
> 621:         public String toString() {
> 622:             return "BuffferStore{" +

Typo here too

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/CallingSequenceBuilder.java line 93:

> 91:
> 92:         for (Binding b : bindings) {
> 93:             if (b.tag() == Binding.Tag.VM_LOAD

We could have static EnumSet for what's an box vs. unbox binding, so that you can filter by checking for presence in
the right enum set?

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/CallingSequenceBuilder.java line 107:

> 105:
> 106:     private static void verifyBoxBindings(Class<?> expectedReturnType, List<Binding> bindings) {
> 107:         Deque<Class<?>> stack = new ArrayDeque<>();

I've seen in the code that the verifyUnbox method speaks about a `returnType` parameter - should it be an `outType` for
symmetry (since, I guess, we use this more than just for returns - e.g. we use it for args in upcalls).

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/ProgrammableUpcallHandler.java line 102:

> 100:                                 : bufferBase.asSlice(layout.argOffset(storage));
> 101:                             return SharedUtils.read(ptr, type);
> 102:                         }, DEFAULT_ALLOCATOR);

For upcalls, couldn't we use a `NativeScope` and close at the end of the call? If the callback leaks segments
outside... well, too bad.

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

Marked as reviewed by mcimadamore (Committer).

PR: https://git.openjdk.java.net/panama-foreign/pull/375