<div dir="ltr"><div dir="ltr">Hi Daniil,<div><br></div><div>It looks good to me.</div><div><br></div><div>I noticed a tiny nit in TTY.java where you removed an empty line (l171)</div><div><br></div><div>Thanks!</div><div>Jc</div><div><br></div><div></div></div></div><br><div class="gmail_quote"><div dir="ltr">On Wed, Oct 17, 2018 at 5:50 PM Daniil Titov <<a href="mailto:daniil.x.titov@oracle.com">daniil.x.titov@oracle.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Chris, Alex and JC,<br>
<br>
I created a separate issue to deal with "non-atomic" jdb output at <a href="https://bugs.openjdk.java.net/browse/JDK-8212626" rel="noreferrer" target="_blank">https://bugs.openjdk.java.net/browse/JDK-8212626</a> .<br>
<br>
Please review an updated fix that includes the changes Alex suggested.<br>
<br>
Webrev: <a href="http://cr.openjdk.java.net/~dtitov/8211736/webrev.04" rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~dtitov/8211736/webrev.04</a> <br>
Issue: <a href="https://bugs.openjdk.java.net/browse/JDK-8211736" rel="noreferrer" target="_blank">https://bugs.openjdk.java.net/browse/JDK-8211736</a><br>
<br>
Thanks!<br>
--Daniil<br>
<br>
<br>
On 10/17/18, 5:06 PM, "Daniil Titov" <<a href="mailto:daniil.x.titov@oracle.com" target="_blank">daniil.x.titov@oracle.com</a>> wrote:<br>
<br>
    Hi Chris,<br>
<br>
    The previous email was accidentally fired before I managed to type anything in. Sorry for confusion.<br>
<br>
    Jdb constantly reads new commands from System.in despite whether the breakpoint is hit and/or the prompt is printed.<br>
<br>
    cat -n src/jdk.jdi/share/classes/com/sun/tools/example/debug/tty/TTY.java<br>
<br>
    786             // Process interactive commands.<br>
       787                  MessageOutput.printPrompt();<br>
       788                  while (true) {<br>
       789                      String ln = in.readLine();<br>
       790                      if (ln == null) {<br>
       791                          /*<br>
       792                           *  Jdb is being shutdown because debuggee exited, ignore any 'null'<br>
       793                           *  returned by readLine() during shutdown. JDK-8154144.<br>
       794                           */<br>
       795                          if (!isShuttingDown()) {<br>
       796                              MessageOutput.println("Input stream closed.");<br>
       797                          }<br>
       798                          ln = "quit";<br>
       799                      }<br>
       800      <br>
       801                      if (ln.startsWith("!!") && lastLine != null) {<br>
       802                          ln = lastLine + ln.substring(2);<br>
       803                          MessageOutput.printDirectln(ln);// Special case: use printDirectln()<br>
       804                      }<br>
       805      <br>
       806                      StringTokenizer t = new StringTokenizer(ln);<br>
       807                      if (t.hasMoreTokens()) {<br>
       808                          lastLine = ln;<br>
       809                          executeCommand(t);<br>
       810                      } else {<br>
       811                          MessageOutput.printPrompt();<br>
       812                      }<br>
       813                  }<br>
       814              } catch (VMDisconnectedException e) {<br>
       815                  handler.handleDisconnectedException();<br>
       816              }<br>
<br>
    Below is a sample debug session for the following test class that sends commands "threads" and "quit"<br>
    1   public class LoopTest {<br>
         2          public static void main(String[] args) throws Exception {<br>
         3              Thread thread = Thread.currentThread();<br>
         4              while(true) {<br>
         5                  print(thread);<br>
         6                  Thread.sleep(5000);<br>
         7              }<br>
         8          }<br>
         9      <br>
        10          public static void print(Object obj) {<br>
        11              //System.out.println(obj);<br>
        12          }<br>
        13      }<br>
<br>
    datitov-mac:work datitov$ ~/src/jdk-hs/build/macosx-x64-debug/images/jdk/bin/jdb LoopTest<br>
    Initializing jdb ...<br>
    > stop go at LoopTest:6<br>
    Deferring breakpoint LoopTest:6.<br>
    It will be set after the class is loaded.<br>
    > run<br>
    run LoopTest<br>
    Set uncaught java.lang.Throwable<br>
    Set deferred uncaught java.lang.Throwable<br>
    > <br>
    VM Started: Set deferred breakpoint LoopTest:6<br>
