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

8262894: [macos_aarch64] SIGBUS in Assembler::ld_st2 #3241

Closed

Conversation

AntonKozlov
Copy link
Member

@AntonKozlov AntonKozlov commented Mar 29, 2021

Please review a fix for compiler/debug/VerifyAdapterSharing.java failure on macos/aarch64 platform. The root cause is in missing W^X switch in JNI DestroyJavaVM.

I reviewed the rest of the JNI Invoke Interface functions. DetachCurrentThread needs a similar change, although nothing fails immediately. So DetachCurrentThread is changed as a precaution.


Progress

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

Issue

  • JDK-8262894: [macos_aarch64] SIGBUS in Assembler::ld_st2

Reviewers

Contributors

  • Mikael Vidstedt <mikael@openjdk.org>

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3241

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

Using diff file

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

@bridgekeeper
Copy link

bridgekeeper bot commented Mar 29, 2021

👋 Welcome back akozlov! 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 Pull request is ready for review label Mar 29, 2021
@openjdk
Copy link

openjdk bot commented Mar 29, 2021

@AntonKozlov The following label will be automatically applied to this pull request:

  • hotspot

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 hotspot hotspot-dev@openjdk.org label Mar 29, 2021
@mlbridge
Copy link

mlbridge bot commented Mar 29, 2021

Webrevs

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Choose a reason for hiding this comment

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

Hi Anton,

In so much as I understand the fact transitions are missing the introduction of those transitions seems fine - but can be simplified due to an existing code quirk (see comments below).

My main concern with this W^X stuff is that I don't see a clear way to know exactly where a transition needs to be placed. The missing cases here suggest it should be handled in the thread-state transition code, but you've previously written:

" when we execute JVM code (owned by libjvm.so, starting from JVM entry function), we switch to Write state. When we leave JVM to execute generated or JNI code, we switch to Executable state. I would like to highlight that JVM code does not mean the VM state of the java thread"

so I'm unclear exactly how we identify the points where these transitions must occur? What kind of "VM code" must be guarded this way? I don't see this documented in the code anywhere.

Thanks,
David

