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

[David Holmes]

Hi Yasumasa,

On 26/08/2020 5:34 pm, Yasumasa Suenaga wrote:
> Hi Patricio, David,
> 
> Thanks for your comment!
> 
> I updated webrev which includes the fix which is commented by Patricio, 
> and it passed submit repo. So I switch this mail thread to RFR.
> 
>    JBS: https://bugs.openjdk.java.net/browse/JDK-8242427
>    webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8242427/webrev.00/
> 
> I understand David said same concerns as Patricio about active 
> handshaker. This webrev checks active handshaker is current thread or not.

How can the current thread already be in a handshake with the target 
when you execute this code?

David
-----

> 
> Cheers,
> 
> Yasumasa
> 
> 
> On 2020/08/26 10:13, Patricio Chilano wrote:
>> 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
>>