[foreign-abi] RFR 8227718: Add support for SystemABI

[Maurizio Cimadamore]

Here's a revised webrev:

http://cr.openjdk.java.net/~mcimadamore/panama/8227718_v3/

Maurizio

On 18/07/2019 10:07, Maurizio Cimadamore wrote:
>
> On 18/07/2019 08:20, Nick Gasson wrote:
>> Hi Maurizio,
>>
>> I tried this on AArch64, looks good. Couple of minor things:
>>
>> * Found one conflict in JavaLangAccess.java when I applied the patch on
>>    top of the foreign-abi branch. Looks like you need to rebase after
>>    "8227587: Add internal privileged System.loadLibrary" which also 
>> added
>>    a method at the end of this class.
> I noticed the merge issue even on the main foreign branch - I was 
> going to merge that branch later today and then refresh this one.
>>
>> * I got an error compiling CallGeneratorHelper.java:
>>
>> /home/nicgas01/panama/test/jdk/java/foreign/CallGeneratorHelper.java:389: 
>>
>> error: cannot find symbol
>>                  value = ForeignUnsafe.getOffset((MemoryAddress)value);
>>                                       ^
>>    symbol:   method getOffset(MemoryAddress)
>>    location: class ForeignUnsafe
>>
>>    I think this should be ForeignUnsafe.getUnsafeOffset?
> Yep - that's another change that didn't got pulled in fully, sorry
>>
>> * There were two test failures caused by the AArch64ABI boxValue()
>>    method writing off the end of the destination buffer. When a 
>> struct is
>>    passed in a series of registers boxValue would always copy some
>>    multiple of the storage size (64 bits) but this is wrong if the 
>> struct
>>    is e.g. 12 bytes. The change below fixes that:
>>
>> --- 
>> a/src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/aarch64/AArch64ABI.java
>> +++ 
>> b/src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/aarch64/AArch64ABI.java
>> @@ -220,8 +220,10 @@ public class AArch64ABI implements SystemABI {
>>                        for (ArgumentBinding binding : bindings) {
>>                           MemoryAddress dst = 
>> rtmp.offset(binding.offset());
>> +                        long remainBytes = layout.byteSize() - 
>> binding.offset();
>> MemoryAddress.copy(srcPtrFunc.apply(binding), dst,
>> - binding.storage().getSize());
>> + Math.min(binding.storage().getSize(),
>> + remainBytes));
>>                       }
>>                   } else {
>>                       // Struct is passed by pointer
>
> Thanks - that's exactly what I was looking for - when rewriting the 
> SysV code and the Win64 code I introduced certain mistakes which I 
> then fixed (and then somehow applied fixes also to Aarch) - but I was 
> unsure whether the whole thing worked :-)
>
> I will merge, integrate your changes and share another webrev.
>
> Maurizio
>
>
>>
>>
>> With these fixed the jdk_foreign jtreg group passes.
>>
>> Thanks,
>> Nick
>>
>>
>>> Hi,
>>> this patch adds initial support for SystemABI low-level interface in a
>>> new branch, namely [foreign-abi]. This is the second step in the Panama
>>> ladder, as per the plan outlined in [1].
>>>
>>> http://cr.openjdk.java.net/~mcimadamore/panama/8227718_v2/
>>>
>>> So, the goal of this patch is essentially to backport the SystemABI
>>> interface available in the foreign branch [2] and make it available 'on
>>> top' of the foreign memory access API. As such, as part of this work I
>>> had to remove references to various abstractions that are defined in 
>>> the
>>> high level C interop API, such as Scope, Pointer and LayoutType and
>>> replace them with what's available at a lower level.
>>>
>>> The result is good and I was even able to port most of the existing
>>> stress test (stdlib test, combinatorial upcall/downcall tests) on 
>>> top of
>>> the lower level ABI implementation.
>>>
>>> In the backend we will still find two invokers - direct invoker, 
>>> used in
>>> x64/Arch64 and the universal invoker, used as a fallback on all systems
>>> (and the only backend available for WIn64). The long term plan, as
>>> already mentioned, is to replace these backends with the more portable
>>> one being developed in the 'linkToNative' branch.
>>>
>>> While most of the port was smooth, I encountered some issues:
>>>
>>> * There's no way to test if a layout is a padding layout - to do this I
>>> added a quick and dirty helper method on the Util class which uses a
>>> reflection-based test. This is obviously not ideal. I think a better,
>>> long term move would be to move the implementation of _all_ layout
>>> classes outside of the public package jdk.incubating.foreign, so 
>>> that we
>>> can add more helper methods that are not exposed outside (this is what
>>> we do for MemorySegment and MemoryAddress, whose impl is defined 
>>> elsewhere).
>>>
>>> * There's no support for varargs call - while varargs method can be
>>> called, by providing a fully specialized signature (methodtype/function
>>> descriptor), calling a vararg method using a Java vararg is not
>>> supported. As pointed out in [3], using Java varargs to mimic C varargs
>>> is lossy - while it sort of works ok for downcalls (albeit with poor
>>> performances), completely breaks apart when it comes to upcalls. So,
>>> some other solution is required there; or, we could say that the only
>>> way to support varargs is to always instantiate the signature (albeit
>>> there are also problems there, especially for Windows ABI, which 
>>> require
>>> vararg arguments to be passed differently, so they will have to be
>>> marked somehow, as discussed in [4]).
>>>
>>> * There's also no support for exotic ABI types such as x87 or complex
>>> types. While that's good for now, we obviously need some kind of robust
>>> story to get there. Again this topic is covered in more details here 
>>> [4].
>>>
>>> Some bonus points of this approach:
>>>
>>> * using memory segments to model structs is very handy - if a function
>>> returns a struct by value, and you need to work with it and then 
>>> discard
>>> it, you can do:
>>>
>>> try (MemorySegment struct = (MemorySegment)func.invokeExact(...)) {
>>>    //some logic here
>>> }
>>>
>>> And the struct will be freed after the TWR block is closed, as with any
>>> other segment.
>>>
>>> * deallocation of callbacks is also more deterministic - I've tweaked
>>> the upcall logic to create a 'callback segment' - which, when closed,
>>> asks the VM to deallocate the corresponding code blob. So, if you 
>>> have a
>>> MemoryAddress which is the entry point of the VM code blob, you can
>>> simply get its segment and then close it; doing so will release any
>>> memory associated with the callback.
>>>
>>> Comments?
>>>
>>> Thanks
>>> Maurizio
>>>
>>> P.S.
>>> The tests pass on both Linux, Windows and Mac, but (as usual) I could
>>> not test Arm. It would be nice to know if things worked there too.
>>>
>>> [1] - http://cr.openjdk.java.net/~mcimadamore/panama/memaccess.html
>>> [2] -
>>> hg.openjdk.java.net/panama/dev/file/tip/src/java.base/share/classes/jdk/internal/foreign/abi/SystemABI.java 
>>>
>>> [3] - http://cr.openjdk.java.net/~mcimadamore/panama/varargs.html
>>> [4] -
>>> https://mail.openjdk.java.net/pipermail/panama-dev/2019-July/005975.html 
>>>