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

8253795: Implementation of JEP 391: macOS/AArch64 Port #715

Closed
wants to merge 13 commits into from

Conversation

VladimirKempik
Copy link

@VladimirKempik VladimirKempik commented Dec 13, 2021

Initial version of JEP-391 backport to jdk11u.
Build system changes are mostly clean, except cds disabling part, it's copy&paste from aix part.
Things needing attention: os_bsd_aarch64.cpp and W^X transitions.
serviceability agent is mostly clean.
This passed GHA_tier1 testing.
Full regression testing is running now on Azul's infra, will report/update PR when done.
Update: TCK passed, full regression testing is fine on intel platforms, macos/aarch64/openjdk11 is good compared to macos/aarch64/zulu11
Sharing this PR slightly earlier to let other interested parties to run the tests too.
Example of cross-building on intel mac (needs Xcode12/13):
sh configure --with-boot-jdk=/Library/Java/JavaVirtualMachines/zulu-11.jdk/Contents/Home/ --with-build-jdk=/Library/Java/JavaVirtualMachines/zulu-11.jdk/Contents/Home/ --disable-warnings-as-errors --openjdk-target=aarch64-apple-darwin


Progress

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

Issues

  • JDK-8253795: Implementation of JEP 391: macOS/AArch64 Port
  • JDK-8253816: Support macOS W^X
  • JDK-8253817: Support macOS Aarch64 ABI in Interpreter
  • JDK-8253818: Support macOS Aarch64 ABI for compiled wrappers
  • JDK-8253819: Implement os/cpu for macOS/AArch64
  • JDK-8253839: Update tests and JDK code for macOS/Aarch64
  • JDK-8254941: Implement Serviceability Agent for macOS/AArch64
  • JDK-8255776: Change build system for macOS/AArch64
  • JDK-8262903: [macos_aarch64] Thread::current() called on detached thread
  • JDK-8262896: [macos_aarch64] Crash in jni_fast_GetLongField

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk11u-dev pull/715/head:pull/715
$ git checkout pull/715

Update a local copy of the PR:
$ git checkout pull/715
$ git pull https://git.openjdk.java.net/jdk11u-dev pull/715/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 715

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk11u-dev/pull/715.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Dec 13, 2021

👋 Welcome back vkempik! 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 changed the title Backport dbc9e4b50cdda35f5712deaf440c49f50b9edc96 8253795: Implementation of JEP 391: macOS/AArch64 Port Dec 13, 2021
@openjdk
Copy link

openjdk bot commented Dec 13, 2021

This backport pull request has now been updated with issues from the original commit.

@openjdk openjdk bot added backport rfr Pull request is ready for review labels Dec 13, 2021
@openjdk
Copy link

openjdk bot commented Dec 13, 2021

@VladimirKempik
Updating description of additional solved issue: 8253816: Support macOS W^X.

@openjdk
Copy link

openjdk bot commented Dec 13, 2021

@VladimirKempik
Updating description of additional solved issue: 8253817: Support macOS Aarch64 ABI in Interpreter.

@openjdk
Copy link

openjdk bot commented Dec 13, 2021

@VladimirKempik
Updating description of additional solved issue: 8253818: Support macOS Aarch64 ABI for compiled wrappers.

@openjdk
Copy link

openjdk bot commented Dec 13, 2021

@VladimirKempik
Updating description of additional solved issue: 8253819: Implement os/cpu for macOS/AArch64.

@openjdk
Copy link

openjdk bot commented Dec 13, 2021

@VladimirKempik
Updating description of additional solved issue: 8253839: Update tests and JDK code for macOS/Aarch64.

@openjdk
Copy link

openjdk bot commented Dec 13, 2021

@VladimirKempik
Adding additional issue to issue list: 8253841: jextract --source mode is broken.

@openjdk
Copy link

openjdk bot commented Dec 13, 2021

@VladimirKempik
Updating description of additional solved issue: 8255776: Change build system for macOS/AArch64.

@openjdk
Copy link

openjdk bot commented Dec 13, 2021

@VladimirKempik
Updating description of additional solved issue: 8262903: [macos_aarch64] Thread::current() called on detached thread.

@openjdk
Copy link

openjdk bot commented Dec 13, 2021

@VladimirKempik
Updating description of additional solved issue: 8254941: Implement Serviceability Agent for macOS/AArch64.

@openjdk
Copy link

openjdk bot commented Dec 13, 2021

@VladimirKempik
Removing additional issue from issue list: 8253841.

@mlbridge
Copy link

mlbridge bot commented Dec 13, 2021

@VladimirKempik
Copy link
Author

VladimirKempik commented Dec 14, 2021

Testing revealed the need to include https://bugs.openjdk.java.net/browse/JDK-8262896 into this PR as well.

8262896: [macos_aarch64] Crash in jni_fast_GetLongField
@VladimirKempik
Copy link
Author

/issue 8262896

@openjdk
Copy link

openjdk bot commented Dec 14, 2021

