3-nd round RFR 6471769: Error: assert(_cur_stack_depth == count_frames(), "cur_stack_depth out of sync")

[serguei.spitsyn at oracle.com]

Please, review the fix for:
   https://bugs.openjdk.java.net/browse/JDK-6471769


Open webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2014/hotspot/6471769-JVMTI-DEPTH.3

Summary:

   It is another attempt to fix the JTREG com/sun/jdi tests regression
   discovered in the first round change.
   The fix is to avoid lock synchronization at 
safepoints(jvmtiEventController.cpp).
   Thanks to Dan for catching the problem in the 2-nd round of review!

Testing:
   All tests are passed: nsk.jvmti, nsk.jdi, nsk.jdwp, JTreg com/sun/jdi


Thanks,
Serguei


On 2/27/14 2:00 PM, serguei.spitsyn at oracle.com wrote:
> On 2/27/14 1:03 PM, serguei.spitsyn at oracle.com wrote:
>> On 2/27/14 12:28 PM, serguei.spitsyn at oracle.com wrote:
>>> Dan,
>>>
>>> Thank you a lot for reviewing this!
>>>
>>> On 2/27/14 11:09 AM, Daniel D. Daugherty wrote:
>>>> On 2/27/14 1:25 AM, serguei.spitsyn at oracle.com wrote:
>>>>> Please, review the fix for:
>>>>>   https://bugs.openjdk.java.net/browse/JDK-6471769
>>>>>
>>>>>
>>>>> Open webrev:
>>>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2014/hotspot/6471769-JVMTI-DEPTH.2 
>>>>>
>>>>
>>>> src/share/vm/runtime/vm_operations.hpp
>>>>     No comments.
>>>>
>>>> src/share/vm/prims/jvmtiEnvBase.hpp
>>>>     No comments.
>>>>
>>>> src/share/vm/prims/jvmtiEnv.cpp
>>>>     No comments.
>>>>
>>>> src/share/vm/prims/jvmtiEnvThreadState.cpp
>>>>     No comments.
>>>>
>>>> src/share/vm/prims/jvmtiEventController.cpp
>>>>     JvmtiEventController::set_frame_pop() is called by
>>>>     JvmtiEnvThreadState::set_frame_pop() which is called by
>>>>     JvmtiEnv::NotifyFramePop().
>>>>
>>>>     The "MutexLocker mu(JvmtiThreadState_lock)" in
>>>>     JvmtiEventController::set_frame_pop() protected the work
>>>>     done by JvmtiEventControllerPrivate::set_frame_pop():
>>>>
>>>>       ets->get_frame_pops()->set(fpop);
>>>> recompute_thread_enabled(ets->get_thread()->jvmti_thread_state());
>>>
>>> Your check is the right thing to do, thanks!
>>> I had to explain this more clearly in this 2-nd review request.
>>>
>>> The approach I've taken here is that all this code paths are executed
>>> on the target thread or at a safepoint.
>>>
>>> It is true for all 3 functions:
>>>   set_frame_pop(), clear_frame_pop() and clear_to_frame_pop().
>>>
>>> And the updated assert guards ensure that it is the case.
>>>
>>> It could be a good idea to add a No_Safepoint_Verifier for 
>>> PopFrame() and NotifyFramePop()
>>> to make sure the current/target thread does not go to safepoint 
>>> until it is returned from
>>> update_for_pop_top_frame() and set_frame_pop() correspondingly.
>>> A No_Safepoint_Verifier can be also needed in the 
>>> JvmtiExport::post_method_exit().
>>>
>>> These are all places where these functions are called:
>>> prims/jvmtiEnv.cpp: 
>>> state->env_thread_state(this)->set_frame_pop(frame_number); // 
>>> JvmtiEnv::NotifyFramePop()
>>> prims/jvmtiExport.cpp: ets->clear_frame_pop(cur_frame_number); // 
>>> JvmtiExport::post_method_exit()
>>> prims/jvmtiThreadState.cpp: 
>>> ets->clear_frame_pop(popframe_number);              // 
>>> JvmtiThreadState::update_for_pop_top_frame()
>>>
>>> The function JvmtiEnvThreadState::clear_to_frame_pop() is never 
>>> called now.
>>
>> There is still a concern about recompute_thread_enabled().
>> If it is normally always protected with the JvmtiThreadState_lock
>> then the approach above is not going to work.
>> I'm trying to check this now.
>
> Dan,
>
> I came to a conclusion that these 3 functions still must be protected
> by the JvmtiThreadState_lock when they are called out of a safepoint.
> It is a little bit ugly but has to be safe though.
>
> Please, let me know if you see eny problems with that.
> I'll send a new webrev soon.
>
> Thanks,
> Serguei
>
>
>>
>> Thanks,
>> Serguei
>>
>>
>>>
>>> Thanks,
>>> Serguei
>>>
>>>
>>>
>>>
>>>
>>>>
>>>>     Since multiple threads can call JVM/TI NotifyFramePop() on the
>>>>     same target thread, what keeps the threads from messing with
>>>>     the list of frame pops simultaneously or messing with the
>>>>     thread enabled events bits in parallel?
>>>>
>>>>     I suspect that this might also be an issue for
>>>>     JvmtiEventController::clear_frame_pop() and
>>>>     JvmtiEventController::clear_to_frame_pop() also.
>>>>
>>>> src/share/vm/prims/jvmtiThreadState.cpp
>>>>     No comments.
>>>>
>>>> Dan
>>>>
>>>>
>>>>>
>>>>> Summary:
>>>>>
>>>>>   It is the 2-nd round of review because the JTREG com/sun/jdi 
>>>>> tests discovered a regression
>>>>>   in the first round change. The issue was in the 
>>>>> JvmtiEventController::clear_frame_pop()
>>>>>   lock synchronization that is not allowed at safepoints.
>>>>>
>>>>>   As a result I've changed the JvmtiEnv::NotifyFramePop to use a 
>>>>> VM operation for safety.
>>>>>   Also, I've removed the lock synchronization from the 3 impacted 
>>>>> JvmtiEventController::
>>>>>   functions: set_frame_pop(), clear_frame_pop() and 
>>>>> clear_to_frame_pop().
>>>>>
>>>>> Testing:
>>>>>   In progress: nsk.jvmti, nsk.jdi, nsk.jdwp, JTreg com/sun/jdi
>>>>>
>>>>>
>>>>> Thanks,
>>>>> Serguei
>>>>>
>>>>>
>>>>> On 2/25/14 12:43 PM, serguei.spitsyn at oracle.com wrote:
>>>>>> Please, review the fix for:
>>>>>>   https://bugs.openjdk.java.net/browse/JDK-6471769
>>>>>>
>>>>>>
>>>>>> Open webrev:
>>>>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2014/hotspot/6471769-JVMTI-DEPTH.1 
>>>>>>
>>>>>>
>>>>>> Summary:
>>>>>>
>>>>>>   This is another Test Stabilization issue.
>>>>>>   The fix is very similar to other JVMTI stabilization fixes.
>>>>>>   It is to use safepoints for updating the PopFrame data instead 
>>>>>> of relying on the
>>>>>>   suspend equivalent condition mechanism 
>>>>>> (JvmtiEnv::is_thread_fully_suspended())
>>>>>>   which is not adequate from the reliability point of view.
>>>>>>
>>>>>> Testing:
>>>>>>   In progress: nsk.jvmti, nsk.jdi, nsk.jdwp, JTreg com/sun/jdi
>>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> Serguei
>>>>>>
>>>>>
>>>>
>>>
>>
>