<html xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40"><head><meta http-equiv=Content-Type content="text/html; charset=utf-8"><meta name=Generator content="Microsoft Word 15 (filtered medium)"><style><!--
/* Font Definitions */
@font-face
        {font-family:"Cambria Math";
        panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
        {font-family:Calibri;
        panose-1:2 15 5 2 2 2 4 3 2 4;}
@font-face
        {font-family:Consolas;
        panose-1:2 11 6 9 2 2 4 3 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
        {margin:0in;
        margin-bottom:.0001pt;
        font-size:11.0pt;
        font-family:"Calibri",sans-serif;}
a:link, span.MsoHyperlink
        {mso-style-priority:99;
        color:blue;
        text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
        {mso-style-priority:99;
        color:purple;
        text-decoration:underline;}
pre
        {mso-style-priority:99;
        mso-style-link:"HTML Preformatted Char";
        margin:0in;
        margin-bottom:.0001pt;
        font-size:10.0pt;
        font-family:"Courier New";}
p.msonormal0, li.msonormal0, div.msonormal0
        {mso-style-name:msonormal;
        mso-margin-top-alt:auto;
        margin-right:0in;
        mso-margin-bottom-alt:auto;
        margin-left:0in;
        font-size:11.0pt;
        font-family:"Calibri",sans-serif;}
span.HTMLPreformattedChar
        {mso-style-name:"HTML Preformatted Char";
        mso-style-priority:99;
        mso-style-link:"HTML Preformatted";
        font-family:Consolas;}
span.removed
        {mso-style-name:removed;}
span.new
        {mso-style-name:new;}
span.changed
        {mso-style-name:changed;}
span.EmailStyle23
        {mso-style-type:personal-reply;
        font-family:"Calibri",sans-serif;}
.MsoChpDefault
        {mso-style-type:export-only;
        font-size:10.0pt;}
@page WordSection1
        {size:8.5in 11.0in;
        margin:1.0in 1.0in 1.0in 1.0in;}
div.WordSection1
        {page:WordSection1;}
--></style></head><body lang=EN-US link=blue vlink=purple><div class=WordSection1><p class=MsoNormal>Hi Serguei,<o:p></o:p></p><p class=MsoNormal><o:p> </o:p></p><p class=MsoNormal>Sure. That was my intention.<o:p></o:p></p><p class=MsoNormal><o:p> </o:p></p><p class=MsoNormal>Thanks!<o:p></o:p></p><p class=MsoNormal><o:p> </o:p></p><p class=MsoNormal>Bets regards,<o:p></o:p></p><p class=MsoNormal>Daniil<o:p></o:p></p><p class=MsoNormal><o:p> </o:p></p><p class=MsoNormal><o:p> </o:p></p><div style='border:none;border-top:solid #B5C4DF 1.0pt;padding:3.0pt 0in 0in 0in'><p class=MsoNormal><b><span style='font-size:12.0pt;color:black'>From: </span></b><span style='font-size:12.0pt;color:black'>"serguei.spitsyn@oracle.com" <serguei.spitsyn@oracle.com><br><b>Date: </b>Monday, October 8, 2018 at 6:17 PM<br><b>To: </b>Daniil Titov <daniil.x.titov@oracle.com>, <gary.adams@oracle.com>, Alex Menkov <alexey.menkov@oracle.com>, Chris Plummer <chris.plummer@oracle.com><br><b>Cc: </b>serviceability-dev <serviceability-dev@openjdk.java.net><br><b>Subject: </b>Re: RFR 8193879: Java debugger hangs on method invocation<o:p></o:p></span></p></div><div><p class=MsoNormal><o:p> </o:p></p></div><div><p class=MsoNormal>Daniil,<br><br>Could you wait a little bit for reply from Severin G.?<br><br>Thanks,<br>Serguei<br><br>On 10/8/18 18:12, Daniil Titov wrote:<o:p></o:p></p></div><blockquote style='margin-top:5.0pt;margin-bottom:5.0pt'><div><p class=MsoNormal style='mso-margin-top-alt:auto;mso-margin-bottom-alt:auto'>Hi Serguei, Chris, Alex, and Garry,<o:p></o:p></p><p class=MsoNormal style='mso-margin-top-alt:auto;mso-margin-bottom-alt:auto'> <o:p></o:p></p><p class=MsoNormal style='mso-margin-top-alt:auto;mso-margin-bottom-alt:auto'>Thank you for reviewing this change. I will remove added space in test/jdk/com/sun/jdi/TestScaffold.java  resumeTo() method before pushing the change. It was accidentally added. <o:p></o:p></p><p class=MsoNormal style='mso-margin-top-alt:auto;mso-margin-bottom-alt:auto'> <o:p></o:p></p><pre><span class=removed>-        return (BreakpointEvent)waitForRequestedEvent(request);</span><o:p></o:p></pre><pre><span class=new>+        return (BreakpointEvent) waitForRequestedEvent(request);</span><o:p></o:p></pre><p class=MsoNormal style='mso-margin-top-alt:auto;mso-margin-bottom-alt:auto'> <o:p></o:p></p><p class=MsoNormal style='mso-margin-top-alt:auto;mso-margin-bottom-alt:auto'>Best regards,<o:p></o:p></p><p class=MsoNormal style='mso-margin-top-alt:auto;mso-margin-bottom-alt:auto'>Daniil<o:p></o:p></p><p class=MsoNormal style='mso-margin-top-alt:auto;mso-margin-bottom-alt:auto'> <o:p></o:p></p><div><p class=MsoNormal style='mso-margin-top-alt:auto;mso-margin-bottom-alt:auto'><b>From: </b><a href="mailto:serguei.spitsyn@oracle.com">"serguei.spitsyn@oracle.com"</a> <a href="mailto:serguei.spitsyn@oracle.com"><serguei.spitsyn@oracle.com></a><br><b>Date: </b>Monday, October 8, 2018 at 5:53 PM<br><b>To: </b>Daniil Titov <a href="mailto:daniil.x.titov@oracle.com"><daniil.x.titov@oracle.com></a>, Chris Plummer <a href="mailto:chris.plummer@oracle.com"><chris.plummer@oracle.com></a>, <a href="mailto:gary.adams@oracle.com"><gary.adams@oracle.com></a><br><b>Cc: </b>serviceability-dev <a href="mailto:serviceability-dev@openjdk.java.net"><serviceability-dev@openjdk.java.net></a><br><b>Subject: </b>Re: RFR 8193879: Java debugger hangs on method invocation<o:p></o:p></p></div><div><p class=MsoNormal style='mso-margin-top-alt:auto;mso-margin-bottom-alt:auto'> <o:p></o:p></p></div><div><p class=MsoNormal style='mso-margin-top-alt:auto;mso-margin-bottom-alt:auto'>Hi Daniil,<br><br>It seems to me, this fix is going to work.<br><br>The freeze() method only cares there are no pending resume commands:<o:p></o:p></p><pre>  99     synchronized void freeze() {<o:p></o:p></pre><pre><span class=changed> 100         if (cache == null && (pendingResumeCommands.isEmpty())) {</span><o:p></o:p></pre><pre> 101             /*<o:p></o:p></pre><pre> 102              * No pending resumes to worry about. The VM is suspended<o:p></o:p></pre><pre> 103              * and additional state can be cached. Notify all<o:p></o:p></pre><pre> 104              * interested listeners.<o:p></o:p></pre><pre> 105              */<o:p></o:p></pre><pre> 106             processVMAction(new VMAction(vm, VMAction.VM_SUSPENDED));<o:p></o:p></pre><pre> 107             enableCache();<o:p></o:p></pre><pre> 108         }<o:p></o:p></pre><pre> 109     }<o:p></o:p></pre><p class=MsoNormal style='mso-margin-top-alt:auto;mso-margin-bottom-alt:auto'>With new version of the notifyCommandComplete:<o:p></o:p></p><pre><span class=changed>  95     void notifyCommandComplete(int id) {</span><o:p></o:p></pre><pre><span class=changed>  96         pendingResumeCommands.remove(id);</span><o:p></o:p></pre><pre>  97     }<o:p></o:p></pre><p class=MsoNormal style='mso-margin-top-alt:auto;mso-margin-bottom-alt:auto'>a pending resume command can be deleted from the <span class=changed>pendingResumeCommands set.</span><br><span class=changed>This does not matter if the collection is already empty.</span><br><br><span class=changed>The only other place for a potential conflict is the method:</span><o:p></o:p></p><pre> 111     synchronized PacketStream thawCommand(CommandSender sender) {<o:p></o:p></pre><pre> 112         PacketStream stream = sender.send();<o:p></o:p></pre><pre><span class=changed> 113         pendingResumeCommands.add(stream.id());</span><o:p></o:p></pre><pre> 114         thaw();<o:p></o:p></pre><pre> 115         return stream;<o:p></o:p></pre><pre> 116     }<o:p></o:p></pre><p class=MsoNormal style='mso-margin-top-alt:auto;margin-bottom:12.0pt'>However, there is no problem here as the <span class=changed>pendingResumeCommands is a synchronized set.</span><o:p></o:p></p><pre><span class=removed>-        return (BreakpointEvent)waitForRequestedEvent(request);</span><o:p></o:p></pre><pre><span class=new>+        return (BreakpointEvent) waitForRequestedEvent(request);</span><o:p></o:p></pre><p class=MsoNormal style='mso-margin-top-alt:auto;mso-margin-bottom-alt:auto'> Not sure why have you added space after the cast.<br> We should not have it by coding convention.<br> Also, the local style does not have it as well.<br> Examples are:<o:p></o:p></p><pre> 761         StepEvent retEvent = (StepEvent)waitForRequestedEvent(sr);<o:p></o:p></pre><pre> 835         return (Location)locs.get(0);<o:p></o:p></pre><pre> 873         return (ClassPrepareEvent)waitForRequestedEvent(request);<o:p></o:p></pre><pre> <o:p></o:p></pre><pre>Otherwise, it looks pretty good to me.<o:p></o:p></pre><pre>No need in another webrev if you fix the above.<o:p></o:p></pre><p class=MsoNormal style='mso-margin-top-alt:auto;mso-margin-bottom-alt:auto'><br>Thanks,<br>Serguei<br><br><br>On 10/4/18 14:19, Daniil Titov wrote:<o:p></o:p></p></div><blockquote style='margin-top:5.0pt;margin-bottom:5.0pt'><pre>Hi Gary and Chris,<o:p></o:p></pre><pre> <o:p></o:p></pre><pre>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.java are no longer required.<o:p></o:p></pre><pre> <o:p></o:p></pre><pre>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.<o:p></o:p></pre><pre> <o:p></o:p></pre><pre>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 <a href="https://bugs.openjdk.java.net/browse/JDK-8211736">https://bugs.openjdk.java.net/browse/JDK-8211736</a> .<o:p></o:p></pre><pre> <o:p></o:p></pre><pre>Webrev: <a href="http://cr.openjdk.java.net/%7Edtitov/8193879/webrev.02/">http://cr.openjdk.java.net/~dtitov/8193879/webrev.02/</a><o:p></o:p></pre><pre>Issue: <a href="https://bugs.openjdk.java.net/browse/JDK-8193879">https://bugs.openjdk.java.net/browse/JDK-8193879</a><o:p></o:p></pre><pre> <o:p></o:p></pre><pre>Thanks,<o:p></o:p></pre><pre>--Daniil<o:p></o:p></pre><pre> <o:p></o:p></pre><pre>´╗┐On 10/4/18, 10:28 AM, "Chris Plummer" <a href="mailto:chris.plummer@oracle.com"><chris.plummer@oracle.com></a> wrote:<o:p></o:p></pre><pre> <o:p></o:p></pre><pre>    On 10/4/18 5:12 AM, Gary Adams wrote:<o:p></o:p></pre><pre>    > In TTY.java do you need to force a simple prompt for the<o:p></o:p></pre><pre>    > breakpoint event output? What prevents currentThread from<o:p></o:p></pre><pre>    > being set at the time you are printing the prompt?<o:p></o:p></pre><pre>    ><o:p></o:p></pre><pre>    ><o:p></o:p></pre><pre>    >  103         // Print the prompt if suspend policy is <o:p></o:p></pre><pre>    > SUSPEND_EVENT_THREAD. In case of<o:p></o:p></pre><pre>    >  104         // SUSPEND_ALL policy this is handled by vmInterrupted() <o:p></o:p></pre><pre>    > method.<o:p></o:p></pre><pre>    >  105         if (be.request().suspendPolicy() == <o:p></o:p></pre><pre>    > EventRequest.SUSPEND_EVENT_THREAD) {<o:p></o:p></pre><pre>    >  106             MessageOutput.println();<o:p></o:p></pre><pre>    >  107             MessageOutput.printPrompt();<o:p></o:p></pre><pre>    Normally the thread is suspended after the above code is executed:<o:p></o:p></pre><pre>    <o:p></o:p></pre><pre>         public void run() {<o:p></o:p></pre><pre>             EventQueue queue = Env.vm().eventQueue();<o:p></o:p></pre><pre>             while (connected) {<o:p></o:p></pre><pre>                 try {<o:p></o:p></pre><pre>                     EventSet eventSet = queue.remove();<o:p></o:p></pre><pre>                     boolean resumeStoppedApp = false;<o:p></o:p></pre><pre>                     EventIterator it = eventSet.eventIterator();<o:p></o:p></pre><pre>                     while (it.hasNext()) {<o:p></o:p></pre><pre>                         resumeStoppedApp |= !handleEvent(it.nextEvent()); <o:p></o:p></pre><pre>    <--- calls the modified code above<o:p></o:p></pre><pre>                     }<o:p></o:p></pre><pre>    <o:p></o:p></pre><pre>                     if (resumeStoppedApp) {<o:p></o:p></pre><pre>                         eventSet.resume();<o:p></o:p></pre><pre>                     } else if (eventSet.suspendPolicy() == <o:p></o:p></pre><pre>    EventRequest.SUSPEND_ALL) {<o:p></o:p></pre><pre>                         setCurrentThread(eventSet);   <------<o:p></o:p></pre><pre>                         notifier.vmInterrupted();<o:p></o:p></pre><pre>                     }<o:p></o:p></pre><pre>    <o:p></o:p></pre><pre>    However, it only calls setCurrentThread() for SUSPEND_ALL policies. So <o:p></o:p></pre><pre>    what Daniil has done here is make it print a simple prompt if the policy <o:p></o:p></pre><pre>    is SUSPEND_EVENT_THREAD. It's unclear to me what the actual debugger <o:p></o:p></pre><pre>    behavior is in this case. Don't we still suspend and get a prompt from <o:p></o:p></pre><pre>    which we can type in the next command? In this case, wouldn't you want a <o:p></o:p></pre><pre>    full prompt? Related to that question, why is vmInterrupted() only <o:p></o:p></pre><pre>    called if we suspend all threads, and not when we just suspend the <o:p></o:p></pre><pre>    thread that the breakpoint came in on?<o:p></o:p></pre><pre>    <o:p></o:p></pre><pre>    Chris<o:p></o:p></pre><pre>    ><o:p></o:p></pre><pre>    ><o:p></o:p></pre><pre>    > In Jdb.java you allow the waiting for the simple prompt.<o:p></o:p></pre><pre>    > I don't see waitForSimplePrompt used in any existing tests.<o:p></o:p></pre><pre>    > Since it is only looking for a single character it could<o:p></o:p></pre><pre>    > produce false positives if the '>' was produced in the<o:p></o:p></pre><pre>    > output stream. Is this wait paired to the added prompt for the<o:p></o:p></pre><pre>    > break point event?<o:p></o:p></pre><pre>    ><o:p></o:p></pre><pre>    >  236         return waitForSimplePrompt ? waitForSimplePrompt(1, <o:p></o:p></pre><pre>    > cmd.allowExit) : waitForPrompt(1, cmd.allowExit);<o:p></o:p></pre><pre>    ><o:p></o:p></pre><pre>    > It might be a good idea to include a test with multiple<o:p></o:p></pre><pre>    > threads each with a breakpoint that will trigger SUSPEND_EVENT_THREAD<o:p></o:p></pre><pre>    > behavior.<o:p></o:p></pre><pre>    ><o:p></o:p></pre><pre>    > On 10/4/18, 12:29 AM, Daniil Titov wrote:<o:p></o:p></pre><pre>    >> Please review the changes that fix the deadlock in the debugger when <o:p></o:p></pre><pre>    >> the debugger is running with the tracing option on.<o:p></o:p></pre><pre>    >><o:p></o:p></pre><pre>    >> The problem here is that when the tracing is on the "JDI Target VM <o:p></o:p></pre><pre>    >> Interface" thread (the thread that reads all replies and then <o:p></o:p></pre><pre>    >> notifies the thread that sent the request that the reply has been<o:p></o:p></pre><pre>    >> received) is waiting for a lock which is already acquired by the <o:p></o:p></pre><pre>    >> thread that sent the request and is waiting for reply.<o:p></o:p></pre><pre>    >><o:p></o:p></pre><pre>    >> The fix itself is in <o:p></o:p></pre><pre>    >> src/jdk.jdi/share/classes/com/sun/tools/jdi/VMState.java.<o:p></o:p></pre><pre>    >><o:p></o:p></pre><pre>    >> The patch also reverts the changes done in 8129348 "Debugger hangs in <o:p></o:p></pre><pre>    >> trace mode with TRACE_SENDS" in <o:p></o:p></pre><pre>    >> src/jdk.jdi/share/classes/com/sun/tools/jdi/InvokableTypeImpl.java <o:p></o:p></pre><pre>    >> since they address only a specific case (VM is suspended and static <o:p></o:p></pre><pre>    >> method is invoked) while the proposed fix also solves issue 8129348 <o:p></o:p></pre><pre>    >> as well as issue 8193801 "Debugger hangs in trace mode for non-static <o:p></o:p></pre><pre>    >> methods".<o:p></o:p></pre><pre>    >><o:p></o:p></pre><pre>    >> The changes include new tests for issues 8193879, 8193801 and 8129348.<o:p></o:p></pre><pre>    >><o:p></o:p></pre><pre>    >> The changes in <o:p></o:p></pre><pre>    >> src/jdk.jdi/share/classes/com/sun/tools/example/debug/tty/TTY.java <o:p></o:p></pre><pre>    >> solve the problem that the prompt is not printed in the debugger <o:p></o:p></pre><pre>    >> output when the breakpoint is hit and the suspend policy is <o:p></o:p></pre><pre>    >> SUSPEND_EVENT_THREAD. This is required for new tests to detect that <o:p></o:p></pre><pre>    >> command "stop thread at ..." is completed.<o:p></o:p></pre><pre>    >><o:p></o:p></pre><pre>    >> Mach5 build and jdk_jdi tests succeeded.<o:p></o:p></pre><pre>    >><o:p></o:p></pre><pre>    >> Webrev: <a href="http://cr.openjdk.java.net/%7Edtitov/8193879/webrev.01/">http://cr.openjdk.java.net/~dtitov/8193879/webrev.01/</a><o:p></o:p></pre><pre>    >> Issue: <a href="https://bugs.openjdk.java.net/browse/JDK-8193879">https://bugs.openjdk.java.net/browse/JDK-8193879</a><o:p></o:p></pre><pre>    >><o:p></o:p></pre><pre>    >> Thanks,<o:p></o:p></pre><pre>    >> --Daniil<o:p></o:p></pre><pre>    >><o:p></o:p></pre><pre>    >><o:p></o:p></pre><pre>    >><o:p></o:p></pre><pre>    ><o:p></o:p></pre><pre>    <o:p></o:p></pre><pre>    <o:p></o:p></pre><pre> <o:p></o:p></pre><pre> <o:p></o:p></pre></blockquote><p class=MsoNormal style='mso-margin-top-alt:auto;margin-bottom:12.0pt'><o:p> </o:p></p></div></blockquote><p class=MsoNormal><br><br><o:p></o:p></p></div></body></html>