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

Thu Jul 18 09:07:13 UTC 2019

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