@@ -3731,6 +3735,7 @@ static jint JNICALL jni_DestroyJavaVM_inner(JavaVM *vm) {
return res;
} else {
ThreadStateTransition::transition(thread, _thread_in_vm, _thread_in_native);
MACOS_AARCH64_ONLY(thread->enable_wx(oldmode));
Copy link
Member

Choose a reason for hiding this comment

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

This is actually unnecessary as Threads::destroy_vm never returns anything but true (we should change it to a void method and clean this up). If it were to fail you would have to know at what point it failed to determine whether you can actually touch the current thread any more.

@@ -3723,6 +3723,10 @@ static jint JNICALL jni_DestroyJavaVM_inner(JavaVM *vm) {

// Since this is not a JVM_ENTRY we have to set the thread state manually before entering.
JavaThread* thread = JavaThread::current();

// We are going to VM, change W^X state to the expected one.
MACOS_AARCH64_ONLY(WXMode oldmode = thread->enable_wx(WXWrite));
Copy link
Member

Choose a reason for hiding this comment

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

No need to save old state - see below.

@openjdk
Copy link

openjdk bot commented Mar 29, 2021

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

8262894: [macos_aarch64] SIGBUS in Assembler::ld_st2

Co-authored-by: Mikael Vidstedt <mikael@openjdk.org>
Reviewed-by: dholmes, gziemski

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 30 new commits pushed to the master branch:

  • 21e7402: 8263707: C1 RangeCheckEliminator support constant array and NewMultiArray
  • 2ad6f2d: 8263896: Make not_suspended parameter from ObjectMonitor::exit() have default value
  • b652198: 8264429: Test runtime/cds/appcds/VerifyWithDefaultArchive.java assumes OpenJDK build
  • 2c9365d: 8264271: Avoid creating non_oop_word oops
  • daeca3f: 8262958: (fs) UnixUserDefinedFileAttributeView cleanup
  • af02883: 8264191: Javadoc search is broken in Internet Explorer
  • 6e74c3a: 8264193: Remove TRAPS parameters for modules and defaultmethods
  • ee5e00b: 8264279: Shenandoah: Missing handshake after JDK-8263427
  • ac604a1: 8264374: Shenandoah: Remove leftover parallel reference processing argument
  • f3726a8: 8264020: Optimize double negation elimination
  • ... and 20 more: https://git.openjdk.java.net/jdk/compare/aefc1560b51f0ce96d8f5ce396ba0d2fe08fd650...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@dholmes-ora, @gerard-ziemski) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Mar 29, 2021
@AntonKozlov
Copy link
Member Author

Hi David,

Thank you for the review.

so I'm unclear exactly how we identify the points where these transitions must occur? What kind of "VM code" must be guarded this way? I don't see this documented in the code anywhere.

For usual JNI function implementation we switch to WXWrite in JNI_ENTRY (JNI_ENTRY_NO_PRESERVE to be precise), so xxx_ENTRY style macro defines a border between JVM and the rest of the code. JNI Invocation functions are also called directly from native code, so they should have W^X transition. But since they are implementing something rather special, they don't use any special ENTRY macro, which makes them less evident to be JVM entry points. Here and in other cases, when we do W^X in an apparently random function, it is because the function is called directly from the interpreter or native or generated code, but the function is not defined with ENTRY macro. Hope it clarifies the logic a bit.

I've checked Threads::destroy_vm, you're right! But I'd consider missing handling after possible false as a bug. If you don't mind, I prefer the extra code in the existing else clause. But I agree the whole clause can be cleaned up. Thanks for pointing!

@mlbridge
Copy link

mlbridge bot commented Mar 30, 2021

Mailing list message from David Holmes on hotspot-dev:

On 30/03/2021 5:18 am, Anton Kozlov wrote:

On Mon, 29 Mar 2021 13:44:18 GMT, David Holmes <dholmes at openjdk.org> wrote:

Please review a fix for compiler/debug/VerifyAdapterSharing.java failure on macos/aarch64 platform. The root cause is in missing W^X switch in JNI DestroyJavaVM.

I reviewed the rest of the JNI Invoke Interface functions. DetachCurrentThread needs a similar change, although nothing fails immediately. So DetachCurrentThread is changed as a precaution.

Hi Anton,

In so much as I understand the fact transitions are missing the introduction of those transitions seems fine - but can be simplified due to an existing code quirk (see comments below).

My main concern with this W^X stuff is that I don't see a clear way to know exactly where a transition needs to be placed. The missing cases here suggest it should be handled in the thread-state transition code, but you've previously written:

" when we execute JVM code (owned by libjvm.so, starting from JVM entry function), we switch to Write state. When we leave JVM to execute generated or JNI code, we switch to Executable state. I would like to highlight that JVM code does not mean the VM state of the java thread"

so I'm unclear exactly how we identify the points where these transitions must occur? What kind of "VM code" must be guarded this way? I don't see this documented in the code anywhere.

Thanks,
David

Hi David,

Thank you for the review.

so I'm unclear exactly how we identify the points where these transitions must occur? What kind of "VM code" must be guarded this way? I don't see this documented in the code anywhere.

For usual JNI function implementation we switch to WXWrite in JNI_ENTRY (JNI_ENTRY_NO_PRESERVE to be precise), so xxx_ENTRY style macro defines a border between JVM and the rest of the code. JNI Invocation functions are also called directly from native code, so they should have W^X transition. But since they are implementing something rather special, they don't use any special ENTRY macro, which makes them less evident to be JVM entry points. Here and in other cases, when we do W^X in an apparently random function, it is because the function is called directly from the interpreter or native or generated code, but the function is not defined with ENTRY macro. Hope it clarifies the logic a bit.

I get the gist of where you have placed the transitions around the
various "entry" points to VM code, but I don't understand what kind of
VM code actually requires these transitions. For example a LEAF function
doesn't have the transition so the code called from there can't require
it - but what is the characteristic of code that can require it?

I've checked `Threads::destroy_vm`, you're right! But I'd consider missing handling after possible `false` as a bug. If you don't mind, I prefer the extra code in the existing else clause. But I agree the whole clause can be cleaned up. Thanks for pointing!

I'll clean this part up under JDK-8264372.

Cheers,
David

@AntonKozlov
Copy link
Member Author

what kind of
VM code actually requires these transitions. For example a LEAF function
doesn't have the transition so the code called from there can't require
it - but what is the characteristic of code that can require it?

I assume any VM entry code should have W^X transitions. Now VM_LEAF_BASE transits to WXWrite, and the macro is used in various kinds of LEAF's. An initial W^X implementation had no transitions in LEAF functions, but now the code and W^X policy should be much more straightforward. If we still don't have the transition somewhere, there should be a reason and a comment in the code. Otherwise, it's a bug :)

@AntonKozlov
Copy link
Member Author

/contributor add mikael
/integrate

@openjdk
Copy link

openjdk bot commented Mar 30, 2021

@AntonKozlov
Contributor Mikael Vidstedt <mikael@openjdk.org> successfully added.

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Mar 30, 2021
@openjdk
Copy link

openjdk bot commented Mar 30, 2021

@AntonKozlov
Your change (at version 4487f7c) is now ready to be sponsored by a Committer.

@theRealAph
Copy link
Contributor

I assume any VM entry code should have W^X transitions.

That sounds very extreme to me. I guess it all depends on the cost of doing the transition.

I'm curious why the W^X transition isn't done only when accessing the code cache.

@AntonKozlov
Copy link
Member Author

I assume any VM entry code should have W^X transitions.

That sounds very extreme to me. I guess it all depends on the cost of doing the transition.

It is made for simplicity and robustness. I did a few iterations fixing various issues until this scheme came up, which I pretty confident now in correctness. Yes, it depends on the transition to be fast enough. To get the sense of "fast" I did #2200 (comment). But this approach can be and probably should be optimized.

I'm curious why the W^X transition isn't done only when accessing the code cache.

There are a lot of places where we need or potentially need the transition. Every deoptimization may want to write to codecache. Adding explicit transitions is rather tedious and error-prone. I've tried this and it takes significant time to get to java -version working, without guarantees it will work to something bigger.

Another approach is to do a lazy transition to WXWrite on segfault after we try to write to codecache in VM and to ensure WXExec during exit from VM. Is it beneficial or not depends on a ratio between entries into runtime and ones actually need WXWrite.

This raises a question about a realistic workload to measure effects after various W^X approaches. I used the first iteration of macro benchmarks like Renaissance and SPECjvm2008 with zero warm-up, in the assumption that the most number of runtime calls and deoptimization happens there. Is there a better workload for this purpose?

Copy link

@gerard-ziemski gerard-ziemski left a comment

Choose a reason for hiding this comment

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

Looks good to me, thank you for fixing it.

@VladimirKempik
Copy link

/sponsor

@openjdk openjdk bot closed this Mar 31, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed sponsor Pull request is ready to be sponsored ready Pull request is ready to be integrated rfr Pull request is ready for review labels Mar 31, 2021
@openjdk
Copy link

openjdk bot commented Mar 31, 2021

@VladimirKempik @AntonKozlov Since your change was applied there have been 37 commits pushed to the master branch:

  • ab6faa6: 8263582: WB_IsMethodCompilable ignores compiler directives
  • 928fa5b: 8244540: Print more information with -XX:+PrintSharedArchiveAndExit
  • e073486: 8262093: java/util/concurrent/tck/JSR166TestCase.java failed "assert(false) failed: unexpected node"
  • 815248a: 8264148: Update spec for exceptions retrofitted for exception chaining
  • 353807c: 8263898: (fs) Files.newOutputStream on the "NUL" special device throws FileSystemException: "nul: Incorrect function" (win)
  • 2bd80f9: 8264326: Modernize javax.script.ScriptEngineManager and related classes' implementation
  • b08d638: 8262503: Support records in Dynalink
  • 21e7402: 8263707: C1 RangeCheckEliminator support constant array and NewMultiArray
  • 2ad6f2d: 8263896: Make not_suspended parameter from ObjectMonitor::exit() have default value
  • b652198: 8264429: Test runtime/cds/appcds/VerifyWithDefaultArchive.java assumes OpenJDK build
  • ... and 27 more: https://git.openjdk.java.net/jdk/compare/aefc1560b51f0ce96d8f5ce396ba0d2fe08fd650...master

Your commit was automatically rebased without conflicts.

Pushed as commit 8a4a911.

💡 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
hotspot hotspot-dev@openjdk.org integrated Pull request has been integrated
5 participants