RFR(S) 8177901: JDWP exit error JVMTI_ERROR_WRONG_PHASE(112): on checking for an interface

[serguei.spitsyn at oracle.com]

On 9/14/17 16:40, Daniel D. Daugherty wrote:
> On 9/14/17 5:26 PM, serguei.spitsyn at oracle.com wrote:
>> Hi Dan,
>>
>> Thank you a lot for reviewing!
>> All your suggestions are accepted.
>>
>> The updated webrev:
>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2017/hotspot/8177901-jdi-wrong_phase.2/ 
>>
>
> Re-reviewed all the files. Thumbs up!

Thanks, Dan!
Serguei


>
> Dan
>
>
>>
>>
>> Just a sanity check is needed.
>>
>> Thanks,
>> Serguei
>>
>>
>>
>> On 9/14/17 14:53, Daniel D. Daugherty wrote:
>>> On 9/4/17 11:23 PM, serguei.spitsyn at oracle.com wrote:
>>>> Hi David,
>>>>
>>>>
>>>> On 9/4/17 21:43, David Holmes wrote:
>>>>> Hi Serguei,
>>>>>
>>>>> This looks good to me. One not below.
>>>>>
>>>>> On 2/09/2017 6:30 PM, serguei.spitsyn at oracle.com wrote:
>>>>>> Please, review a fix for:
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8177901
>>>>>>
>>>>>> Webrev:
>>>>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2017/hotspot/8177901-jdi-wrong_phase.1 
>>>>>>
>>>>>
>>>>> src/jdk.jdwp.agent/share/native/libjdwp/debugInit.c
>>>>>
>>>>> +     // Release commandLoop vmDeathLock if necessary
>>>>> +     void commandLoop_exitVmDeathLockOnError(void);
>>>>> +     commandLoop_exitVmDeathLockOnError();
>>>>>
>>>>> The declaration should be in the eventHelper.h header file. It 
>>>>> looks really odd to declare the function then call it.
>>>>
>>>> Ok, I'll move it to the header.
>>>
>>> I did look for a newer webrev and didn't find one.
>>>
>>>
>>> General comments:
>>>   - Please update the copyright years as needed before pushing.
>>>
>>> src/jdk.jdwp.agent/share/native/libjdwp/debugInit.c
>>>     No comments (other than agreeing with David H).
>>>
>>> src/jdk.jdwp.agent/share/native/libjdwp/eventHandler.c
>>>     L1289:     /*
>>>     L1290:      * The VM will die soon after the completion of this 
>>> callback - we
>>>     L1291:      * may need to do a final synchronization with the 
>>> command loop to
>>>     L1292:      * avoid the VM terminating with replying to the 
>>> final (resume)
>>>     L1293:      * command.
>>>     L1294:      */
>>>     L1295:     commandLoop_sync();
>>>
>>>         Seems like your addition of the call to commandLoop_sync()
>>>         might resolve what appears (to me) to be a placeholder
>>>         comment for a future change. Perhaps the comment should be
>>>         something like this:
>>>
>>>         /*
>>>          * The VM will die soon after the completion of this callback -
>>>          * we synchronize with both the command loop and the debug loop
>>>          * for a more orderly shutdown.
>>>          */
>>>
>>> src/jdk.jdwp.agent/share/native/libjdwp/eventHelper.c
>>>     L772: void commandLoop_exitVmDeathLockOnError() {
>>>         The '{' should be on a line by itself to keep in the
>>>         same existing style.
>>>
>>>     L127: static jrawMonitorID vmDeathLock;
>>>     L708:             debugMonitorEnter(vmDeathLock);
>>>     L714:             debugMonitorExit(vmDeathLock);
>>>     L778:     err = JVMTI_FUNC_PTR(gdata->jvmti, GetCurrentThread)
>>>     L779:               (gdata->jvmti, &cur_thread);
>>>     L794:     debugMonitorExit(vmDeathLock);
>>>         L794 assumes that the vmDeathLock is held by the Command
>>>         Loop Thread. Perhaps add a new volatile flag after L127
>>>         that is set to true after L708 and set to false before
>>>         L714. Check the flag before calling GetCurrentThread()
>>>         on L778 and return early if the flag is not true.
>>>
>>> src/jdk.jdwp.agent/share/native/libjdwp/eventHelper.h
>>>     L57: void commandLoop_sync(void); /* commandLoop sync with 
>>> cbVMDeath */
>>>         Extra space before 'cbVMDeath'.
>>>
>>> Dan
>>>
>>>
>>>>
>>>> Thank you a lot for he review!
>>>> Serguei
>>>>
>>>>>
>>>>> Thanks,
>>>>> David
>>>>> -----
>>>>>
>>>>>>
>>>>>> Summary:
>>>>>>
>>>>>>    The approach to fix this is to introduce a new lock 
>>>>>> vmDeathLock in eventHelper.c
>>>>>>    and use it to sync the commandLoop with the JVMTI event 
>>>>>> callback cbVMDeath
>>>>>>    the same way as it was done for debugLoop_run.
>>>>>>    (see the fix of: 
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8134103)
>>>>>>
>>>>>>    One potential issue with it is that the commandLoop() 
>>>>>> transitively uses some helper
>>>>>>    functions (e.g. from util.c) that use the macro EXIT_ERROR, 
>>>>>> and so, can abort.
>>>>>>    It seems, in such a case the vmDeathLock will remain locked, 
>>>>>> and so,
>>>>>>    the cbVMDeath() will block on it causing a deadlock.
>>>>>>    Note, that these helper functions can be also called from 
>>>>>> different contexts/threads
>>>>>>    (not from the commandLoop thread only). In such contexts the 
>>>>>> commandLoop vmDeathLock
>>>>>>    is not being entered, and so, should not be exited.
>>>>>>
>>>>>>    To fix this potential issue, new function, 
>>>>>> commandLoop_exitVmDeathLockOnError(),
>>>>>>    is introduced, and it is called from the debugInit_exit().
>>>>>> The commandLoop_exitVmDeathLockOnError() always checks if current 
>>>>>> thread is
>>>>>>    the commandLoop thread and only in such a case unlocks the 
>>>>>> vmDeathLock.
>>>>>>
>>>>>> Testing:
>>>>>>    Ran the nsk.jdi, nsk.jdwp and jtreg jdk_jdi for both release 
>>>>>> and fastdebug builds.
>>>>>>    All tests are passed.
>>>>>>
>>>>>> Thanks,
>>>>>> Serguei
>>>>>>
>>>>
>>>
>>
>