On Fri, 10 Oct 2025 08:39:37 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.
>>
>> 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. :) For consistency with
> `JvmtiExport::continuation_yield_cleanup()`.
> @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);
-------------
PR Comment: https://git.openjdk.org/jdk/pull/27716#issuecomment-3389242915