<br>
    Breakpoint hit: "thread=main", LoopTest.main(), line=6 bci=8<br>
    6                Thread.sleep(5000);<br>
<br>
    Breakpoint hit: "thread=main", LoopTest.main(), line=6 bci=8<br>
    6                Thread.sleep(5000);<br>
    threads<br>
    Group system:<br>
      (java.lang.ref.Reference$ReferenceHandler)0x172 Reference Handler running<br>
      (java.lang.ref.Finalizer$FinalizerThread)0x173  Finalizer         cond. waiting<br>
      (java.lang.Thread)0x174                         Signal Dispatcher running<br>
    Group main:<br>
      (java.lang.Thread)0x1                           main              sleeping<br>
    Group InnocuousThreadGroup:<br>
      (jdk.internal.misc.InnocuousThread)0x197        Common-Cleaner    cond. waiting<br>
    > <br>
    Breakpoint hit: "thread=main", LoopTest.main(), line=6 bci=8<br>
    6                Thread.sleep(5000);<br>
<br>
    Breakpoint hit: "thread=main", LoopTest.main(), line=6 bci=8<br>
    6                Thread.sleep(5000);<br>
    quit<br>
    Breakpoint hit: "thread=main", LoopTest.main(), line=6 bci=8<br>
    6                Thread.sleep(5000);<br>
<br>
    datitov-mac:work datitov$<br>
<br>
    I think we could print a simple prompt in this case as Alex suggested. <br>
<br>
    Best regards,<br>
    Daniil<br>
<br>
    On 10/17/18, 3:52 PM, "Chris Plummer" <<a href="mailto:chris.plummer@oracle.com" target="_blank">chris.plummer@oracle.com</a>> wrote:<br>
<br>
        What is the jdb state for a breakpoint with SUSPEND_NONE? Can you <br>
        actually type in commands even though no threads are suspended?<br>
<br>
        Chris<br>
