RFR 8211736: jdb doesn't print prompt when breakpoint is hit and suspend policy is STOP_EVENT_THREAD

JC Beyler jcbeyler at google.com
Thu Oct 18 01:53:09 UTC 2018


Hi Daniil,

It looks good to me.

I noticed a tiny nit in TTY.java where you removed an empty line (l171)

Thanks!
Jc


On Wed, Oct 17, 2018 at 5:50 PM Daniil Titov <daniil.x.titov at oracle.com>
wrote:

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

-- 

Thanks,
Jc
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20181017/532408a9/attachment-0001.html>


More information about the serviceability-dev mailing list