[foreign-abi] RFR 8237358: Split the DEREFERENCE binding operator into a load/store + move

[Nick Gasson]

Hi Jorn,

On 01/28/20 21:35 pm, Jorn Vernee wrote:
> Hi,
>
> Please review the following patch which splits the DEREFERENCE binding
> operator into a load/store + move. This simplifies what the binding
> operator does itself, but surfaces the need for a stack of values in the
> binding interpreter rather than a single object. We also need a DUP
> operator now, for instance in the case where a struct is being passed to
> an upcall, we get ALLOC_BUFFER, DUP, MOVE, DEREFERENCE where the buffer
> is left as last value on the stack to be passed to the target method.
>
> However, we've found that splitting DEREFERENCE makes later
> intrinsification much simpler.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8237358
> Webrev:
> http://cr.openjdk.java.net/~jvernee/panama/webrevs/8237358/webrev.00/
>
> I've tested  this on Windows and Linux (WSL). I also have a binding
> recipe verifier which I ran this through. Will submit that in the next
> patch. (But, I thought it would be best to get this API change out of
> the way before adding more tests, to avoid having to update them).
>

Sorry I missed this patch. There's a small bug in the spillStruct
methods in the AArch64 CallArranger: if the struct size is not a
multiple of 8 then we get the following exception due to reading past
the end of the segment:

test TestUpcall.testUpcalls("f20_S_SSS_FFF", NON_VOID, [STRUCT, STRUCT, STRUCT], [FLOAT, FLOAT, FLOAT]): failure
java.lang.IndexOutOfBoundsException: Out of bound access on segment MemorySegment{ id=0x2330c6bf limit: 12 }; new offset = 8; new length = 8
        at jdk.incubator.foreign/jdk.internal.foreign.MemorySegmentImpl.outOfBoundException(MemorySegmentImpl.java:230)

Easy to fix spillStructUnbox with:

@@ -315,8 +315,9 @@ public class CallArranger {
                 if (offset + STACK_SLOT_SIZE < layout.byteSize()) {
                     bindings.add(new Binding.Dup());
                 }
-                bindings.add(new Binding.Dereference(offset, long.class));
-                bindings.add(new Binding.Move(storage, long.class));
+                Class<?> type = SharedUtils.primitiveCarrierForSize(copy);
+                bindings.add(new Binding.Dereference(offset, type));
+                bindings.add(new Binding.Move(storage, type));
                 offset += STACK_SLOT_SIZE;
             }
         }

And the same in spillStructBox.

I'll send out a separate RFR later.


Thanks,
Nick