On 9/14/17 9:12 PM, serguei.spit...@oracle.com wrote:
On 9/14/17 19:07, David Holmes wrote:
On 15/09/2017 9:26 AM, serguei.spit...@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.spit...@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.spit...@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
