[foreign-abi] On invokers

[Jorn Vernee]

Hi Nick,

Thanks for fixing that bug. Looking at the tests, it looks like stack 
arguments are not really tested for upcalls on Windows (made a note of 
it [1]).

The patch looks good but I have a few of comments/questions:

universalNativeInvoker_aarch64.cpp
- Why are there 15 bytes added to stack_arg_bytes when allocating space? 
I'd think that `__ sub(sp, sp, Rstack_size); __ andr(sp, sp, -16);` 
would be sufficient? Or is there a requirement to keep `sp` 16-byte 
aligned at _all_ times?
- I see you've also added support for shadow space. But, since it's not 
used by any currently supported AArch64 ABI, I think it's better to 
remove the support for now to reduce clutter. If you want to keep it 
though; shadow space (at least on Windows) should not mess with the 
stack alignment, so there's no need to reallign the stack. Could add an 
assert like `abi._shadow_space_bytes % abi._stack_alignment_bytes == 0` 
to make sure. You will also, in that case, need to account for the 
shadow space when computing stack arguments offset for upcalls `__ 
add(rscratch1, rfp, 16);` -> `__ add(rscratch1, rfp, 16 + 
abi._shadow_space_byte)`. (Looks like I forgot to change the hard-coded 
value in the x86 impl for that as well).

universalNativeInvoker_aarch64.cpp/universalUpcallHandler_aarch64.cpp
- You should use abi._stack_alignment_bytes to align the stack instead 
of the hard-coded '16' literal. This is more important to have, since 
the current supported ABI actually uses stack alignment.

Jorn

[1] : https://bugs.openjdk.java.net/browse/JDK-8233007

On 25/10/2019 09:11, Nick Gasson wrote:
> Hi Jorn,
>
> Thanks for the feedback.
>
>>
>> I think your first solution is causing some undefined behaviour; The
>> argument buffer allocates number-of-stack-storages * STACK_SLOT_SIZE
>> bytes for stack arguments (See the constructor of ProgrammableInvoker).
>> So if you try to pass an argument that is bigger than STACK_SLOT_SIZE
>> with a single binding it's going to read/write out of bounds of the
>> allocated space. I think the fact that it worked in some cases is
>> coincidental.
>>
>> Your current solution looks good to me (cutting up the struct in to
>> STACK_SLOT_SIZEd pieces and generating a binding for each). Your idea of
>> doing a bulk copy is interesting as well, but I'd like to keep breaking
>> up the arguments into chunks, since this seems easier to intrinsify
>> based on the current JIT code for doing runtime calls.
>>
>
> OK, I see that now. I think in the upcall case it worked because the 
> stack memory comes from
>
>    MemoryAddressImpl.ofNative((long) 
> VH_LONG.get(buffer.offset(layout.stack_args)));
>
> And that MemorySegment covers all the memory. I've changed it do the 
> stack-slot-at-a-time copy in both directions. Updated webrev here:
>
> http://cr.openjdk.java.net/~ngasson/panama/new-invoker/webrev.02/
>
> After I did that I ran into a small bug in the pointer function in 
> ProgrammableUpcallHandler: it needs to offset by the the stack slot 
> index multiplied by the type size, like ProgrammableInvoker does 
> (fixed in the above patch).
>
> Thanks,
> Nick