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

[David Holmes]

On 15/09/2017 9:26 AM, 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/ 

Not sure the new flag was actually needed if the only way to get to the 
commandLoop_exitVmDeathLockOnError in the event helper thread is via 
completeCommand(command) - which is within the locked region. But its 
harmless.

So thumbs up.

Cheers,
David

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