-
Notifications
You must be signed in to change notification settings - Fork 168
8212155: Race condition when posting dynamic_code_generated event leads to JVM crash #674
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
base: master
Are you sure you want to change the base?
Conversation
👋 Welcome back krk! A progress list of the required criteria for merging this PR into |
❗ This change is not yet ready to be integrated. |
This backport pull request has now been updated with issue from the original commit. |
@krk Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information. |
Failing tests seem unrelated: Windows x64 release and debug build:
macOS x64 jdk/security_infra:
Looks similar to #673 Linux x86 hotspot/tier1:
Test timed out:
also observed in #628 (comment) |
|
/approval request Fixes a critical JVM crash from a race condition when posting dynamic_code_generated JVMTI events. The patch required manual conflict resolution in JvmtiExport.cpp because JvmtiThreadState::state_for() was previously refactored in this branch. For JDK 8, DynamicCodeGeneratedTest.sh was added as the runner, which compiles the library first. This fix has been stable in mainline since June 2021. The risk is low, as the fix is a minimal, defensive check. |
/approval request Fixes a critical JVM crash from a race condition when posting dynamic_code_generated JVMTI events. We have seen the JVM crashes with production services running with async-profiler on this code path. The patch required manual conflict resolution, see 8u PR. This fix has been stable in mainline since June 2021; there is a minor testbug bugtail. The risk is low and benefit is high: adds a simple defensive check, avoids crashes seen in the wild, matches the backports in all other currently active JDK releases. |
@gnu-andrew, @jerboaa -- would you be the right reviewers for this? It is a fairly safe change that avoids production JVM crashes with profilers. |
Backport of JDK-8212155.
The patch did not apply cleanly due to a merge conflict. The original code
JvmtiThreadState::state_for(JavaThread::current())
had already been refactored toJavaThread::current()->jvmti_thread_state()
in this branch.I have also removed the
@requires vm.jvmti
tag from the test, as it is not supported by the jtreg version in JDK 11.For JDK 8,
DynamicCodeGeneratedTest.sh
was added as the runner, which compiles the library first.The
DynamicCodeGeneratedTest
was confirmed to sometimes crash without this patch and passes with it.Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk8u-dev.git 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.org/jdk8u-dev.git 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.org/jdk8u-dev/pull/674.diff
Using Webrev
Link to Webrev Comment