8369482: JVMTI + Loom: JDK-8368159 introduced safepoint poll in disallowed state #27716

xmas92 · 2025-10-09T08:05:27Z

JDK-8368159 added JvmtiExport::has_frame_pops(JavaThread* thread)
which calls JvmtiExport::get_jvmti_thread_state(); which may safepoint.

Example stack trace:

V [libjvm.dylib+0x125f888] VMError::report(outputStream*, bool)+0x1b68 (javaThread.cpp:375)
V [libjvm.dylib+0x1263184] VMError::report_and_die(int, char const*, char const*, char*, Thread*, unsigned char*, void const*, void const*, char const*, int, unsigned long)+0x55c
V [libjvm.dylib+0x5de268] print_error_for_unit_test(char const*, char const*, char*)+0x0
V [libjvm.dylib+0x9427ec] JavaThread::check_for_valid_safepoint_state()+0x120
V [libjvm.dylib+0xe7e4e4] Mutex::lock(Thread*)+0x48
V [libjvm.dylib+0xbbb198] JvmtiThreadState::state_for(JavaThread*, Handle)+0x200
V [libjvm.dylib+0xc462ec] JvmtiEventControllerPrivate::thread_started(JavaThread*)+0x1a0
V [libjvm.dylib+0xc493a4] JvmtiExport::get_jvmti_thread_state(JavaThread*, bool)+0xc0
V [libjvm.dylib+0xc5078c] JvmtiExport::has_frame_pops(JavaThread*)+0x24
V [libjvm.dylib+0x59c398] freeze_epilog(JavaThread*, ContinuationWrapper&, freeze_result)+0xf8
V [libjvm.dylib+0x59b6e8] Config<(oop_kind)0, CardTableBarrierSet>::freeze(JavaThread*, long*)+0x6f4
V [libjvm.dylib+0x59a4d4] int freeze<Config<(oop_kind)0, CardTableBarrierSet>>(JavaThread*, long*)+0x108

I suggest we do double checked on JvmtiExport::can_post_frame_pop() and move the ContinuationWrapper::SafepointOp scope.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8369482: JVMTI + Loom: JDK-8368159 introduced safepoint poll in disallowed state (Bug - P4)

Reviewers

Serguei Spitsyn (@sspitsyn - Reviewer)
Patricio Chilano Mateo (@pchilano - Reviewer)

Contributors

Patricio Chilano Mateo <[email protected]>

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/27716/head:pull/27716
$ git checkout pull/27716

Update a local copy of the PR:
$ git checkout pull/27716
$ git pull https://git.openjdk.org/jdk.git pull/27716/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 27716

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/27716.diff

Using Webrev

Link to Webrev Comment

…lowed state

bridgekeeper · 2025-10-09T08:06:33Z

👋 Welcome back aboldtch! 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 · 2025-10-09T08:07:22Z

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

8369482: JVMTI + Loom: JDK-8368159 introduced safepoint poll in disallowed state

Co-authored-by: Patricio Chilano Mateo <[email protected]>
Reviewed-by: sspitsyn, pchilanomate

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

d47e6b7: 8369469: Rdtsc: Remove potential races in Rdtsc::initialize
d278043: 8367772: Refactor createUI in PassFailJFrame
62fa971: 8369255: Assess and remedy any unsafe usage of the Semaphores used by JFR
... and 49 more: https://git.openjdk.org/jdk/compare/991f8e6f385ab85b33d2e4d274506995b651ce65...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 · 2025-10-09T08:08:00Z

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

hotspot-runtime

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.

mlbridge · 2025-10-09T08:11:31Z

Webrevs

pchilano · 2025-10-09T17:45:23Z

I think JvmtiExport::has_frame_pops should just check if thread->jvmti_thread_state() is nullptr and return false, and not try to create the state. Same with JvmtiExport::continuation_yield_cleanup(). This JvmtiExport::get_jvmti_thread_state() method was added in 8312174 but I don’t think we want it for this case in freeze. Not only because no state should imply no frame pop requests, but also because it seems the JvmtiThreadState created and set to the platform thread will be for the vthread instance, but the rebind operation has already been executed in VTMS_unmount_begin so we would leave the wrong state in the platform thread until the next transition.

