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 11:55:02 PST 2014 

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 
>>
>>
>>
>> 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 serviceability-dev mailing list