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.men...@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 <jcbey...@google.com> >> *Date: *Thursday, October 11, 2018 at 7:17 PM >> *To: *<daniil.x.ti...@oracle.com> >> *Cc: *<serviceability-dev@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.ti...@oracle.com <mailto:daniil.x.ti...@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 >> >
