[foreign-abi] On invokers

[Maurizio Cimadamore]

Hi,
thanks for experimenting with the invoker.

For the code sharing issues, I'd say for now the priority is to move to 
the new invoker - cutting and pasting code is ok for now. We can 
revisit/share code once it's all in once place.

I'll leave the question on binding interpreter to Jorn - that is going 
to be relevant on SysV too.

Maurizio

On 23/10/2019 10:33, Nick Gasson wrote:
> 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