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

[Daniel D. Daugherty]

On 9/14/17 9:12 PM, serguei.spitsyn at oracle.com wrote:
> On 9/14/17 19:07, David Holmes wrote:
>> 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.
>
> Yes, it is harmless.

Think of it as insurance against someone changing the code in
the future. :-)

Dan


>
>
>>
>> So thumbs up.
>
> Thank you a lot, David!
> Serguei
>
>>
>> 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
>>>>>>>
>>>>>
>>>>
>>>
>