[foreign-abi] On invokers

[Nick Gasson]

Hi Jorn,

I tried getting this to work on Arm using your webrev.03 and the Windows 
ABI implementation as a base. The patch is here:

http://cr.openjdk.java.net/~ngasson/panama/new-invoker/webrev.01/

(I used the Skara webrev tool, seemed to work ok...). This passes all 
the tests in the jdk_foreign group. Just a couple of notes:

* The upcall/downcall stubs on AArch64 are correct if we are calling 
from/to the standard C ABI, because the stubs use only caller-saved 
registers. For other potential ABIs I guess we need to preserve the set 
of registers that are callee-saved in the "from" ABI but caller-saved in 
the "to" ABI and vice-versa. I'm not sure the x86 version is correct 
wrt. this at the moment either (?).

* Previously I had to make some changes to the shared code to support 
the "indirect result register" (r8) for by-value struct returns. This 
time I could make this work by just treating it as an input register.

* foreign_globals_aarch64.[ch]pp is just cut+paste from the x86 version 
with the x87 registers removed. Maybe we can put this in shared code to 
remove the duplication as the platform specific bits seem minimal? Not 
sure about other architectures in the future...

* Similarly CallArranger.java has some complex code in `if 
(returnsInMemory) { ... }' to shuffle the parameters for by-value struct 
returns. This is identical between AArch64 and Windows x86 so maybe we 
can factor it out? Not sure if it applies to SysV x86 too.

* I had a bit of trouble generating the right instructions for the 
binding interpreter in the following case: if a struct should be passed 
in multiple arguments registers but there are insufficient remaining, 
then the entire struct should be passed as a copy on the stack. I tried 
doing this by allocating a stack VMStorage with the layout size, and 
then generating a single Binding.Dereference to copy the whole struct. 
This worked in BoxBindingCalculator but not UnboxBindingCalculator where 
I have to dereference it eight bytes at a time (otherwise I get an 
exception about reading outside the memory segment). Have a look at 
UnboxBindingCalculator::spillStruct to see what I mean...


Thanks,
Nick


On 14/10/2019 17:53, Jorn Vernee wrote:
> Hi Nick,
> 
> Good to hear that you got it working!
> 
> FTR, I also just generated a diff separately that seems to apply cleanly
> (module whitespace warnings):
> http://cr.openjdk.java.net/~jvernee/prog-back/diff.patch
> 
> (be aware that the code review server seems to be dying a bit currently)
> 
> Cheers,
> Jorn
> 
> On 14/10/2019 11:47, Nick Gasson wrote:
>> Hi Jorn,
>>
>> Thanks for the hint about git apply --recount. That works modulo the
>> two errors below that I fixed by just downloading the raw files.
>>
>> error: patch failed: src/hotspot/cpu/x86/foreign_globals_x86.hpp:20
>> error: src/hotspot/cpu/x86/foreign_globals_x86.hpp: patch does not apply
>> error: patch failed: src/hotspot/cpu/x86/universalUpcallHandler_x86.cpp:194
>> error: src/hotspot/cpu/x86/universalUpcallHandler_x86.cpp: patch does not apply
>> panama.git.patch:4716: new blank line at EOF.
>>
>>
>> Nick
>>
>> On 14/10/2019 16:56, Jorn Vernee wrote:
>>> Hi Nick,
>>>
>>> Thanks for trying the patch. I'm using Git, so I'm using a port of the
>>> original webrev.ksh script. I've previously done a set of fixes to the
>>> port, since we were having off-by-one errors before, but apparently not
>>> all the bugs were fixed :/ (well, at least I now have a test case).
>>>
>>> I'll look into a fix, but that's probably gonna take a while. In the
>>> mean time it might be fastest to manually fix the line count. Git also
>>> has an option to --recount the number of lines when applying a patch,
>>> but I couldn't find anything similar for HG unfortunately.
>>>
>>> Sorry,
>>> Jorn
>>>
>>> On 14/10/2019 08:19, Nick Gasson wrote:
>>>> Hi Jorn,
>>>>
>>>>> Here is an updated version that gets the build working on GCC:
>>>>> cr.openjdk.java.net/~jvernee/prog-back/webrev.03/
>>>> I'm having trouble applying the patch file from this webrev (and also
>>>> the one from webrev.02).
>>>>
>>>> http://cr.openjdk.java.net/~jvernee/prog-back/webrev.03/panama.git.patch
>>>>
>>>> patching file src/hotspot/cpu/aarch64/directUpcallHandler_aarch64.cpp
>>>> patching file src/hotspot/cpu/x86/directUpcallHandler_x86.cpp
>>>> patching file src/hotspot/cpu/x86/foreign_globals_x86.cpp
>>>> patching file src/hotspot/cpu/x86/foreign_globals_x86.hpp
>>>> patch: **** malformed patch at line 697: @@ -66,6 +42,38 @@
>>>>
>>>> Looks like the added/removed line counts are off-by-one?
>>>>
>>>> Thanks,
>>>> Nick