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

[Serguei Spitsyn]

On Fri, 10 Oct 2025 06:00:24 GMT, Serguei Spitsyn <sspitsyn at openjdk.org> wrote:

>> 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?
>
>> 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?

> /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. :)

-------------

PR Comment: https://git.openjdk.org/jdk/pull/27716#issuecomment-3388899792