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

[David Holmes]

Hi Serguei,

On 28/02/2014 1:50 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.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!

The basic approach here seems sound.

I find the checking for cur->is_VMThread() somewhat overly conservative 
- if we are at a safepoint, and executing this code, then we must be the 
VMThread. But ok.

You could also use MutexLockerEx to avoid the need for locked and 
unlocked paths to a common call, but that's just stylistic. Though if 
you are grabbing the current thread anyway you can also use the 
MutexLocker calls that take the thread arg - to avoid a second look-up 
of the current thread.

David
-----

> 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
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>