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

[serguei.spitsyn at oracle.com]

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.

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