[foreign-abi] On invokers

[Jorn Vernee]

Hi Nick,

Thanks for the explanation.

The changes look good!

W.r.t pushing the changes. There are some dependencies on the 
foreign-abi branch, and since pushing would temporarily remove support 
on some platforms (I believe we need separate patches to get the 
attribution right), I'd like to ask around before going ahead with that. 
So, I'll come back on that.

Thanks,
Jorn

On 28/10/2019 10:45, Nick Gasson wrote:
> Hi Jorn,
>
> Updated webrev here:
>
> http://cr.openjdk.java.net/~ngasson/panama/new-invoker/webrev.03/
>
> I tested this with Maurizio's fix to CallGeneratorHelper.
>
>>
>> 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?
>
> The architecture requires that SP be 16-byte aligned when it's used as 
> the base for an address calculation. So in theory it should be fine to 
> subtract Rstack_size and then AND with -16. But on AArch64 the 
> encoding for the SP register is a bit special: register 31 can refer 
> to either SP or the zero register (ZR) depending on the particular 
> instruction. For the AND-immediate instruction, R31 as a source 
> operand always denotes ZR so it's impossible to encode `__ andr(sp, 
> sp, -16);`. We have to do the calculation in another register and then 
> move it back into SP.
>
>> - 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).
>
> Removed the shadow space stuff as it not testable and added an assert 
> that it's zero. Also replaced the hard-coded constant as suggested.
>
>>
>> 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.
>>
>
> Done.
>
> Thanks,
> Nick
>