Hi Seguei, On Mon, 2018-10-08 at 17:57 -0700, serguei.spit...@oracle.com wrote: > Hi Severin, > > You already fixed a couple of deadlocks in the debugger method > invocation and have an expertise in this area. > Do you have any time to review the webrev from Daniil?
Sorry, not at the moment. It's also been a while since spelunking in that area. > Otherwise, I'd like to keep you aware about this fix. OK, thanks! I'll try to have a look at it post-push. Any particular reason why the bug is private? I don't seem to be able to view it: https://bugs.openjdk.java.net/browse/JDK-8193879 Thanks, Severin > Thanks, > Serguei > > > On 10/8/18 17:53, serguei.spit...@oracle.com wrote: > > Hi Daniil, > > > > It seems to me, this fix is going to work. > > > > The freeze() method only cares there are no pending resume > > commands: > > 99 synchronized void freeze() { > > 100 if (cache == null && > > (pendingResumeCommands.isEmpty())) { > > 101 /* > > 102 * No pending resumes to worry about. The VM is > > suspended > > 103 * and additional state can be cached. Notify all > > 104 * interested listeners. > > 105 */ > > 106 processVMAction(new VMAction(vm, > > VMAction.VM_SUSPENDED)); > > 107 enableCache(); > > 108 } > > 109 } > > With new version of the notifyCommandComplete: > > 95 void notifyCommandComplete(int id) { > > 96 pendingResumeCommands.remove(id); > > 97 } > > a pending resume command can be deleted from the > > pendingResumeCommands set. > > This does not matter if the collection is already empty. > > > > The only other place for a potential conflict is the method: > > 111 synchronized PacketStream thawCommand(CommandSender > > sender) { > > 112 PacketStream stream = sender.send(); > > 113 pendingResumeCommands.add(stream.id()); > > 114 thaw(); > > 115 return stream; > > 116 } > > However, there is no problem here as the pendingResumeCommands is a > > synchronized set. > > > > > > - return (BreakpointEvent)waitForRequestedEvent(request); > > + return (BreakpointEvent) waitForRequestedEvent(request); > > Not sure why have you added space after the cast. > > We should not have it by coding convention. > > Also, the local style does not have it as well. > > Examples are: > > 761 StepEvent retEvent = > > (StepEvent)waitForRequestedEvent(sr); > > 835 return (Location)locs.get(0); > > 873 return > > (ClassPrepareEvent)waitForRequestedEvent(request); > > > > Otherwise, it looks pretty good to me. > > No need in another webrev if you fix the above. > > > > Thanks, > > Serguei > > > > > > On 10/4/18 14:19, Daniil Titov wrote: > > > Hi Gary and Chris, > > > > > > Please review an updated version of the patch that has newly > > > added test for the case when suspend policy is set to > > > SUSPEND_EVENT_THREAD reimplemented using JDI API. Thus, the > > > changes in > > > src/jdk.jdi/share/classes/com/sun/tools/example/debug/tty/TTY.jav > > > a are no longer required. > > > > > > I think vmInterrupted() is not invoked when suspend policy is set > > > to SUSPEND_EVENT_THREAD to address the case when different > > > threads keep hitting the same breakpoint and to avoid the > > > switching the current thread in the background. > > > > > > The actual behavior of the debugger in the case when the > > > breakpoint is hit and suspend policy is set to > > > SUSPEND_EVENT_THREAD is just to print "Breakpoint hit:" in the > > > output without adding any additional information or new line > > > character. After that you need to set the current thread by > > > entering "thread" command , e.g. "thread 1" and only then jdb > > > prints the prompt (e.g. "main[1]") . The behavior looks as > > > confusing since it is not obvious for the user that some input is > > > expected from him (e.g. to set the current thread). I created a > > > separated issue for that at > > > https://bugs.openjdk.java.net/browse/JDK-8211736 . > > > > > > Webrev: http://cr.openjdk.java.net/~dtitov/8193879/webrev.02/ > > > Issue: https://bugs.openjdk.java.net/browse/JDK-8193879 > > > > > > Thanks, > > > --Daniil > > > > > > On 10/4/18, 10:28 AM, "Chris Plummer" <chris.plum...@oracle.com> > > > wrote: > > > > > > On 10/4/18 5:12 AM, Gary Adams wrote: > > > > In TTY.java do you need to force a simple prompt for the > > > > breakpoint event output? What prevents currentThread from > > > > being set at the time you are printing the prompt? > > > > > > > > > > > > 103 // Print the prompt if suspend policy is > > > > SUSPEND_EVENT_THREAD. In case of > > > > 104 // SUSPEND_ALL policy this is handled by > > > vmInterrupted() > > > > method. > > > > 105 if (be.request().suspendPolicy() == > > > > EventRequest.SUSPEND_EVENT_THREAD) { > > > > 106 MessageOutput.println(); > > > > 107 MessageOutput.printPrompt(); > > > Normally the thread is suspended after the above code is > > > executed: > > > > > > public void run() { > > > EventQueue queue = Env.vm().eventQueue(); > > > while (connected) { > > > try { > > > EventSet eventSet = queue.remove(); > > > boolean resumeStoppedApp = false; > > > EventIterator it = eventSet.eventIterator(); > > > while (it.hasNext()) { > > > resumeStoppedApp |= > > > !handleEvent(it.nextEvent()); > > > <--- calls the modified code above > > > } > > > > > > if (resumeStoppedApp) { > > > eventSet.resume(); > > > } else if (eventSet.suspendPolicy() == > > > EventRequest.SUSPEND_ALL) { > > > setCurrentThread(eventSet); <------ > > > notifier.vmInterrupted(); > > > } > > > > > > However, it only calls setCurrentThread() for SUSPEND_ALL > > > policies. So > > > what Daniil has done here is make it print a simple prompt if > > > the policy > > > is SUSPEND_EVENT_THREAD. It's unclear to me what the actual > > > debugger > > > behavior is in this case. Don't we still suspend and get a > > > prompt from > > > which we can type in the next command? In this case, wouldn't > > > you want a > > > full prompt? Related to that question, why is vmInterrupted() > > > only > > > called if we suspend all threads, and not when we just > > > suspend the > > > thread that the breakpoint came in on? > > > > > > Chris > > > > > > > > > > > > In Jdb.java you allow the waiting for the simple prompt. > > > > I don't see waitForSimplePrompt used in any existing tests. > > > > Since it is only looking for a single character it could > > > > produce false positives if the '>' was produced in the > > > > output stream. Is this wait paired to the added prompt for > > > the > > > > break point event? > > > > > > > > 236 return waitForSimplePrompt ? > > > waitForSimplePrompt(1, > > > > cmd.allowExit) : waitForPrompt(1, cmd.allowExit); > > > > > > > > It might be a good idea to include a test with multiple > > > > threads each with a breakpoint that will trigger > > > SUSPEND_EVENT_THREAD > > > > behavior. > > > > > > > > On 10/4/18, 12:29 AM, Daniil Titov wrote: > > > >> Please review the changes that fix the deadlock in the > > > debugger when > > > >> the debugger is running with the tracing option on. > > > >> > > > >> The problem here is that when the tracing is on the "JDI > > > Target VM > > > >> Interface" thread (the thread that reads all replies and > > > then > > > >> notifies the thread that sent the request that the reply > > > has been > > > >> received) is waiting for a lock which is already acquired > > > by the > > > >> thread that sent the request and is waiting for reply. > > > >> > > > >> The fix itself is in > > > >> src/jdk.jdi/share/classes/com/sun/tools/jdi/VMState.java. > > > >> > > > >> The patch also reverts the changes done in 8129348 > > > "Debugger hangs in > > > >> trace mode with TRACE_SENDS" in > > > >> > > > src/jdk.jdi/share/classes/com/sun/tools/jdi/InvokableTypeImpl.jav > > > a > > > >> since they address only a specific case (VM is suspended > > > and static > > > >> method is invoked) while the proposed fix also solves > > > issue 8129348 > > > >> as well as issue 8193801 "Debugger hangs in trace mode for > > > non-static > > > >> methods". > > > >> > > > >> The changes include new tests for issues 8193879, 8193801 > > > and 8129348. > > > >> > > > >> The changes in > > > >> > > > src/jdk.jdi/share/classes/com/sun/tools/example/debug/tty/TTY.jav > > > a > > > >> solve the problem that the prompt is not printed in the > > > debugger > > > >> output when the breakpoint is hit and the suspend policy > > > is > > > >> SUSPEND_EVENT_THREAD. This is required for new tests to > > > detect that > > > >> command "stop thread at ..." is completed. > > > >> > > > >> Mach5 build and jdk_jdi tests succeeded. > > > >> > > > >> Webrev: > > > http://cr.openjdk.java.net/~dtitov/8193879/webrev.01/ > > > >> Issue: https://bugs.openjdk.java.net/browse/JDK-8193879 > > > >> > > > >> Thanks, > > > >> --Daniil > > > >> > > > >> > > > >> > > > > > > > > > > > > > > > > > > >
