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

David Holmes david.holmes at oracle.com
Wed Sep 2 06:29:44 UTC 2020


Hi Serguei,

On 2/09/2020 4:11 pm, serguei.spitsyn at oracle.com wrote:
> Hi Yasumasa,
> 
> It seems to me your update for sync with the JvmtiThreadState_lock is 
> incorrect.
> Let me explain it.
> The original design was that the functions is_frame_pop, set_frame_pop, 
> clear_frame_pop and clear_to_frame_pop are always called either on the 
> current thread or in a VMop.
> There are 3 levels of these functions: in JvmtiEnvThreadState, 
> JvmtiEventController and JvmtiEventControllerPrivate.
> You already found the JvmtiThreadState_lock is grabbed in the 
> JvmtiEventController versions of these functions.
> It is for MT-safety of the recompute_thread_enabled() which can be 
> called not only on current thread and VMop.

Right, but now that we use a handshake, not a VMop, we have no safepoint 
to guarantee MT-safety and so we have to use the lock to ensure that.

David
-----

> So, I think adding MutexLocker's to the jvmtiEnv.cpp and jvmtiExport.cpp 
> is not needed:
> 
> +      MutexLocker mu(JvmtiThreadState_lock);
> +      if (java_thread == JavaThread::current()) {
> +        state->update_for_pop_top_frame();
> +      } else {
> +        UpdateForPopTopFrameClosure op(state);
> 
> . . .
> 
> +  MutexLocker mu(JvmtiThreadState_lock);
>     if (java_thread == JavaThread::current()) {
>       int frame_number = state->count_frames() - depth;
>       state->env_thread_state(this)->set_frame_pop(frame_number);
>     } else {
> -    VM_SetFramePop op(this, state, depth);
> -    VMThread::execute(&op);
> -    err = op.result();
> +    SetFramePopClosure op(this, state, depth);
> +    bool executed = Handshake::execute_direct(&op, java_thread);
> +    err = executed ? op.result() : JVMTI_ERROR_THREAD_NOT_ALIVE;
> 
> . . .
> 
> + MutexLocker mu(JvmtiThreadState_lock);
>           ets->clear_frame_pop(cur_frame_number);
> 
> 
> Instead, they have to be restored in the JvmtiEventController functions:
> 
>   void
>   JvmtiEventController::set_frame_pop(JvmtiEnvThreadState *ets, JvmtiFramePop fpop) {
> -  MutexLocker mu(SafepointSynchronize::is_at_safepoint() ? NULL : JvmtiThreadState_lock);
> +  assert_lock_strong(JvmtiThreadState_lock);
>     JvmtiEventControllerPrivate::set_frame_pop(ets, fpop);
>   }
>   
>   
>   void
>   JvmtiEventController::clear_frame_pop(JvmtiEnvThreadState *ets, JvmtiFramePop fpop) {
> -  MutexLocker mu(SafepointSynchronize::is_at_safepoint() ? NULL : JvmtiThreadState_lock);
> +  assert_lock_strong(JvmtiThreadState_lock);
>     JvmtiEventControllerPrivate::clear_frame_pop(ets, fpop);
>   }
>   
>   
>   void
>   JvmtiEventController::clear_to_frame_pop(JvmtiEnvThreadState *ets, JvmtiFramePop fpop) {
> -  MutexLocker mu(SafepointSynchronize::is_at_safepoint() ? NULL : JvmtiThreadState_lock);
> +  assert_lock_strong(JvmtiThreadState_lock);
>     JvmtiEventControllerPrivate::clear_to_frame_pop(ets, fpop);
>   }
> 
> 
> Thanks,
> Serguei
> 
> 
> On 9/1/20 21:34, serguei.spitsyn at oracle.com wrote:
>> Hi Yasumasa,
>>
>>
>> On 9/1/20 21:17, Yasumasa Suenaga wrote:
>>> Hi David,
>>>
>>> On 2020/09/02 13:13, David Holmes wrote:
>>>> Hi Yasumasa,
>>>>
>>>> On 31/08/2020 7:10 pm, Yasumasa Suenaga wrote:
>>>>> Hi David,
>>>>>
>>>>> I uploaded new webrev. Could you review again?
>>>>>
>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8242427/webrev.04/
>>>>>
>>>>> This webrev includes two changes:
>>>>>
>>>>>    1. Use assert_lock_strong() for JvmtiThreadState_lock
>>>>> http://hg.openjdk.java.net/jdk/submit/rev/c85f93d2042d
>>>>>
>>>>>    2. Check return value from execute_direct() with assert()
>>>>> http://hg.openjdk.java.net/jdk/submit/rev/8746e1651343
>>>>
>>>> The message for the assertion:
>>>>
>>>> assert(executed, "Direct handshake failed. Target thread is still 
>>>> alive?");
>>>>
>>>> should be phrased:
>>>>
>>>> assert(executed, "Direct handshake failed. Target thread is not 
>>>> alive?");
>>>>
>>>> otherwise it sounds like the expectation is that it should not be 
>>>> alive.
>>>>
>>>> Other changes fine.
>>>>
>>>> No need to see updated webrev.
>>>
>>> Thanks for your review!
>>> I will fix them before pushing.
>>
>> Please, hold on.
>> I'm still reviewing this.
>> It is not clear yet if sync with the JvmtiThreadState_lock is fully 
>> correct.
>>
>> Thanks,
>> Serguei
>>
>>>
>>>
>>> Yasumasa
>>>
>>>
>>>> Thanks,
>>>> David
>>>> -----
>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Yasumasa
>>>>>
>>>>>
>>>>> On 2020/08/31 15:22, Yasumasa Suenaga wrote:
>>>>>> Hi David,
>>>>>>
>>>>>> On 2020/08/31 14:43, David Holmes wrote:
>>>>>>> Hi Yasumasa,
>>>>>>>
>>>>>>> On 28/08/2020 1:01 pm, Yasumasa Suenaga wrote:
>>>>>>>> Hi David,
>>>>>>>>
>>>>>>>> On 2020/08/28 11:04, David Holmes wrote:
>>>>>>>>> Hi Yasumasa,
>>>>>>>>>
>>>>>>>>> On 28/08/2020 11:24 am, Yasumasa Suenaga wrote:
>>>>>>>>>> Hi David,
>>>>>>>>>>
>>>>>>>>>> On 2020/08/27 15:49, David Holmes wrote:
>>>>>>>>>>> Sorry I just realized I reviewed version 00 :(
>>>>>>>>>
>>>>>>>>> Note that my comments on version 00 in my earlier email still 
>>>>>>>>> apply.
>>>>>>>>
>>>>>>>> I copied here your comment on webrev.00:
>>>>>>>>
>>>>>>>>>>>> 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 heard deadlock would be happen if execute_direct() calls in 
>>>>>>>> direct handshake. Thus we need to use active_handshaker() in 
>>>>>>>> this change.
>>>>>>>
>>>>>>> Okay. This is something we need to clarify with direct handshake 
>>>>>>> usage information. I think it would be preferable if this was 
>>>>>>> handled in execute_direct rather than the caller ... though it 
>>>>>>> may also be the case that we need the writer of the handshake 
>>>>>>> operation to give due consideration to nesting ...
>>>>>>
>>>>>> Agree, I also prefer to check whether caller is in direct 
>>>>>> handshake in execute_direct().
>>>>>> But I think this is another enhancement because we need to change 
>>>>>> the behavior of execute_direct().
>>>>>>
>>>>>>
>>>>>>>>>>>> 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).
>>>>>>>>
>>>>>>>> I filed it as another RFE:
>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8252479
>>>>>>>
>>>>>>> Thanks.
>>>>>>>
>>>>>>>>
>>>>>>>>>>>>   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?
>>>>>>>>
>>>>>>>> Existing code seems to assume that target thread is alive, frame 
>>>>>>>> operations (e.g. PopFrame()) should be performed on live thread. 
>>>>>>>> And also existing code would not set any JVMTI error and cannot 
>>>>>>>> propagate it to caller. So I do not add the check for thread state.
>>>>>>>
>>>>>>> Okay. But note that for PopFrame the tests for isAlive and 
>>>>>>> is-suspended have already been performed before we do the 
>>>>>>> execute_direct; so in that case we could simply assert that 
>>>>>>> execute_direct returns true. Similarly for other cases.
>>>>>>
>>>>>> Ok, I will change as following in next webrev:
>>>>>>
>>>>>> ```
>>>>>> bool result = Handshake::execute_direct(&hs, target);
>>>>>> guarantee(result, "Direct handshake failed. Target thread is still 
>>>>>> alive?");
>>>>>> ```
>>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> Yasumasa
>>>>>>
>>>>>>
>>>>>>>>>>>> Do we know if the existing tests actually test the nested 
>>>>>>>>>>>> cases?
>>>>>>>>
>>>>>>>> I saw some error with assertion for JvmtiThreadState_lock and 
>>>>>>>> safepoint in vmTestbase at first, so I guess nested call would 
>>>>>>>> be tested, but I'm not sure.
>>>>>>>>
>>>>>>>>
>>>>>>>>>>> I have concerns with the added locking:
>>>>>>>>>>>
>>>>>>>>>>> MutexLocker mu(JvmtiThreadState_lock);
>>>>>>>>>>>
>>>>>>>>>>> Who else may be holding that lock? Could it be our target 
>>>>>>>>>>> thread that we have already initiated a handshake with? (The 
>>>>>>>>>>> lock ranking checks related to safepoints don't help us 
>>>>>>>>>>> detect deadlocks between a target thread and its handshaker. 
>>>>>>>>>>> :( )
>>>>>>>>>>
>>>>>>>>>> I checked source code again, then I couldn't find the point 
>>>>>>>>>> that target thread already locked JvmtiThreadState_lock at 
>>>>>>>>>> direct handshake.
>>>>>>>>>
>>>>>>>>> I'm very unclear exactly what state this lock guards and under 
>>>>>>>>> what conditions. But looking at:
>>>>>>>>>
>>>>>>>>> src/hotspot/share/prims/jvmtiEnv.cpp
>>>>>>>>>
>>>>>>>>> Surely the lock is only needed in the direct-handshake case and 
>>>>>>>>> not when operating on the current thread? Or is it there 
>>>>>>>>> because you've removed the locking from the lower-level 
>>>>>>>>> JvmtiEventController methods and so now you need to take the 
>>>>>>>>> lock higher-up the call chain? (I find it hard to follow the 
>>>>>>>>> call chains in the JVMTI code.)
>>>>>>>>
>>>>>>>> We need to take the lock higher-up the call chain. It is 
>>>>>>>> suggested by Robbin, and works fine.
>>>>>>>
>>>>>>> Okay. It seems reasonably safe in this context as there is little 
>>>>>>> additional work done while holding the lock.
>>>>>>>
>>>>>>>>
>>>>>>>>>>> It is far from clear now which functions are reachable from 
>>>>>>>>>>> handshakes, which from safepoint VM_ops and which from both.
>>>>>>>>>>>
>>>>>>>>>>> ! assert(SafepointSynchronize::is_at_safepoint() || 
>>>>>>>>>>> JvmtiThreadState_lock->is_locked(), "Safepoint or must be 
>>>>>>>>>>> locked");
>>>>>>>>>>>
>>>>>>>>>>> This can be written as:
>>>>>>>>>>>
>>>>>>>>>>> assert_locked_or_safepoint(JvmtiThreadState_lock);
>>>>>>>>>>>
>>>>>>>>>>> or possibly the weak variant of that. ('m puzzled by the 
>>>>>>>>>>> extra check in the strong version ... I think it is intended 
>>>>>>>>>>> for the case of the VMThread executing a non-safepoint VMop.)
>>>>>>>>>>
>>>>>>>>>>> JvmtiEventController::set_frame_pop(), 
>>>>>>>>>>> JvmtiEventController::clear_frame_pop() and 
>>>>>>>>>>> JvmtiEventController::clear_to_frame_pop() are no longer 
>>>>>>>>>>> called at safepoint, so I remove safepoint check from 
>>>>>>>>>>> assert() in new webrev.
>>>>>>>>>
>>>>>>>>> You should use assert_lock_strong for this.
>>>>>>>>
>>>>>>>> I will do that.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> David
>>>>>>> -----
>>>>>>>
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>>
>>>>>>>> Yasumasa
>>>>>>>>
>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> David
>>>>>>>>>
>>>>>>>>>>    webrev: 
>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8242427/webrev.03/
>>>>>>>>>>      diff from previous webrev: 
>>>>>>>>>> http://hg.openjdk.java.net/jdk/submit/rev/2a2c02ada080
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>>
>>>>>>>>>> Yasumasa
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> Thanks,
>>>>>>>>>>> David
>>>>>>>>>>> -----
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On 27/08/2020 4:34 pm, David Holmes wrote:
>>>>>>>>>>>> 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
>>>>>>>>>>>>>>>>
>>
> 


More information about the serviceability-dev mailing list