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

[serguei.spitsyn at oracle.com]

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/


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