8242427: JVMTI frame pop operations should use Thread-Local Handshakes

[Patricio Chilano]

Hi Yasumasa,

On 8/23/20 11:40 PM, Yasumasa Suenaga wrote:
> Hi all,
>
> I want to hear your opinions about the change for JDK-8242427.
>
> I'm trying to migrate following operations to direct handshake.
>
>     - VM_UpdateForPopTopFrame
>     - VM_SetFramePop
>     - VM_GetCurrentLocation
>
> Some operations (VM_GetCurrentLocation and EnterInterpOnlyModeClosure) 
> might be called at safepoint, so I want to use 
> JavaThread::active_handshaker() in production VM to detect the process 
> is in direct handshake or not.
>
> However this function is available in debug VM only, so I want to hear 
> the reason why it is for debug VM only, and there are no problem to 
> use it in production VM. Of course another solutions are welcome.
I added the _active_handshaker field to the HandshakeState class when 
working on 8230594 to adjust some asserts, where instead of checking for 
the VMThread we needed to check for the active handshaker of the target 
JavaThread. Since there were no other users of it, there was no point in 
declaring it and having to write to it for the release bits. There are 
no issues with having it in production though so you could change that 
if necessary.

> webrev is here. It passed jtreg tests (vmTestbase/nsk/{jdi,jdwp,jvmti} 
> serviceability/{jdwp,jvmti})
>   http://cr.openjdk.java.net/~ysuenaga/JDK-8242427/proposal/
Some comments on the proposed change.

src/hotspot/share/prims/jvmtiEnvThreadState.cpp, 
src/hotspot/share/prims/jvmtiEventController.cpp
Why is the check to decide whether to call the handshake or execute the 
operation with the current thread different for 
GetCurrentLocationClosure vs EnterInterpOnlyModeClosure?

(GetCurrentLocationClosure)
if ((Thread::current() == _thread) || (_thread->active_handshaker() != 
NULL)) {
      op.do_thread(_thread);
} else {
      Handshake::execute_direct(&op, _thread);
}

vs

(EnterInterpOnlyModeClosure)
if (target->active_handshaker() != NULL) {
     hs.do_thread(target);
} else {
     Handshake::execute_direct(&hs, target);
}

If you change VM_SetFramePop to use handshakes then it seems you could 
reach JvmtiEventControllerPrivate::enter_interp_only_mode() with the 
current thread being the target.
Also I think you want the second expression of that check to be 
(target->active_handshaker() == Thread::current()). So either you are 
the target or the current active_handshaker for that target. Otherwise 
active_handshaker() could be not NULL because there is another 
JavaThread handshaking the same target. Unless you are certain that it 
can never happen, so if active_handshaker() is not NULL it is always the 
current thread, but even in that case this way is safer.

src/hotspot/share/prims/jvmtiThreadState.cpp
The guarantee() statement exists in release builds too so the "#ifdef 
ASSERT" directive should be removed, otherwise "current" will not be 
declared.

Thanks!

Patricio
> Thanks,
>
> Yasumasa