[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR(S) 8177901: JDWP exit error JVMTI_ERROR_WRONG_PHASE(112): on checking for an interface
From: "serguei.spitsyn () oracle ! com" <serguei ! spitsyn () oracle ! com>
Date: 2017-09-15 6:52:39
Message-ID: b0f00d0c-ea7e-9a75-cadd-ab1c651b9974 () oracle ! com
[Download RAW message or body]
On 9/14/17 20:28, Daniel D. Daugherty wrote:
> On 9/14/17 9:12 PM, serguei.spitsyn@oracle.com wrote:
>> On 9/14/17 19:07, David Holmes wrote:
>>> On 15/09/2017 9:26 AM, serguei.spitsyn@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. :-)
Yes, it is the point. :)
Thanks,
Serguei
>
> 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@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@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
>>>>>>>>
>>>>>>
>>>>>
>>>>
>>
>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic