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

8245877: assert(_value != __null) failed: resolving NULL _value in JvmtiExport::post_compiled_method_load #4602

Closed
wants to merge 7 commits into from

Conversation

@lmesnik
Copy link
Member

@lmesnik lmesnik commented Jun 26, 2021

The crash happens because nmethod might become a zombie before it is enqueued in JvmtiDeferredEventQueue or after it is dequeued from it. The crash is reproduced by serviceability/jvmti/CompiledMethodLoad/Zombie.java. However, it takes ~3K runs to hit it. I verified the fix by running this test >100K on each platform. Also, I verified that protecting in 'void JvmtiDeferredEventQueue::post(JvmtiEnv* env)' is not enough.


Progress

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

Issue

  • JDK-8245877: assert(_value != __null) failed: resolving NULL _value in JvmtiExport::post_compiled_method_load

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4602

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jun 26, 2021

👋 Welcome back lmesnik! 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
Copy link

@openjdk openjdk bot commented Jun 26, 2021

@lmesnik The following labels will be automatically applied to this pull request:

  • hotspot
  • serviceability

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

@openjdk
Copy link

@openjdk openjdk bot commented Jun 28, 2021

@lmesnik this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout 8245877
git fetch https://git.openjdk.java.net/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

@openjdk openjdk bot removed the merge-conflict label Jun 28, 2021
@lmesnik lmesnik marked this pull request as ready for review Jun 28, 2021
@openjdk openjdk bot added the rfr label Jun 28, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Jun 28, 2021

Webrevs

Copy link
Contributor

@sspitsyn sspitsyn left a comment

Hi Leonid,
I looks good to me.
Thank you for addressing it!
Thanks,
Serguei

@openjdk
Copy link

@openjdk openjdk bot commented Jun 28, 2021

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

8245877: assert(_value != __null) failed: resolving NULL _value in JvmtiExport::post_compiled_method_load

Reviewed-by: sspitsyn, dholmes, coleenp

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

  • 1ac8dee: 8269615: Fix for 8263640 broke Windows build
  • 7ca753b: 8269268: JDWP: Properly fix thread lookup assert in findThread()
  • 7a23c9c: 8260540: serviceability/jdwp/AllModulesCommandTest.java failed with "Debuggee error: 'ERROR: transport error 202: bind failed: Address already in use'"
  • c1e2a29: 8263640: hs_err improvement: handle class path longer than O_BUFLEN
  • bb42d75: 8269417: Minor clarification on NonblockingQueue utility
  • e238cbd: 8269530: runtime/ParallelLoad/ParallelSuperTest.java timeout
  • 3f2c372: 8269126: Rename G1AllowPreventiveGC option to G1UsePreventiveGC
  • a977157: 8261579: AArch64: Support for weaker memory ordering in Atomic
  • ee1e202: 8268821: Split systemDictionaryShared.cpp
  • 03d54e6: Merge
  • ... and 7 more: https://git.openjdk.java.net/jdk/compare/d0d26f5c550e44bf22d9155e4219351104be4635...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.

➡️ 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 28, 2021
Copy link
Member

@dholmes-ora dholmes-ora left a comment

Hi Leonid,

I'm not clear on the details here - please see comments below.

Thanks,
David

for (QueueNode* node = _queue_head; node != NULL; node = node->next()) {
node->event().post_compiled_method_load_event(env);
}
Copy link
Member

@dholmes-ora dholmes-ora Jun 29, 2021

Can't you dequeue() immediately after calling post_compiled_method_load_event()?

Copy link
Member Author

@lmesnik lmesnik Jun 29, 2021

Seems that dequeue in for-loop deletes the node which posted. It is possible to update the loop to have update dequeue in the same iteration however, I don't think it is a good idea to mix iterator/deletion in the same loop. What is the reason for this change?

Copy link
Member

@dholmes-ora dholmes-ora Jun 29, 2021

Just to save iterating the queue twice. And that is what the original loop did - you just have to switch the order of processing and deleting.

@@ -1608,6 +1608,7 @@ void nmethod::post_compiled_method_load_event(JvmtiThreadState* state) {
if (is_not_entrant() && can_convert_to_zombie()) {
return;
}
mark_as_seen_on_stack();
Copy link
Member

@dholmes-ora dholmes-ora Jun 29, 2021

Not obvious what this actually does in relation to the dequeuing problem.

Copy link
Member Author

@lmesnik lmesnik Jun 29, 2021

updated comments

Copy link

@coleenp coleenp Jun 29, 2021

I'm not 100% this is needed because we don't handshake this thread and we just checked that it can't be converted to zombie, but I don't think this hurts anything and I thought it might be needed.

@lmesnik
Copy link
Member Author

@lmesnik lmesnik commented Jun 29, 2021

Replied to [@dholmes-ora] comments but don't see any notifications from GitHub yet. Not clear if it is my PR/GitHub/mail issues.

@dholmes-ora
Copy link
Member

@dholmes-ora dholmes-ora commented Jun 29, 2021

I see the comment update in the PR and email, but no response to my comment about dequeuing within the for-loop.

Copy link
Member Author

@lmesnik lmesnik left a comment

replied to comments

@@ -1608,6 +1608,7 @@ void nmethod::post_compiled_method_load_event(JvmtiThreadState* state) {
if (is_not_entrant() && can_convert_to_zombie()) {
return;
}
mark_as_seen_on_stack();
Copy link
Member Author

@lmesnik lmesnik Jun 29, 2021

updated comments

@lmesnik
Copy link
Member Author

@lmesnik lmesnik commented Jun 29, 2021

[@dholmes-ora] I've updated the loop in the post. Seems your comments were removed because I remove the first loop.

Copy link

@coleenp coleenp left a comment

Looks good!

@@ -1608,6 +1608,7 @@ void nmethod::post_compiled_method_load_event(JvmtiThreadState* state) {
if (is_not_entrant() && can_convert_to_zombie()) {
return;
}
mark_as_seen_on_stack();
Copy link

@coleenp coleenp Jun 29, 2021

I'm not 100% this is needed because we don't handshake this thread and we just checked that it can't be converted to zombie, but I don't think this hurts anything and I thought it might be needed.

JvmtiDeferredEvent event = dequeue();
event.post_compiled_method_load_event(env);
_queue_head->event().post_compiled_method_load_event(env);
dequeue();
Copy link

@coleenp coleenp Jun 29, 2021

Good find! So we can zombie the nmethod after we take it off the list. Makes a lot of sense. Thank you for your perseverance in tracking this down!

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Looks good!

Thanks,
David

@lmesnik
Copy link
Member Author

@lmesnik lmesnik commented Jun 30, 2021

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Jun 30, 2021

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

  • ee526a2: Merge
  • 0d745ae: 8269034: AccessControlException for SunPKCS11 daemon threads
  • d042029: 8269529: javax/swing/reliability/HangDuringStaticInitialization.java fails in Windows debug build
  • 401cb0a: 8269232: assert(!is_jweak(handle)) failed: wrong method for detroying jweak
  • b8a16e9: 8268884: C2: Compile::remove_speculative_types must iterate top-down
  • 25f9f19: 8249646: Runtime.exec(String, String[], File) documentation contains literal {@link ...}
  • 0d83dc2: 8268699: Shenandoah: Add test for JDK-8268127
  • 7010dfd: 8269517: compiler/loopopts/TestPartialPeelingSinkNodes.java crashes with -XX:+VerifyGraphEdges
  • fc0fd96: 8269126: Rename G1AllowPreventiveGC option to G1UsePreventiveGC
  • 1ac8dee: 8269615: Fix for 8263640 broke Windows build
  • ... and 16 more: https://git.openjdk.java.net/jdk/compare/d0d26f5c550e44bf22d9155e4219351104be4635...master

Your commit was automatically rebased without conflicts.

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

@openjdk openjdk bot commented Jun 30, 2021

@lmesnik Pushed as commit b969136.

💡 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
4 participants