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

serguei.spitsyn at oracle.com serguei.spitsyn at oracle.com
Fri Feb 28 13:24:47 PST 2014 

On 2/28/14 1:12 PM, Daniel D. Daugherty wrote:
> On 2/28/14 12:55 PM, serguei.spitsyn at oracle.com wrote:
>> On 2/27/14 10:04 PM, David Holmes wrote:
>>> 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 
>>>>
>
> Thumbs up! (including the tweaks in the .4 version)

Thanks a lot, Dan!

>
> src/share/vm/runtime/vm_operations.hpp
>     No comments.
>
> src/share/vm/prims/jvmtiEnvBase.hpp
>     line 375: bool allow_nested_vm_operations() const        { return 
> true; }
>         Does VM_SetFramePop really gave to permit nested VMops?

Yes.
It was a real surprise that it is really necessary.

Thanks!
Serguei

>
> src/share/vm/prims/jvmtiEnv.cpp
>     No comments.
>
> src/share/vm/prims/jvmtiEnvThreadState.cpp
>     No comments.
>
> src/share/vm/prims/jvmtiEventController.cpp
>     No comments.
>
> src/share/vm/prims/jvmtiThreadState.cpp
>     No comments.
>
> Dan
>
>
>
>>>>
>>>>
>>>> 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.
>>
>> Thank you for reviewing the fix!
>>
>>>
>>> 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.
>>
>> Agreed and simplified. Thanks!
>>
>>>
>>> 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.
>>
>> Thank you for reminding. I keep forgetting about it.
>> Will check what is better here, just do not want to rerun the whole 
>> testing.
>> But I'm in favor to make it simpler. :)
>>
>> Thanks,
>> Serguei
>>
>>>
>>> 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
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>
>

More information about the hotspot-dev mailing list