Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 9 commits into from

Conversation

@nick-arm
Copy link
Member

@nick-arm nick-arm commented Apr 22, 2021

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


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8263512: [macos_aarch64] issues with calling va_args functions from invoke_native

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3617/head:pull/3617
$ git checkout pull/3617

Update a local copy of the PR:
$ git checkout pull/3617
$ git pull https://git.openjdk.java.net/jdk pull/3617/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3617

View PR using the GUI difftool:
$ git pr show -t 3617

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3617.diff

…nvoke_native

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.
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Apr 22, 2021

👋 Welcome back ngasson! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr label Apr 22, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Apr 22, 2021

@nick-arm The following label will be automatically applied to this pull request:

  • core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the core-libs label Apr 22, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 22, 2021

Webrevs

@nick-arm
Copy link
Member Author

@nick-arm nick-arm commented Apr 22, 2021

@mcimadamore @JornVernee could you take a look? Thanks!

@JornVernee
Copy link
Member

@JornVernee JornVernee commented Apr 26, 2021

Hi Nick. Sorry for the late reply, I've been out sick. I'll hopefully be taking a thorough look at this soon (still catching up on things).

I'm pretty impressed that such a large amount of code can just be shared between the two platforms :)

Copy link
Contributor

@mcimadamore mcimadamore left a comment

I'll leave fine details to @JornVernee - high level bits seem correct. I think we should be more transparent in e.g. enum constant on the fact that AArch64 really means linux - and perhaps use Arm64 instead of AArch64.

Copy link
Member

@JornVernee JornVernee left a comment

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.

@Override
public MethodHandle downcallHandle(Addressable symbol, MethodType type, FunctionDescriptor function) {
Objects.requireNonNull(symbol);
Objects.requireNonNull(type);
Objects.requireNonNull(function);
MethodType llMt = SharedUtils.convertVaListCarriers(type, StackVaList.CARRIER);
MethodHandle handle = CallArranger.arrangeDowncall(symbol, llMt, function);
handle = SharedUtils.unboxVaLists(type, handle, MH_unboxVaList);
return handle;
}
Comment on lines 77 to 86

This comment has been minimized.

@JornVernee

JornVernee May 3, 2021
Member

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 #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.

@nick-arm
Copy link
Member Author

@nick-arm nick-arm commented May 6, 2021

@JornVernee thanks for the review. I'll park this until the JEP is integrated and then fix it up afterwards.

@JornVernee
Copy link
Member

@JornVernee JornVernee commented Jun 2, 2021

The JEP has been integrated, so we can pick this back up (and handle it as a bug for 17 even after the fork).

Thank you for your patience.

nick-arm added 3 commits Jun 3, 2021
Change-Id: Ic06fec084099ff2053dd251a255cbbf4a64a59d7
CustomizedGitHooks: yes
Change-Id: Iaa13b3869522c8814c3f7ef4c1eac8e8267657e6
CustomizedGitHooks: yes
Change-Id: Ibf91c570c88be2587e9e23240477c4a5cc56b4c5
CustomizedGitHooks: yes
@nick-arm
Copy link
Member Author

@nick-arm nick-arm commented Jun 3, 2021

I've rebased this on the latest master after the JEP integration and fixed the issues above. Should be good to re-review. I've done the suggested renaming to LinuxAArch64Linker, MacOsAArch64Linker, etc.

Copy link
Member

@JornVernee JornVernee left a comment

Looks very good, thanks!

@openjdk
Copy link

@openjdk openjdk bot commented Jun 3, 2021

@nick-arm This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

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

Reviewed-by: jvernee

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Jun 3, 2021
@nick-arm
Copy link
Member Author

@nick-arm nick-arm commented Jun 4, 2021

Thanks @JornVernee! I noticed VaListTest has started failing on Windows with this error:

test VaListTest.testUpcall(java.lang.invoke.BoundMethodHandle$Species_LLLLLLL@198ebce4, MethodHandle(VaList)void): success
test VaListTest.testUpcall(java.lang.invoke.BoundMethodHandle$Species_LLLLLLL@7a97cd30, MethodHandle(VaList)void): success
Uncaught exception:
java.lang.IllegalArgumentException 
{0x00000000d6506c20} - klass: 'java/lang/IllegalArgumentException'
#
# A fatal error has been detected by the Java Runtime Environment:
#
#  Internal Error (universalUpcallHandler.cpp:113), pid=13972, tid=23500
#  Error: ShouldNotReachHere()
#

I guess it must be related to the two new cases I added and the Windows code is now throwing an IllegalArgumentException but I can't see where from. Any ideas?

@JornVernee
Copy link
Member

@JornVernee JornVernee commented Jun 4, 2021

Hey @nick-arm I'm on Windows, so I can take a look here. We recently added a patch that handles these exceptions better and actually prints the stack trace as well.

Copy link
Contributor

@mcimadamore mcimadamore left a comment

Looks good - except for the issue raised by Jorn (carrier mismatch on test)

nick-arm and others added 3 commits Jun 4, 2021
Co-authored-by: Jorn Vernee <JornVernee@users.noreply.github.com>
Change-Id: I49bef0437b4c47bef8bf74d192299d06b25e1555
CustomizedGitHooks: yes
@mcimadamore
Copy link
Contributor

@mcimadamore mcimadamore commented Jun 4, 2021

@nick-arm, I've just integrated a fix which, I believe will create a minor build issue with the changes in this patch:

https://git.openjdk.java.net/jdk/pull/4316

That fix has a switch on the ABI type in the SystemLookup class (a new class introduced by that fix). I believe that switch will no longer compile with the changes in this PR as the ABI enum constants have changed - hopefully the fix should be easy - e.g. just replace the case label with references AARCH64 with a compound case label which covers both ARM/linux and ARM/MacOS.

@nick-arm
Copy link
Member Author

@nick-arm nick-arm commented Jun 4, 2021

That fix has a switch on the ABI type in the SystemLookup class (a new class introduced by that fix). I believe that switch will no longer compile with the changes in this PR as the ABI enum constants have changed - hopefully the fix should be easy - e.g. just replace the case label with references AARCH64 with a compound case label which covers both ARM/linux and ARM/MacOS.

OK sure, the last commit fixes that.

@nick-arm
Copy link
Member Author

@nick-arm nick-arm commented Jun 4, 2021

/integrate

@openjdk openjdk bot closed this Jun 4, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Jun 4, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jun 4, 2021

@nick-arm Since your change was applied there have been 6 commits pushed to the master branch:

  • 4e6748c: 8267687: ModXNode::Ideal optimization is better than Parse::do_irem
  • 48dc72b: 8268272: Remove JDK-8264874 changes because Graal was removed.
  • 20b6312: 8268151: Vector API toShuffle optimization
  • 64ec8b3: 8212155: Race condition when posting dynamic_code_generated event leads to JVM crash
  • cd0678f: 8199318: add idempotent copy operation for Map.Entry
  • b27599b: 8268222: javax/xml/jaxp/unittest/transform/Bug6216226Test.java failed, cannot delete file

Your commit was automatically rebased without conflicts.

Pushed as commit 76b54a1.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants