On Fri, 10 Oct 2025 06:00:24 GMT, Serguei Spitsyn <[email protected]> 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