⚠️ @VladimirKempik This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

@openjdk
Copy link

openjdk bot commented Dec 14, 2021

@VladimirKempik
Adding additional issue to issue list: 8262896: [macos_aarch64] Crash in jni_fast_GetLongField.

@VladimirKempik
Copy link
Author

Together with #716 the hotspot can be built without warnings, some warning in splashscreen yet.

@theRealAph
Copy link
Contributor

This is an extremely difficult patch to review. I'll be soliciting other people's advice to help understand the risk versus reward ratio to bring this in to mainline.
One thing to consider is that, given the good performance of Rosetta, x86 binaries can be used. So, in effect this is only a performance patch for Mac/AArch64. On the other hand, Mac/AArch64 has been working well for some time on the AArch64-port/11u repo.

@VladimirKempik
Copy link
Author

VladimirKempik commented Feb 5, 2022

Crashlog with hsdis - https://pastebin.com/5kz2vfEH

SIGBUS (0xa) at pc=0x000000012100bbfc, pid=76024, tid=39683
siginfo: si_signo: 10 (SIGBUS), si_code: 1 (BUS_ADRALN), si_addr: 0x000000012100bbfc

#128
0x000000012100bbc0: movk x8, #0x2100, lsl #16
0x000000012100bbc4: movk x8, #0x1, lsl #32
0x000000012100bbc8: br x8
0x000000012100bbcc: ldp x22, x24, [x29, #-72]
0x000000012100bbd0: ldur x12, [x29, #-24]
0x000000012100bbd4: ldr x10, [x12, #80]
0x000000012100bbd8: add x0, x28, #0x350
0x000000012100bbdc: adr x8, 0x000000012100bbfc
0x000000012100bbe0: str x8, [x28, #824]
0x000000012100bbe4: str x20, [x28, #816]
0x000000012100bbe8: str x29, [x28, #832]
0x000000012100bbec: orr x8, xzr, #0x4
0x000000012100bbf0: add x9, x28, #0x3c8
0x000000012100bbf4: stlr w8, [x9]
0x000000012100bbf8: blr x10
0x000000012100bbfc: isb

0x000000012100bc00: ldur x12, [x29, #-24]
0x000000012100bc04: sub x8, x20, #0x20
0x000000012100bc08: and sp, x8, #0xfffffffffffffff0
0x000000012100bc0c: str d0, [x20, #-16]!
0x000000012100bc10: str xzr, [x20, #-8]!
0x000000012100bc14: str x0, [x20, #-8]!
0x000000012100bc18: mov x8, #0x5

Pretty strange, @theRealAph, any idea ?

a snippet from macos's crashlog for java
Crashed Thread: 21 Java: MainThread

Exception Type: EXC_BAD_ACCESS (SIGABRT)
Exception Codes: KERN_PROTECTION_FAILURE at 0x000000012103bdac
Exception Note: EXC_CORPSE_NOTIFY

VM Regions Near 0x12103bdac:
MALLOC_SMALL 120800000-121000000 [ 8192K] rw-/rwx SM=PRV
--> VM_ALLOCATE 121000000-121270000 [ 2496K] rwx/rwx SM=ZER
VM_ALLOCATE 121270000-121594000 [ 3216K] ---/rw

38 libjvm.dylib 0x0000000107541f1c JVM_handle_bsd_signal + 820 (os_bsd_aarch64.cpp:471)
39 libjvm.dylib 0x000000010753f9d8 signalHandler(int, __siginfo*, void*) + 56 (os_bsd.cpp:2879)
40 libsystem_platform.dylib 0x0000000194868c44 _sigtramp + 56
41 ??? 0x000000012100bbfc 0 + 4848663548
42 ??? 0x0000000121006ab0 0 + 4848642736
43 ??? 0x000000012100719c 0 + 4848644508
44 ??? 0x0000000121006ee0 0 + 4848643808
45 ??? 0x0000000121006ee0 0 + 4848643808
46 ??? 0x0000000121006ee0 0 + 4848643808
47 ??? 0x000000012100719c 0 + 4848644508
48 ??? 0x0000000121000144 0 + 4848615748
49 libjvm.dylib 0x0000000107322c9c JavaCalls::call_helper(JavaValue*, methodHandle const&, JavaCallArguments*, Thread*) + 548 (javaCalls.cpp:444)

@VladimirKempik
Copy link
Author

VladimirKempik commented Feb 5, 2022

Looks like it was two missing WX transitions in JNI_QUICK_ENTRY and JVM_QUICK_ENTRY
These transitions are present in zulu11
JNI_QUICK_ENTRY and JVM_QUICK_ENTRY macros are missing in jdk17/upstream.

That explains everything, except the macos version reproducibility, which is probably due to the difference in library calls leaving different w^x state up on exit.

@ahmedmuhsin, when you will have a time, please check the updated PR on macos 11.0.1 or 11.2.1, for me it doesn't crash anymore

@ahmedmuhsin
Copy link

@VladimirKempik I tested again on all the OS versions that I mentioned above and everything looks very good. No crashes. Thanks!

@VladimirKempik
Copy link
Author

VladimirKempik commented Feb 8, 2022

@rnkovacs , parts of your jep-388 backport ( VladimirKempik@7bdb8ac7f9f9#diff-0f4150a9c607ccd590bf256daa800c0276144682a92bc6bdced5e8bc1bb81f3aL1495, VladimirKempik@7bdb8ac7f9f9#diff-0f4150a9c607ccd590bf256daa800c0276144682a92bc6bdced5e8bc1bb81f3aL1558, VladimirKempik@7bdb8ac7f9f9#diff-0f4150a9c607ccd590bf256daa800c0276144682a92bc6bdced5e8bc1bb81f3aL1671 ) results in more warning for this PR:

/Users/runner/work/jdk11u-dev/jdk11u-dev/jdk/src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp:1495:52: error: format specifies type 'unsigned long long' but the argument has type 'uintptr_t' (aka 'unsigned long') [-Werror,-Wformat]
snprintf(buffer, sizeof(buffer), PTR64_FORMAT, imm64);
~~~~~~~~~~~~ ^~~~~
1 error generated.

was these three changes really needed or can I revert them ?

reverting these three lines will allow me to fix macos-aarch64 github CI job ( https://github.com/VladimirKempik/jdk11u-dev/runs/5106043614?check_suite_focus=true )

@theRealAph
Copy link
Contributor

theRealAph commented Feb 8, 2022 via email

@VladimirKempik
Copy link
Author

VladimirKempik commented Feb 8, 2022

These three line changes are part of jep-388 (win-arm64) backport to jdk11u-dev, they cause troubles ( warnings which are under Werror becomes an issue)
these changes aren't present in upstream (jdk19), for example - https://github.com/openjdk/jdk/blob/master/src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp#L1316

so I wonder why did they became part of jep-388 backport to jdk11u-dev
if there is no good reason, then I would like to revert these changes as part of this PR.

@theRealAph
Copy link
Contributor

so I wonder why did they became part of jep-388 backport to jdk11u-dev if there is no good reason, then I would like to revert these changes as part of this PR.

There can be no good reason for an incorrect change, so please do that.

@VladimirKempik
Copy link
Author

VladimirKempik commented Feb 8, 2022

Ok.
Also. After latest merge with master this can now be crosscompiled without -with-extra-"c/ld/cxx"flags

@ahmedmuhsin
Copy link

@VladimirKempik should I submit a PR to backport 8241004: NMT tests fail on unaligned thread size with debug build or is it already on your radar?

The backport solves issue 2 from my post here: #715 (comment)

@VladimirKempik
Copy link
Author

Please proceed with 8241004, I have few other macarm related issues on my radar atm.

@VladimirKempik
Copy link
Author

@theRealAph what do you think, is this PR ready to go jdk11u-dev ?
it would be good to merge it some time before rdp2 to have a time for non-critical fixes.

Copy link
Contributor

@theRealAph theRealAph left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good enough.

@VladimirKempik
Copy link
Author

Oracle has pulled this into their java11 - https://bugs.openjdk.java.net/browse/JDK-8275573

@GoeLin is there anything holding us from getting approval for 11u ?

@VladimirKempik
Copy link
Author

/integrate

@openjdk
Copy link

openjdk bot commented Feb 15, 2022

Going to push as commit eb0708f.
Since your change was applied there have been 13 commits pushed to the master branch:

  • fe17b44: 8280414: Memory leak in DefaultProxySelector
  • fd7d0d3: 8280526: x86_32 Math.sqrt performance regression with -XX:UseSSE={0,1}
  • bd420d2: 8279076: C2: Bad AD file when matching SqrtF with UseSSE=0
  • 29eb422: 8281520: JFR: A wrong parameter is passed to the constructor of LeakKlassWriter
  • d79e710: 8281599: test/lib/jdk/test/lib/KnownOIDs.java is redundant since JDK-8268801
  • 6c26ba2: 8190748: java/text/Format/DateFormat/DateFormatTest.java and NonGregorianFormatTest fail intermittently
  • f4e14a6: 8281061: [s390] JFR runs into assertions while validating interpreter frames
  • 668c200: 8280155: [PPC64, s390] frame size checks are not yet correct
  • 6921496: 8279924: [PPC64, s390] implement frame::is_interpreted_frame_valid checks
  • 1db4ed1: 8261205: AssertionError: Cannot add metadata to an intersection type
  • ... and 3 more: https://git.openjdk.java.net/jdk11u-dev/compare/afb94aeae8ffab20ca07dd0fad3765fd65a15255...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Feb 15, 2022
@openjdk openjdk bot closed this Feb 15, 2022
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Feb 15, 2022
@openjdk
Copy link

openjdk bot commented Feb 15, 2022

@VladimirKempik Pushed as commit eb0708f.

💡 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
Labels
backport integrated Pull request has been integrated
7 participants