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

[David Holmes]

Hi Yasumasa,

On 27/08/2020 9:40 am, Yasumasa Suenaga wrote:
> Hi David,
> 
> On 2020/08/27 8:09, David Holmes wrote:
>> 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?
> 
> EnterInterpOnlyModeClosure might be called in handshake with 
> UpdateForPopTopFrameClosure or with SetFramePopClosure.
> 
> EnterInterpOnlyModeClosure is introduced in JDK-8238585 as an 
> alternative in VM_EnterInterpOnlyMode.
> VM_EnterInterpOnlyMode returned true in allow_nested_vm_operations(). 
> Originally, it could have been called from other VM operations.

I see. It is a pity that we have now lost that critical indicator that 
shows how this operation can be nested within another operation. The 
possibility of nesting is even more obscure with 
JvmtiEnvThreadState::reset_current_location. And the fact it is now up 
to the caller to handle that case explicitly raises some concern - what 
will happen if you call execute_direct whilst already in a handshake 
with the target thread?

I can't help but feel that we need a more rigorous and automated way of 
dealing with nesting ... perhaps we don't even need to care and 
handshakes should always allow nested handshake requests? (Question more 
for Robbin and Patricio.)

Further comments:

src/hotspot/share/prims/jvmtiEnvThreadState.cpp

  194 #ifdef ASSERT
  195   Thread *current = Thread::current();
  196 #endif
  197   assert(get_thread() == current || current == 
get_thread()->active_handshaker(),
  198          "frame pop data only accessible from same thread or 
direct handshake");

Can you factor this out into a separate function so that it is not 
repeated so often. Seems to me that there should be a global function on 
Thread: assert_current_thread_or_handshaker()  [yes unpleasant name but 
...] that will allow us to stop repeating this code fragment across 
numerous files. A follow up RFE for that would be okay too (I see some 
guarantees that should probably just be asserts so they need a bit more 
checking).

  331         Handshake::execute_direct(&op, _thread);

You aren't checking the return value of execute_direct, but I can't tell 
where _thread was checked for still being alive ??

---

src/hotspot/share/prims/jvmtiEventController.cpp

  340     Handshake::execute_direct(&hs, target);

I know this is existing code but I have the same query as above - no 
return value check and no clear check that the JavaThread is still alive?

---

Do we know if the existing tests actually test the nested cases?

Thanks,
David
-----

> 
> Thanks,
> 
> Yasumasa
> 
> 
>> 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
>>>>