8284072: foreign/StdLibTest.java randomly crashes on MacOS/AArch64 #674

nick-arm · 2022-04-01T09:41:10Z

This test fails intermittently on Apple silicon macOS with a JVM SIGBUS and the stack trace below:

  Stack: [0x0000000171758000,0x000000017195b000], sp=0x0000000171957ef0, free space=2047k
  Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native code)
  V [libjvm.dylib+0x984f70] MarkActivationClosure::do_code_blob(CodeBlob*)+0x3c
  V [libjvm.dylib+0x9bfb60] JavaThread::nmethods_do(CodeBlobClosure*)+0xac
  V [libjvm.dylib+0x4669a8] HandshakeState::process_by_self(bool)+0x31c
  V [libjvm.dylib+0x891230] SafepointMechanism::process(JavaThread*, bool)+0xac
  V [libjvm.dylib+0x9f4df0] UpcallLinker::on_entry(UpcallStub::FrameData*)+0xe0
  v blob 0x000000010e1ca4a8
  C [libsystem_c.dylib+0x5c284] _isort+0x88
  C 0x176d80010e0ca444
  j java.lang.invoke.LambdaForm$MH+0x0000000800c82000.invoke(Ljava/lang/Object;JJJJJ)V+16 java.base@19-internal

In UpcallLinker::on_entry we call ThreadStateTransition::transition_from_native() which needs the code cache to be writable if it suspends for a safepoint/handshake.

Use the ThreadWXEnable helper to temporarily set the W^X state to W while inside on_entry() and then toggle it back to X before returning to the upcall stub.

Tested jdk_foreign plus looped java/foreign/StdLibTest.java for several hours on an M1 Mac Mini which used to eventually fail.

Progress

Change must not contain extraneous whitespace
Change must be properly reviewed

Issue

JDK-8284072: foreign/StdLibTest.java randomly crashes on MacOS/AArch64

Reviewers

Jorn Vernee (@JornVernee - Committer)
Maurizio Cimadamore (@mcimadamore - Committer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/panama-foreign pull/674/head:pull/674
$ git checkout pull/674

Update a local copy of the PR:
$ git checkout pull/674
$ git pull https://git.openjdk.java.net/panama-foreign pull/674/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 674

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/panama-foreign/pull/674.diff

This test fails intermittently on Apple silicon macOS with a JVM SIGBUS and the stack trace below: Stack: [0x0000000171758000,0x000000017195b000], sp=0x0000000171957ef0, free space=2047k Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native code) V [libjvm.dylib+0x984f70] MarkActivationClosure::do_code_blob(CodeBlob*)+0x3c V [libjvm.dylib+0x9bfb60] JavaThread::nmethods_do(CodeBlobClosure*)+0xac V [libjvm.dylib+0x4669a8] HandshakeState::process_by_self(bool)+0x31c V [libjvm.dylib+0x891230] SafepointMechanism::process(JavaThread*, bool)+0xac V [libjvm.dylib+0x9f4df0] UpcallLinker::on_entry(UpcallStub::FrameData*)+0xe0 v blob 0x000000010e1ca4a8 C [libsystem_c.dylib+0x5c284] _isort+0x88 C 0x176d80010e0ca444 j java.lang.invoke.LambdaForm$MH+0x0000000800c82000.invoke(Ljava/lang/Object;JJJJJ)V+16 java.base@19-internal In UpcallLinker::on_entry we call ThreadStateTransition::transition_from_native() which needs the code cache to be writable if it suspends for a safepoint/handshake. Use the ThreadWXEnable helper to temporarily set the W^X state to W while inside on_entry() and then toggle it back to X before returning to the upcall stub. Tested jdk_foreign plus looped java/foreign/StdLibTest.java for several hours on an M1 Mac Mini which used to eventually fail.

bridgekeeper · 2022-04-01T09:42:31Z

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

mlbridge · 2022-04-01T09:47:26Z

Webrevs

00: Full (4fd35561)

JornVernee

Thanks for fixing this, Nick!

openjdk · 2022-04-01T09:50:33Z

@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:

8284072: foreign/StdLibTest.java randomly crashes on MacOS/AArch64

Reviewed-by: jvernee, mcimadamore

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 foreign-preview 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 foreign-preview branch, type /integrate in a new comment.

nick-arm · 2022-04-01T10:19:09Z

Thanks @JornVernee. Just to confirm: foreign-preview is the right target branch?

mcimadamore · 2022-04-01T11:04:10Z

Thanks @JornVernee. Just to confirm: foreign-preview is the right target branch?
Yes

mcimadamore · 2022-04-01T11:06:29Z