@sspitsyn IIUC this cleanup of the frame_pop requests should only be needed for the plain continuation case, so shouldn’t we have a !cont.entry()->is_virtual_thread() check too?

pchilano · 2025-10-09T17:59:02Z

Also, pre-existing and maybe for a different bug, but seems we are missing a call to invalidate_jvmti_stack() for the preempt case.

dholmes-ora · 2025-10-10T00:32:19Z

/label add serviceability

openjdk · 2025-10-10T00:32:58Z

@dholmes-ora
The serviceability label was successfully added.

sspitsyn · 2025-10-10T06:00:24Z

I think JvmtiExport::has_frame_pops should just check if thread->jvmti_thread_state() is nullptr and return false, and not try to create the state. Same with JvmtiExport::continuation_yield_cleanup(). This JvmtiExport::get_jvmti_thread_state() method was added in 8312174 but I don’t think we want it for this case in freeze. Not only because no state should imply no frame pop requests, but also because it seems the JvmtiThreadState created and set to the platform thread will be for the vthread instance, but the rebind operation has already been executed in VTMS_unmount_begin so we would leave the wrong state in the platform thread until the next transition.

Thank you, Patrico!
I agree with this. Below is the patch for this change.

diff --git a/src/hotspot/share/prims/jvmtiExport.cpp b/src/hotspot/share/prims/jvmtiExport.cpp
index 077b3fec505..fa6ede86cd9 100644
--- a/src/hotspot/share/prims/jvmtiExport.cpp
+++ b/src/hotspot/share/prims/jvmtiExport.cpp
@@ -1694,7 +1694,7 @@ bool JvmtiExport::has_frame_pops(JavaThread* thread) {
   if (!can_post_frame_pop()) {
     return false;
   }
-  JvmtiThreadState *state = get_jvmti_thread_state(thread);
+  JvmtiThreadState *state = thread->jvmti_thread_state();
   if (state == nullptr) {
     return false;
   }
@@ -1713,7 +1713,7 @@ void JvmtiExport::continuation_yield_cleanup(JavaThread* thread, jint continuati
   }
 
   assert(thread == JavaThread::current(), "must be");
-  JvmtiThreadState *state = get_jvmti_thread_state(thread);
+  JvmtiThreadState *state = thread->jvmti_thread_state();
   if (state == nullptr) {
     return;
   }

@sspitsyn IIUC this cleanup of the frame_pop requests should only be needed for the plain continuation case, so shouldn’t we have a !cont.entry()->is_virtual_thread() check too?

Good idea. I was also thinking about it at some point.

Also, pre-existing and maybe for a different bug, but seems we are missing a call to invalidate_jvmti_stack() for the preempt case.

I can be but could you be more presize about the preempt case? What place do you mean?

…in disallowed state" This reverts commit e133d9b.

openjdk · 2025-10-10T06:26:34Z

@xmas92 hotspot has been added to this pull request based on files touched in new commit(s).

xmas92 · 2025-10-10T06:32:52Z

I agree with this. Below is the patch for this change.

Done.

I was also wondering why the original patch change from just JavaThread::jvmti_thread_state to get_jvmti_thread_state. But it looked so intentional, I thought it was fundamental to JDK-8368159.

/author @sspitsyn

openjdk · 2025-10-10T06:35:03Z

@xmas92
Setting overriding author to Serguei Spitsyn <[email protected]>. When this pull request is integrated, the overriding author will be used in the commit.

sspitsyn · 2025-10-10T08:39:37Z

/author @sspitsyn

This suggestion came from Patricio. :)

I was also wondering why the original patch change from just JavaThread::jvmti_thread_state to get_jvmti_thread_state. But it looked so intentional, I thought it was fundamental to JDK-8368159.

Wrong assumptions. :) For consistency with JvmtiExport::continuation_yield_cleanup().

xmas92 · 2025-10-10T09:04:40Z

/contributor add @pchilano

openjdk · 2025-10-10T09:05:41Z

@xmas92
Contributor Patricio Chilano Mateo <[email protected]> successfully added.

sspitsyn · 2025-10-10T10:23:12Z

