RFR 8193879: Java debugger hangs on method invocation
Alex Menkov
alexey.menkov at oracle.com
Thu Oct 4 22:53:23 UTC 2018
Looks good to me.
--alex
On 10/04/2018 15:13, Daniil Titov wrote:
> Hi Alex,
>
> Just several minutes ago in another email I sent an updated patch on review (please see email attached). The patch has reimplemented tests and no longer uses JdbTest , so the changes you mentioned are no longer there. I also created issue https://bugs.openjdk.java.net/browse/JDK-8211736 to address the missing prompt problem.
>
> Thanks!
> --Daniil
>
> On 10/4/18, 2:53 PM, "Alex Menkov" <alexey.menkov at oracle.com> wrote:
>
>
>
> On 10/04/2018 10:28, Chris Plummer 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?
>
> Did a quick test
> - run debuggee (suspended), connect jdb:
> ===========
> VM Started: No frames on the current call stack
>
> main[1] stop thread at myclass:71
> Deferring breakpoint myclass:71.
> It will be set after the class is loaded.
> main[1] run
> > Set deferred breakpoint myclass:71
>
> Breakpoint hit:
> =====
> I.e. we get only "Breakpoint hit: " without any info about the thread
> (and this doesn't look good), current thread is not set ("next" reports
> "Nothing suspended.")
>
> But if we handle SUSPEND_EVENT_THREAD like SUSPEND_ALL, we need to think
> how to handle possible race conditions when there are 2 threads, each of
> them is stopped by "stop thread at/in".
>
> Some notes about the fix (actually about the tests as the fix itself
> should be implemented):
> JdbStopThreadWithTraceOnTest.java
> 70 jdb.command("stop thread at " + DEBUGGEE_CLASS + ":" + bpLine);
>
> It would be better to introduce
> public static JdbCommand JdbCommand.stopThreadAt(String targetClass, int
> lineNum)
> Actually may be it would be better to drop Jdb.command(String) - it's
> required only by CommandCommentDelimiter (need to update it to use "new
> JdbCommand(...)").
>
> If the fix will require "simple prompt" reply, I'd make it JdbCommand
> property (like JdbCommand.allowExit)
>
> --alex
>
> >
> > 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.java
> >>> 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.java
> >>> 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
> >>>
> >>>
> >>>
> >>
> >
>
>
More information about the serviceability-dev
mailing list