RFR: 8263512: [macos_aarch64] issues with calling va_args functions from invoke_native

[Jorn Vernee]

On Thu, 22 Apr 2021 08:19:53 GMT, Nick Gasson <ngasson at openjdk.org> wrote:

> macOS on Apple silicon uses slightly different ABI conventions to the
> standard AArch64 ABI.  The differences are outlined in [1].  In
> particular in the standard (AAPCS) ABI, variadic arguments may be passed
> in either registers or on the stack following the normal calling
> convention.  To handle this, va_list is a struct containing separate
> pointers for arguments located in integer registers, floating point
> registers, and on the stack.  Apple's ABI simplifies this by passing all
> variadic arguments on the stack and the va_list type becomes a simple
> char* pointer.
> 
> This patch adds a new MacOsAArch64 CABI type and MacOsAArch64Linker to
> represent the new ABI variant on macOS.  StackVaList is based on
> WinVaList lightly modified to handle the different TypeClasses on
> AArch64.  The original AArch64Linker is renamed to AapcsLinker and is
> currently used for all non-Mac platforms.  I think we also need to add a
> WinAArch64 CABI but I haven't yet been able to test on a Windows system
> so will do that later.
> 
> The macOS ABI also uses a different method of spilling arguments to the
> stack (the standard ABI pads each argument to a multiple of 8 byte stack
> slots, but the Mac ABI packs arguments according to their natural
> alignment).  None of the existing tests exercise this so I'll open a new
> JBS issue and work on that separately.
> 
> Tested jdk_foreign on macOS AArch64, Linux AArch64, and Linux X86_64.
> 
> [1] https://developer.apple.com/documentation/xcode/writing_arm64_code_for_apple_platforms

I've reviewed, but I don't think this should be integrated right now since it will likely conflict with the JEP implementation, and probably a chunk of StackVaList will have to be re-written to take the ResourceScope changes into account.

As a point of process; I think this should have been a PR against the panama-foreign repo instead, which would have avoided this conflict.

Probably the best course of action right now is to wait until the JEP is integrated, and then update this PR to take those changes (e.g. ResourceScope, virtual calls) into account.

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/aarch64/CallArranger.java line 394:

> 392:             TypeClass argumentClass = TypeClass.classifyLayout(layout);
> 393:             Binding.Builder bindings = Binding.builder();
> 394:             storageCalculator.adjustForVarArgs(layout);

We don't support variadic upcalls (or returns) currently, so maybe here we should rather assert that the layout is _not_ annotated with `STACK_VARARGS_ATTRIBUTE_NAME` ?

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/aarch64/macos/MacOsAArch64Linker.java line 86:

> 84:         handle = SharedUtils.unboxVaLists(type, handle, MH_unboxVaList);
> 85:         return handle;
> 86:     }

Looking at this I realize this version of the code doesn't include the support for virtual calls yet. I think this will break once the JEP is merged (because the overload taking no symbol is not implemented).

It might be nice to check what conflicts there are with the JEP beforehand https://github.com/openjdk/jdk/pull/3699 Though, if it's too much, I think the JEP should go first (and then fix up & merge this PR afterwards), since the JEP is usually much more tricky to get in due to the size. Sorry.

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/aarch64/macos/StackVaList.java line 51:

> 49: 
> 50: /**
> 51:  * Simplified va_list implementation used on macOS and Windows where all

This is not being used on Windows AFAICS? Was that the intention?

(FWIW, as long as they are split, I think the VaList impls should be named after their ABI/platform. So, StackVaList would be named MacOsAArch64VaList instead).

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/aarch64/macos/StackVaList.java line 55:

> 53:  * char* instead of the structure defined in the AAPCS.
> 54:  */
> 55: class StackVaList implements VaList {

VaList impls were one of the things that were affected a lot by the switch to ResourceScope, so this class will also definitely conflict with the JEP.

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/aarch64/macos/StackVaList.java line 131:

> 129:                     MemorySegment struct = allocator.allocate(layout);
> 130:                     struct.copyFrom(segment.asSlice(0L, layout.byteSize()));
> 131:                     segment = segment.asSlice(VA_SLOT_SIZE_BYTES);

Since arguments are packed according to alignment, I guess the offset could be larger or smaller than 8 bytes as well?

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/aarch64/macos/StackVaList.java line 139:

> 137:             VarHandle reader = SharedUtils.vhPrimitiveOrAddress(carrier, layout);
> 138:             res = reader.get(segment);
> 139:             segment = segment.asSlice(VA_SLOT_SIZE_BYTES);

Same here I guess.

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/aarch64/macos/StackVaList.java line 243:

> 241: 
> 242:             // Each argument may occupy up to four slots
> 243:             MemorySegment segment = allocator.allocate(VA_SLOT_SIZE_BYTES * args.size() * 4);

This seems to confirm that always offsetting by VA_SLOT_SIZE_BYTES in the read and write code can be problematic, if arguments can occupy up to 4 slots, always offsetting the read/write cursor by one slot seems incorrect.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3617