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.ti...@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.plum...@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.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 >>> >> >>> > >>> >>> >> >>