src/hotspot/share/prims/upcallLinker.cpp

@@ -72,6 +72,10 @@ JavaThread* UpcallLinker::on_entry(UpcallStub::FrameData* context) {
  // clear any pending exception in thread (native calls start with no exception pending)
  thread->clear_pending_exception();

+  // The call to transition_from_native below contains a safepoint check
+  // which needs the code cache to be writable.
+  MACOS_AARCH64_ONLY(ThreadWXEnable wx(WXWrite, thread));


Naive question: should we switch back to non-writeable after the transition? E.g. I get that this probably is reverted at the end of this function - I guess I'm asking if it's ok to keep it writeable for longer than we should/need?

That happens when the wx variable goes out of scope: ThreadWXEnable is a RAII object which saves the previous mode in its constructor and then restores in the destructor.

Yes it doesn't matter that it's writable longer than strictly necessary, as long as we make it executable again before returning to the stub. We enable the W mode around the whole body of other VM entries - e.g. see VM_LEAF_BASE in interfaceSupport.inline.hpp.

JornVernee · 2022-04-01T11:57:36Z

@nick-arm foreign-preview is the current branch we are developing on, so that is the right branch.

jextract has also been moved to a standalone repo, so after the current JEP integration and followup patches for the VM changes make it into mainline we might move development to the jdk repo.

After this PR is integrated, I will pull the commit into the PR I have here: openjdk/jdk#7959 and then I'll probably mark that ready for review.

nick-arm · 2022-04-01T14:52:21Z

/integrate

openjdk · 2022-04-01T14:53:39Z

Going to push as commit 2ca24e3.

openjdk · 2022-04-01T14:53:53Z

@nick-arm Pushed as commit 2ca24e3.

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

openjdk bot added the rfr Ready for review label Apr 1, 2022

JornVernee approved these changes Apr 1, 2022

View reviewed changes

openjdk bot added the ready Ready to be integrated label Apr 1, 2022

mcimadamore reviewed Apr 1, 2022

View reviewed changes

mcimadamore approved these changes Apr 1, 2022

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Apr 1, 2022

openjdk bot closed this Apr 1, 2022

openjdk bot removed ready Ready to be integrated rfr Ready for review labels Apr 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

8284072: foreign/StdLibTest.java randomly crashes on MacOS/AArch64 #674

8284072: foreign/StdLibTest.java randomly crashes on MacOS/AArch64 #674

Uh oh!

nick-arm commented Apr 1, 2022 •

edited by openjdk bot

Loading

Uh oh!

bridgekeeper bot commented Apr 1, 2022

Uh oh!

mlbridge bot commented Apr 1, 2022

Uh oh!

JornVernee left a comment

Uh oh!

openjdk bot commented Apr 1, 2022 •

edited

Loading

Uh oh!

nick-arm commented Apr 1, 2022

Uh oh!

mcimadamore commented Apr 1, 2022

Uh oh!

mcimadamore Apr 1, 2022 •

edited

Loading

Uh oh!

nick-arm Apr 1, 2022

Uh oh!

nick-arm Apr 1, 2022

Uh oh!

JornVernee commented Apr 1, 2022 •

edited

Loading

Uh oh!

nick-arm commented Apr 1, 2022

Uh oh!

openjdk bot commented Apr 1, 2022

Uh oh!

openjdk bot commented Apr 1, 2022

Uh oh!

Uh oh!

8284072: foreign/StdLibTest.java randomly crashes on MacOS/AArch64 #674

8284072: foreign/StdLibTest.java randomly crashes on MacOS/AArch64 #674

Uh oh!

Conversation

nick-arm commented Apr 1, 2022 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Apr 1, 2022

Uh oh!

mlbridge bot commented Apr 1, 2022

Webrevs

Uh oh!

JornVernee left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Apr 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nick-arm commented Apr 1, 2022

Uh oh!

mcimadamore commented Apr 1, 2022

Uh oh!

mcimadamore Apr 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nick-arm Apr 1, 2022

Choose a reason for hiding this comment

Uh oh!

nick-arm Apr 1, 2022

Choose a reason for hiding this comment

Uh oh!

JornVernee commented Apr 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nick-arm commented Apr 1, 2022

Uh oh!

openjdk bot commented Apr 1, 2022

Uh oh!

openjdk bot commented Apr 1, 2022

Uh oh!

Uh oh!

nick-arm commented Apr 1, 2022 •

edited by openjdk bot

Loading

openjdk bot commented Apr 1, 2022 •

edited

Loading

mcimadamore Apr 1, 2022 •

edited

Loading

JornVernee commented Apr 1, 2022 •

edited

Loading