@sspitsyn IIUC this cleanup of the frame_pop requests should only be needed for the plain continuation case, so shouldn’t we have a !cont.entry()->is_virtual_thread() check too?

This is one more suggestion from Patricio (I'm testing it now):

diff --git a/src/hotspot/share/runtime/continuationFreezeThaw.cpp b/src/hotspot/share/runtime/continuationFreezeThaw.cpp
index 33b4f2bf488..3e509e71551 100644
--- a/src/hotspot/share/runtime/continuationFreezeThaw.cpp
+++ b/src/hotspot/share/runtime/continuationFreezeThaw.cpp
@@ -1626,7 +1626,7 @@ static void invalidate_jvmti_stack(JavaThread* thread) {
 }
 
 static void jvmti_yield_cleanup(JavaThread* thread, ContinuationWrapper& cont) {
-  if (JvmtiExport::has_frame_pops(thread)) {
+  if (!cont.entry()->is_virtual_thread() && JvmtiExport::has_frame_pops(thread)) {
     int num_frames = num_java_frames(cont);
 
     ContinuationWrapper::SafepointOp so(Thread::current(), cont);

Testing this update showed no regressions.

pchilano · 2025-10-10T14:53:41Z

Also, pre-existing and maybe for a different bug, but seems we are missing a call to invalidate_jvmti_stack() for the preempt case.

I can be but could you be more presize about the preempt case? What place do you mean?

So after freezing for the preempt case we should also call invalidate_jvmti_stack() in case there are FramePop requests in the carrier. I’m guessing we don’t have tests for this. But we could address it in a separate bug.

sspitsyn · 2025-10-10T19:48:47Z

So after freezing for the preempt case we should also call invalidate_jvmti_stack() in case there are FramePop requests in the carrier. I’m guessing we don’t have tests for this. But we could address it in a separate bug.

We have jvmti_yield_cleanup() call in the freeze_epilog() which is called from the preempt_epilog(). The jvmti_yield_cleanup() has a call to invalidate_jvmti_stack(). Do I miss anything?

pchilano · 2025-10-10T20:34:42Z

So after freezing for the preempt case we should also call invalidate_jvmti_stack() in case there are FramePop requests in the carrier. I’m guessing we don’t have tests for this. But we could address it in a separate bug.

We have jvmti_yield_cleanup() call in the freeze_epilog() which is called from the preempt_epilog(). The jvmti_yield_cleanup() has a call to invalidate_jvmti_stack(). Do I miss anything?

Yes, but we are calling the other overload of freeze_epilog() which only logs and verifies the continuation. : )

sspitsyn · 2025-10-10T22:29:56Z

Yes, but we are calling the other overload of freeze_epilog() which only logs and verifies the continuation. : )

I see, thanks! Do I understand it right that there is no need to call the jvmti_yield_cleanup() in this case? Does the preempt_epilog() is called for pure continuations as well?

I've filed new JVMTI bug:
8369609 Continuations preempt_epilog is missing a call to invalidate_jvmti_stack

xmas92 · 2025-10-13T08:31:33Z

Pushed the latest patch.

The intent of this bug fix was to solve the safepoint (check) issue.

Is there anything else we should fix as part of this bug fix? Especially given that there is interest in back-porting JDK-8368159.

sspitsyn

Looks good. Thank you for taking care about this!

pchilano · 2025-10-13T13:47:10Z

Yes, but we are calling the other overload of freeze_epilog() which only logs and verifies the continuation. : )

I see, thanks! Do I understand it right that there is no need to call the jvmti_yield_cleanup() in this case? Does the preempt_epilog() is called for pure continuations as well?

I've filed new JVMTI bug: 8369609 Continuations preempt_epilog is missing a call to invalidate_jvmti_stack

Yes, only invalidate_jvmti_stack is missing since preempt_epilog is only called in the virtual thread case.
Thanks for filing the bug Serguei!

pchilano

Looks good to me, thanks.

sspitsyn · 2025-10-13T21:59:51Z

The intent of this bug fix was to solve the safepoint (check) issue.

Agreed. But thank you for fixing the additional issue pointed out by Patricio!

Is there anything else we should fix as part of this bug fix?

The fix is good as is, thanks!

Especially given that there is interest in back-porting JDK-8368159.

I'm curios who do you know is interested in back-porting JDK-8368159. I considered to back-port it but now it needs to be back-ported together with this one: JDK-8369482.

xmas92 · 2025-10-14T05:38:30Z

Thanks for the reviews.
/integrate

openjdk · 2025-10-14T05:40:11Z

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

be0e49b: 8369467: Rdtsc: Remove experimental support for non invariant tsc
4496418: 8365205: C2: Optimize popcount value computation using knownbits
d6ca382: 8369335: Two sun/java2d/OpenGL tests fail on Windows after JDK-8358058
... and 62 more: https://git.openjdk.org/jdk/compare/991f8e6f385ab85b33d2e4d274506995b651ce65...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-10-14T05:40:22Z

@xmas92 Pushed as commit 5bf1bab.

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

8369482: JVMTI + Loom: JDK-8368159 introduced safepoint poll in disal…

e133d9b

…lowed state

openjdk bot added the hotspot-runtime [email protected] label Oct 9, 2025

openjdk bot added the rfr Pull request is ready for review label Oct 9, 2025

openjdk bot added the serviceability [email protected] label Oct 10, 2025

xmas92 and others added 2 commits October 10, 2025 06:24

Revert "8369482: JVMTI + Loom: JDK-8368159 introduced safepoint poll …

2c7a2c3

…in disallowed state" This reverts commit e133d9b.

sspitsyn patch

4ec7ce9

openjdk bot added the hotspot [email protected] label Oct 10, 2025

Check !cont.entry()->is_virtual_thread()

d5a2cc0

sspitsyn approved these changes Oct 13, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 13, 2025

pchilano approved these changes Oct 13, 2025

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Oct 14, 2025

openjdk bot closed this Oct 14, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 14, 2025

8369482: JVMTI + Loom: JDK-8368159 introduced safepoint poll in disallowed state #27716

8369482: JVMTI + Loom: JDK-8368159 introduced safepoint poll in disallowed state #27716

Uh oh!

Conversation

xmas92 commented Oct 9, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Contributors

Reviewing

Uh oh!

bridgekeeper bot commented Oct 9, 2025

Uh oh!

openjdk bot commented Oct 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Oct 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mlbridge bot commented Oct 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

pchilano commented Oct 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pchilano commented Oct 9, 2025

Uh oh!

dholmes-ora commented Oct 10, 2025

Uh oh!

openjdk bot commented Oct 10, 2025

Uh oh!

sspitsyn commented Oct 10, 2025

Uh oh!

openjdk bot commented Oct 10, 2025

Uh oh!

xmas92 commented Oct 10, 2025

Uh oh!

openjdk bot commented Oct 10, 2025

Uh oh!

sspitsyn commented Oct 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xmas92 commented Oct 10, 2025

Uh oh!

openjdk bot commented Oct 10, 2025

Uh oh!

sspitsyn commented Oct 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pchilano commented Oct 10, 2025

Uh oh!

sspitsyn commented Oct 10, 2025

Uh oh!

pchilano commented Oct 10, 2025

Uh oh!

sspitsyn commented Oct 10, 2025

Uh oh!

xmas92 commented Oct 13, 2025

Uh oh!

sspitsyn left a comment

Choose a reason for hiding this comment

Uh oh!

pchilano commented Oct 13, 2025

Uh oh!

pchilano left a comment

Choose a reason for hiding this comment

Uh oh!

sspitsyn commented Oct 13, 2025

Uh oh!

xmas92 commented Oct 14, 2025

Uh oh!

openjdk bot commented Oct 14, 2025

Uh oh!

openjdk bot commented Oct 14, 2025

Uh oh!

Reviewers

Assignees

Labels

xmas92 commented Oct 9, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Oct 9, 2025 •

edited

Loading

openjdk bot commented Oct 9, 2025 •

edited

Loading

mlbridge bot commented Oct 9, 2025 •

edited

Loading

pchilano commented Oct 9, 2025 •

edited

Loading

sspitsyn commented Oct 10, 2025 •

edited

Loading

sspitsyn commented Oct 10, 2025 •

edited

Loading