2-nd round RFR (S) 8032223: nsk/regression/b4663146 gets assert(SafepointSynchronize::is_at_safepoint() || JvmtiEnv::is_thread_fully_suspended(get_thread(), false, &debug_bits))

[serguei.spitsyn at oracle.com]

On 2/4/14 12:45 PM, serguei.spitsyn at oracle.com wrote:
> Thanks, Dan!
> Some comments below.
>
> On 2/4/14 7:48 AM, Daniel D. Daugherty wrote:
>> On 2/4/14 4:13 AM, serguei.spitsyn at oracle.com wrote:
>>> Please, review the fix for:
>>>   https://bugs.openjdk.java.net/browse/JDK-8032223
>>>
>>>
>>> Open webrev:
>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2014/hotspot/8032223-JVMTI-FRAME.1/ 
>>>
>>
>> src/share/vm/prims/jvmtiEnvBase.hpp
>>     No comments.
>>
>> src/share/vm/prims/jvmtiEnv.cpp
>>     No comments beyond David's tweak to the comment.
>
> Will fix it before push.
>
>> For future work...
>>
>> Looks like these VM ops also need the liveness check on the
>> target JavaThread:
>>
>>     VM_GetOwnedMonitorInfo
>>     VM_GetStackTrace
>>
>> JvmtiEnv::GetStackTrace() looks like it has the same
>> is_thread_fully_suspended() flaw...
>
> I know about this.
> There are even more issues:
>   GetCurrentContendedMonitor
>   GetOwnedMonitorStackDepthInfo
>   NotifyFramePop
>
> I'll check if we can use an existing bug to add this extra request.
> Otherwise, will file new one.

Filed this one:
   https://bugs.openjdk.java.net/browse/JDK-8034249

Thanks,
Serguei

>
> Thanks,
> Serguei
>
>>
>> Dan
>>
>>
>>
>>>
>>> Summary:
>>>
>>>   This is the second round of review for this issue.
>>>   But it was decided that the JDK-8032223 must be used to cover it 
>>> instead of the JDK-6471769.
>>>   The 8032223 was initially closed as a dup of 6471769 but it has 
>>> been re-open now.
>>>
>>>   There is a general issue in the suspend equivalent condition 
>>> mechanism:
>>>   Two subsequent calls to the JvmtiEnv::is_thread_fully_suspended() 
>>> may return different results:
>>>     - 1-st: true
>>>     - 2-nd: false
>>>
>>>   This suspend equivalent issue is covered by another bug:
>>>     https://bugs.openjdk.java.net/browse/JDK-6280037
>>>
>>>   The bug to fix in this review is a specific manifestation of the 
>>> 6280037
>>>   in the JVMTI GetFrameCount() that has a major impact on the SQE 
>>> nightly.
>>>   It is on the Test Stabilization radar as well as the 6280037.
>>>   There are many tests intermittently failing because of this.
>>>   I've also decided to fix the same issue in the JVMTI 
>>> GetFrameLocation() as well.
>>>
>>>   The JVMTI GetFrameCount() spec tells:
>>>     "If this function is called for a thread actively executing 
>>> bytecodes (for example,
>>>      not the current thread and not suspended), the information 
>>> returned is transient."
>>>
>>>   So, it is Ok to call the GetFrameCount() for the non-suspended 
>>> target thread.
>>>   To achieve safety, the frame count for non-suspended threads is 
>>> calculated at a safepoint.
>>>   It should be Ok and more safe to do the same for suspended threads 
>>> as well.
>>>   There should be no big performance impact because it is already on 
>>> a slow path.
>>>   It is still important to avoid safepointing when the target thread 
>>> is current.
>>>
>>>   The bug 6280037 should go out of the Test Stabilization radar 
>>> (remove the svc-nightly label)
>>>   as the most of the impacted tests must be covered by the 8032223.
>>>
>>>
>>> Testing:
>>>   In progress:
>>>     - nsk.jvmti, nsk.jdi, nsk.jdwp
>>>     - JTreg com/sun/jdi
>>>
>>>
>>> Thanks,
>>> Serguei
>>>
>>
>