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

[Daniel D. Daugherty]

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