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

Wed Aug 26 15:08:56 UTC 2020

Hi Yasumasa,

Thanks for fixing, seems good.
Note that there are jdk tests for jdi which also runs this code under:
test/jdk/com/sun/jdi/

/Robbin

On 2020-08-26 16:33, Yasumasa Suenaga wrote:
> Hi Robbin,
> 
> I fixed them in new webrev. Could you review again?
> 
>    webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8242427/webrev.02/
>    diff from previous webrev: 
> http://hg.openjdk.java.net/jdk/submit/rev/2ef7feb5681f
> 
> It passed vmTestbase/nsk/{jdi,jdwp,jvmti} serviceability/{jdwp,jvmti} 
> jtreg tests, so I think JVMTI functions works fine includes 
> clear_frame_pop().
> 
> 
> Thanks,
> 
> Yasumasa
> 
> 
> On 2020/08/26 21:32, Robbin Ehn wrote:
>> Hi Yasumasa,
>>
>> Yes that should work.
>>
>> Can you please add assert where you removed the:
>> -  MutexLocker mu(JvmtiThreadState_lock);
>> E.g.
>> +  // If we are in a handshake we only know that the requesting thread 
>> should have locked it.
>> +  assert(SafepointSynchronize::is_at_safepoint() || 
>> JvmtiThreadState_lock->is_locked(), "Safepoint or must be locked");
>>
>> Because I think you missing a MutexLocker in:
>> jvmtiExport.cpp line ~1650:
>>
>>          // remove the frame's entry
>>          ets->clear_frame_pop(cur_frame_number);
>>
>> In the method void JvmtiExport::post_method_exit(...).
>>
>> Thanks, Robbin
>>
>> On 2020-08-26 14:15, Yasumasa Suenaga wrote:
>>> Hi Robbin,
>>>
>>> Thanks for your comment!
>>>
>>> How about this change?
>>>
>>>    webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8242427/webrev.01/
>>>    diff from previous webrev: 
>>> http://hg.openjdk.java.net/jdk/submit/rev/7facd1dd39d6
>>>
>>> I still use JvmtiThreadState_lock because it has a different locking 
>>> range from SR lock.
>>>
>>>
>>> Thanks,
>>>
>>> Yasumasa
>>>
>>>
>>> On 2020/08/26 18:13, Robbin Ehn wrote:
>>>> Hi Yasumasa,
>>>>
>>>> You cannot take the MutexLocker mu(JvmtiThreadState_lock) with 
>>>> safepoint checks inside a handshake.
>>>> We are missing a NoSafepointVerifier for handshakes.
>>>> (I have added this in my work in progress asynchronous handshake patch)
>>>>
>>>> Also this can deadlock with the handshake semaphore.
>>>> (In my asynch handshake patch I have change the sema to a mutex, 
>>>> thus lock ranking works.)
>>>>
>>>> I solved this by just taking the mutex before the handshake.
>>>> And removed the internal locking from set_frame_pop, etc...
>>>> If there is an issue holding the JvmtiThreadState_lock to long, it 
>>>> should split to a per thread lock instead.
>>>> (Since often the thread is suppose to be suspended, one could 
>>>> consider using the SR lock for serializing access to the per thread 
>>>> JvmtiThreadState instead.)
>>>>
>>>> Thanks, Robbin
>>>>
>>>> On 2020-08-26 09:34, 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.
>>>>>
>>>>>
>>>>> 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
>>>>>>