<br>
        On 10/17/18 3:31 PM, Alex Menkov wrote:<br>
        > Hi Daniil, Chris,<br>
        ><br>
        > As far as I understand, the fix does not completely fixes all <br>
        > "non-atomic" output issues (at least TTY.executeCommand still prints <br>
        > prompt separately), so I agree that handle it as separate issue would <br>
        > be better.<br>
        > Unfortunately I don't know Gary's ideas for the issue.<br>
        ><br>
        > About the fix for print prompt:<br>
        > 1) TTY.java:<br>
        > +            // Print current location if suspend policy is SUSPEND_NONE<br>
        > I suppose you mean "Print breakpoint location"<br>
        ><br>
        > 2) Does it make sense to use printCurrentLocation for <br>
        > SUSPEND_EVENT_THREAD?<br>
        > Is it expected to print something different from printBreakpointLocation?<br>
        ><br>
        > 3) I don't quite understand why we don't print simple prompt for <br>
        > SUSPEND_NONE. IIUC jdb can accept new command in both <br>
        > SUSPEND_EVENT_THREAD and SUSPEND_NONE cases (prompt shows that jdb <br>
        > waits for next command, right?)<br>
        ><br>
        > 4) JdbStopThreadTest.java<br>
        > New line line in Java regexp is "\\R"<br>
        ><br>
        > --alex<br>
        ><br>
        > On 10/17/2018 10:47, Chris Plummer wrote:<br>
        >> Hi Alex,<br>
        >><br>
        >> I think the tty buffering should be a separate bug fix, and I'd also <br>
        >> like input from Gary on it since he had tried something similar at <br>
        >> one point. It should probably also include a multithread test to <br>
        >> prove the fix is working (after first getting the test to fail <br>
        >> without the changes).<br>
        >><br>
        >> thanks,<br>
        >><br>
        >> Chris<br>
        >><br>
        >> On 10/16/18 8:59 PM, Daniil Titov wrote:<br>
        >>> Hi Alex, Chris and JC,<br>
        >>><br>
        >>> Please review an updated version of the patch that addresses these <br>
        >>> issues.<br>
        >>><br>
        >>> Webrev: <a href="http://cr.openjdk.java.net/~dtitov/8211736/webrev.03/" rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~dtitov/8211736/webrev.03/</a><br>
        >>> Issue:  <a href="https://bugs.openjdk.java.net/browse/JDK-8211736" rel="noreferrer" target="_blank">https://bugs.openjdk.java.net/browse/JDK-8211736</a><br>
        >>><br>
        >>> Thanks!<br>
        >>> --Daniil<br>
        >>><br>
        >>><br>
        >>> On 10/12/18, 9:52 AM, "Alex Menkov" <<a href="mailto:alexey.menkov@oracle.com" target="_blank">alexey.menkov@oracle.com</a>> wrote:<br>
        >>><br>
        >>>      Hi Daniil,<br>
        >>>      1) +1 for printCurrentLocation when suspendPolicy != SUSPEND_ALL<br>
        >>>      2) wrong indent in JdbStopThreadTest.java:<br>
        >>>         36         import lib.jdb.JdbCommand;<br>
        >>>         37         import lib.jdb.JdbTest;<br>
        >>>      3) I think it would be better to make waitForSimplePrompt <br>
        >>> property of<br>
        >>>      JdbCommand (like JdbCommand.allowExit)<br>
        >>>      jdb.command(JdbCommand.run().replyIsSimplePrompt());<br>
        >>>      looks much clearer than<br>
        >>>      jdb.command(JdbCommand.run(), true);<br>
        >>>      4) (TTY.java)<br>
        >>>          MessageOutput.lnprint("Breakpoint hit:");<br>
        >>>      +  // Print current location and prompt if suspend policy is<br>
        >>>      SUSPEND_EVENT_THREAD.<br>
        >>>      +  // In case of SUSPEND_ALL policy this is handled by <br>
        >>> vmInterrupted()<br>
        >>>      method.<br>
        >>>      +  if (be.request().suspendPolicy() == <br>
        >>> EventRequest.SUSPEND_EVENT_THREAD) {<br>
        >>>      + printCurrentLocation(ThreadInfo.getThreadInfo(be.thread()));<br>
        >>>      +      MessageOutput.printPrompt();<br>
        >>>      +  }<br>
        >>>      We have 3 separate outputs.<br>
        >>>      If we have several concurrent threads, we can easy get mess of <br>
        >>> outputs<br>
        >>>      from different threads.<br>
        >>>      I think it would be better to print everything in a single chunk.<br>
        >>>      I suppose TTY has other places with similar "non-atomic" <br>
        >>> output, so<br>
        >>>      maybe revising TTY output should be handled by separate issue.<br>
        >>>      --alex<br>
        >>>      On 10/11/2018 22:00, Chris Plummer wrote:<br>
        >>>      > Hi Daniil,<br>
        >>>      ><br>
        >>>      > Can you send output from an example jdb session?<br>
        >>>      ><br>
        >>>      > Do you think maybe we should also call printCurrentLocation() <br>
        >>> when the<br>
        >>>      > suspend policy is NONE?<br>
        >>>      ><br>
        >>>      > There's a typo in the following line. The space is on the <br>
        >>> wrong side of<br>
        >>>      > the comma.<br>
        >>>      ><br>
        >>>      >    72 jdb.command(JdbCommand.stopThreadAt(DEBUGGEE_CLASS <br>
        >>> ,bpLine));<br>
        >>>      ><br>
        >>>      > thanks,<br>
        >>>      ><br>
        >>>      > Chris<br>
        >>>      ><br>
        >>>      > On 10/11/18 8:02 PM, Daniil Titov wrote:<br>
        >>>      >><br>
        >>>      >> Thank you,  JC!<br>
        >>>      >><br>
        >>>      >> Please review an updated version of the patch that fixes <br>
        >>> newly added<br>
        >>>      >> test for Windows platform  (now it uses system dependent line<br>
        >>>      >> separator string).<br>
        >>>      >><br>
        >>>      >> Webrev:<a href="http://cr.openjdk.java.net/~dtitov/8211736/webrev.02/" rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~dtitov/8211736/webrev.02/</a><br>
        >>>      >> <<a href="http://cr.openjdk.java.net/%7Edtitov/8211736/webrev.02/" rel="noreferrer" target="_blank">http://cr.openjdk.java.net/%7Edtitov/8211736/webrev.02/</a>><br>
        >>>      >><br>
        >>>      >> Issue: <a href="https://bugs.openjdk.java.net/browse/JDK-8211736" rel="noreferrer" target="_blank">https://bugs.openjdk.java.net/browse/JDK-8211736</a><br>
        >>>      >><br>
        >>>      >> Best regards,<br>
        >>>      >><br>
        >>>      >> --Daniil<br>
        >>>      >><br>
        >>>      >> *From: *JC Beyler <<a href="mailto:jcbeyler@google.com" target="_blank">jcbeyler@google.com</a>><br>
        >>>      >> *Date: *Thursday, October 11, 2018 at 7:17 PM<br>
        >>>      >> *To: *<<a href="mailto:daniil.x.titov@oracle.com" target="_blank">daniil.x.titov@oracle.com</a>><br>
        >>>      >> *Cc: *<<a href="mailto:serviceability-dev@openjdk.java.net" target="_blank">serviceability-dev@openjdk.java.net</a>><br>
        >>>      >> *Subject: *Re: RFR 8211736: jdb doesn't print prompt when <br>
        >>> breakpoint<br>
        >>>      >> is hit and suspend policy is STOP_EVENT_THREAD<br>
        >>>      >><br>
        >>>      >> Hi Daniil,<br>
        >>>      >><br>
        >>>      >> Looks good to me. I saw a really small nit:<br>
        >>>      >><br>
        >>>      >> <br>
        >>> <a href="http://cr.openjdk.java.net/~dtitov/8211736/webrev.01/test/jdk/com/sun/jdi/JdbStopThreadTest.java.html" rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~dtitov/8211736/webrev.01/test/jdk/com/sun/jdi/JdbStopThreadTest.java.html</a> <br>
        >>><br>
        >>>      >> <br>
        >>> <<a href="http://cr.openjdk.java.net/%7Edtitov/8211736/webrev.01/test/jdk/com/sun/jdi/JdbStopThreadTest.java.html" rel="noreferrer" target="_blank">http://cr.openjdk.java.net/%7Edtitov/8211736/webrev.01/test/jdk/com/sun/jdi/JdbStopThreadTest.java.html</a>> <br>
        >>><br>
        >>>      >><br>
        >>>      >> 70  jdb.command(JdbCommand.stopThreadAt( DEBUGGEE_CLASS <br>
        >>> ,bpLine));<br>
        >>>      >><br>
        >>>      >> There is a space after the '('.<br>
        >>>      >><br>
        >>>      >> No need to send a new webrev for that evidently :),<br>
        >>>      >><br>
        >>>      >> Jc<br>
        >>>      >><br>
        >>>      >> On Thu, Oct 11, 2018 at 5:07 PM Daniil Titov<br>
        >>>      >> <<a href="mailto:daniil.x.titov@oracle.com" target="_blank">daniil.x.titov@oracle.com</a> <br>
        >>> <mailto:<a href="mailto:daniil.x.titov@oracle.com" target="_blank">daniil.x.titov@oracle.com</a>>> wrote:<br>
        >>>      >><br>
        >>>      >>     Please review the change that fixes the issue with <br>
        >>> missing prompt<br>
        >>>      >>     in jdb when a breakpoint is hit and the suspend policy <br>
        >>> is set to<br>
        >>>      >>     stop the thread only.<br>
        >>>      >><br>
        >>>      >>     Webrev: <br>
        >>> <a href="http://cr.openjdk.java.net/~dtitov/8211736/webrev.01" rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~dtitov/8211736/webrev.01</a><br>
        >>>      >> <<a href="http://cr.openjdk.java.net/%7Edtitov/8211736/webrev.01" rel="noreferrer" target="_blank">http://cr.openjdk.java.net/%7Edtitov/8211736/webrev.01</a>><br>
        >>>      >>     Issue: <a href="https://bugs.openjdk.java.net/browse/JDK-8211736" rel="noreferrer" target="_blank">https://bugs.openjdk.java.net/browse/JDK-8211736</a><br>
        >>>      >><br>
        >>>      >>     Thanks!<br>
        >>>      >>     --Daniil<br>
        >>>      >><br>
        >>>      >><br>
        >>>      >><br>
        >>>      >> --<br>
        >>>      >><br>
        >>>      >> Thanks,<br>
        >>>      >><br>
        >>>      >> Jc<br>
        >>>      >><br>
        >>>      ><br>
        >>><br>
        >>><br>
        >><br>
        >><br>
<br>
<br>
<br>
<br>
<br>
</blockquote></div><br clear="all"><div><br></div>-- <br><div dir="ltr" class="gmail_signature" data-smartmail="gmail_signature"><div dir="ltr"><div><br></div>Thanks,<div>Jc</div></div></div>