[foreign] RFR 8210223:Merge marshalling code paths in NativeInvoker and UpcallHandler

[Sundararajan Athijegannathan]

Hi,

I built/tested the patch on Mac - all 41 tests including the mac 
specific "python test" pass.

-Sundar

On 30/08/18, 8:03 PM, Maurizio Cimadamore wrote:
> Hi,
> as I'm working on a couple of biggie patches which touch the way in 
> which native invocation is performed, I thought it was better to start 
> with a cleanup which brings UpcallHandler and NativeInvoker much 
> closer together. The current code has a big issue: the native 
> invocation logic and the upcall logic use different pieces of code to 
> handle marshalling of Java values to and from register/stack.
>
> While it's true that, in principle, we need to specify the 
> marshalling/unmarshalling logic in a single place and then reuse the 
> same code from both places, doing so proved to be quite challenging:
>
> * first, there's a natural arity mismatch - for instance, 
> NativeInvoker needs to convert N Java values into M register/stack 
> words; but UpcallHandler needs to do so only to convert back the Java 
> return value. The reverse is true when we consider the opposite 
> translation - that is, UpcallHandler needs to read N Java values from 
> M registers, while NativeInvoker only needs to do that for the return 
> value.
>
> * then, there's the issue of return values passed in memory; this 
> introduce an asymmetry in the way we perform the 
> marshalling/unmarshalling logic, as for return in memory value, we 
> just need to copy/read the entire contents of the arg (typically a 
> struct) to/from a memory region pointed to by a pointer (this pointer 
> is stored in the first integer register)
>
> * finally, the fact that callbacks lead to stub generation at 
> invocation time, makes the logic even more convoluted (I will get rid 
> of this asymmetry in an upcoming patch)
>
> The idea is to use the following two functions
>
> void unboxValue(Object o, LayoutType<?> type, 
> java.util.function.Function<ArgumentBinding, Pointer<?>> dstPtrFunc, 
> List<ArgumentBinding> bindings)
>
> boxValue(LayoutType<?> type, 
> java.util.function.Function<ArgumentBinding, Pointer<?>> srcPtrFunc, 
> List<ArgumentBinding> bindings)
>
> Where the first converts a Java value into a bunch of register/stack 
> words, while the latter does the opposite, and turns a bunch of 
> register/stack words back into a Java value (in reality the signatures 
> are more complex than that, because of the callback issue mentioned 
> above).
>
> To specify the list of register/stack words to write/read to/from, we 
> use a list of ArgumentBinding instances - which are generated by the 
> CallingSequence. Doing the conversion one Java value at a time is a 
> lot more natural and simper to manage, and get rid of issues with the 
> previous code where we had to buffer intermediate results across 
> multiple reads.
>
> The function from binding to pointer allows the logic to remain 
> general: the code doesn't know how to read/write into the 
> register/stack words, so it just assumes there's some function mapping 
> a given binding back to a good old pointer (which can then be used to 
> read/write). This gives us the generality we need, and allows to share 
> the code between UpcallHandler and NativeInvoker.
>
> Finally, since this logic deals, as mentioned before, with one 
> argument at a time, we need a way to collect all ArgumentBinding 
> instances for a given argument. For this reason, I had to do some 
> surgery into CallingSequence (see new classifyBindings method), in 
> order to pre-compute the set of argument bindings associated to each 
> argument/return. This allow us to get the set of bindings for argument 
> at index I, which are then typically passed to the 'boxValue' routine. 
> There's also some offset pre-computation, so that, given a binding, we 
> know exactly where, in the long[] used by the NativeInvoker call 
> scheme, we have to store the resulting word.
>
>
> I've added a smoke test for structs passed/return by value, which 
> covers both the in-register and in-memory schemes. This use case was 
> not covered in our unit tests (but was covered by the clang-ffi test), 
> so I thought it would be better to have an explicit test for that.
>
> Please, test against anything you've got - the changes in here are 
> quite subtle - the more the merrier :-)
>
> Webrev:
>
> http://cr.openjdk.java.net/~mcimadamore/panama/8210223/
>
> Cheers
> Maurizio
>
>
>