Ok by me
Sent from my iPad > On Oct 22, 2018, at 8:26 PM, Daniil Titov <daniil.x.ti...@oracle.com> wrote: > > Thank you, Chris for reviewing this change! > > Alex, JC, Garry could you please say are you OK with this version of the > webrev? > > Webrev: http://cr.openjdk.java.net/~dtitov/8211736/webrev.05/ > Issue: https://bugs.openjdk.java.net/browse/JDK-8211736 > > Best regards, > Daniil > > On 10/22/18, 11:18 AM, "Chris Plummer" <chris.plum...@oracle.com> wrote: > > Hi Daniil, > > Looks good. > > thanks, > > Chris > >> On 10/19/18 4:01 PM, Daniil Titov wrote: >> Hi Chris, >> >> Please review an updated version of the fix that makes the tests to use a >> custom pattern while waiting for the command to complete. >> >> Webrev: http://cr.openjdk.java.net/~dtitov/8211736/webrev.05/ >> Issue: https://bugs.openjdk.java.net/browse/JDK-8211736 >> >> Thanks! >> --Daniil >> >> >> On 10/19/18, 12:55 PM, "Chris Plummer" <chris.plum...@oracle.com> wrote: >> >>> On 10/19/18 9:47 AM, Daniil Titov wrote: >>> Hi Gary and Chris, >>> >>> I am resending the previous email since the table in that email got >>> corrupted. >>> >>> Below is what output jdb prints when a breakpoint is hit depending on what >>> suspended policy is used: >>> >>> The current behavior. >>> SUSPEND_ALL: Prompt is printed ( e.g. "main[1]"), location is printed >>> SUSPEND_EVENT_THREAD: Prompt is not printed, location is not printed >>> SUSPEND_NONE: Prompt is not printed, location is not printed >>> >>> The fix changes this behavior as the following: >>> >>> SUSPEND_ALL: Prompt is printed ( e.g. "main[1]"), location is printed >>> SUSPEND_EVENT_THREAD: Prompt is printed ( "> "), location is printed >>> SUSPEND_NONE: Prompt is printed ( "> "), location is printed >> I'm still in favor of this fix. >>> >>> >>> Could you please say is it OK for you or you want that the location was >>> printed only for SUSPEND_ALL case? Or probably just leave all behavior >>> unchanged and close the bug as "not an issue"? >>> >>> Regarding settable prompt... As I understand Gary's original concern was >>> that waiting in tests for a simple prompt " > " (> ) could be >>> unreliable if this substring somehow appears in the output of the test >>> program and the suggestion was to use more specific patterns for the cases >>> when the full prompt (thread_name[frame_index]) is not printed ( e.g. when >>> the current thread is not set). However, we need somehow pass this pattern >>> to Jdb.command(JdbCommand) method otherwise it would keep waiting for the >>> full prompt and fail with the timeout. Probably I am missing something >>> here... >> Maybe we need a version of command() that takes a pattern to look for >> other than the prompt. >> >> Chris >>> >>> >>> Thanks! >>> --Daniil >>> >>> On 10/19/18, 9:27 AM, "Daniil Titov" <daniil.x.ti...@oracle.com> wrote: >>> >>> Hi Gary and Chris, >>> >>> Below is the table that shows what jdb output is printed when a >>> breakpoint is hit depending on what suspended policy is used: >>> >>> SUSPEND POLICY | PROMPT PRINTED | LOCATION >>> PRINTED >>> --------------------------------------- >>> |---------------------------|-------------------------- >>> SUSPEND_ALL. | yes, e.g. "main[1]" | >>> yes >>> --------------------------------------- |-------------------------- >>> |-------------------------- >>> SUSPEND_EVENT_THREAD | no | no >>> ----------------------------------------|------------------------ >>> --|-------------------------- >>> SUSPEND_ALL. | no >>> | no >>> >>> >>> The fix changes this behavior as the following: >>> >>> SUSPEND POLICY | PROMPT PRINTED. | LOCATION >>> PRINTED >>> --------------------------------------- >>> |----------------------------|-------------------------- >>> SUSPEND_ALL. | yes , e.g. "main[1]" | >>> yes >>> --------------------------------------- >>> |----------------------------|-------------------------- >>> SUSPEND_EVENT_THREAD | yes , ">" | >>> yes >>> ----------------------------------------|--------------------------- >>> |-------------------------- >>> SUSPEND_ALL. | yes, ">". >>> | yes >>> >>> Could you please say is it OK for you or you want that the location was >>> printed only for SUSPEND_ALL case? Or probably just leave all behavior >>> unchanged and close the bug as "not an issue"? >>> >>> Regarding settable prompt... As I understand Gary's original concern >>> was that waiting in tests for a simple prompt " > " (> ) could be >>> unreliable if this substring somehow appears in the output of the test >>> program and the suggestion was to use more specific patterns for the cases >>> when the full prompt (thread_name[frame_index]) is not printed ( e.g. when >>> the current thread is not set). However, we need somehow pass this pattern >>> to Jdb.command(JdbCommand) method otherwise it would keep waiting for the >>> full prompt and fail with the timeout. Probably I am missing something >>> here... >>> >>> >>> Thanks! >>> >>> Best regards, >>> Daniil >>> >>> >>> >>> On 10/19/18, 12:54 AM, "gary.ad...@oracle.com" <gary.ad...@oracle.com> >>> wrote: >>> >>> It's not clear to me if the omitted location information for the non >>> stopping >>> cases was intentional or not. It may be sufficient to know that the >>> event fired >>> without the extra information. >>> >>> I don't think a settable prompt is required either. There are >>> plenty of >>> jdb commands that >>> could be used with the wait for message in tests that need to >>> synchronize at a specific >>> point in the test sequence. >>> >>> I don't think we see wait for simple prompt in any of the tests, >>> because it >>> is not reliable enough. >>> >>>> On 10/18/18 11:53 AM, Daniil Titov wrote: >>>> Hi Gary, >>>> >>>> Currently when a breakpoint is hit the message "Breakpoint hit:" is >>>> printed in the debugger output. What we do in this fix we just add more >>>> information about what exact breakpoint is hit. Do you suggest we should >>>> not print this line at all if suspend policy is SUSPEND_NONE? If so then >>>> it is not clear what is the use of the command "stop go ..." would be. >>>> Regarding waiting for the simple prompt, we could change JDbCommand to >>>> have a field to store a prompt pattern and use this pattern (if it was >>>> set) when waiting for command to complete. In tests when required we would >>>> set the pattern in JdbCommand to more complicated one matching a specific >>>> output we are expecting at this particular step. Do you think it would be >>>> a better approach? >>>> >>>> Thanks! >>>> >>>> Best regards, >>>> Daniil >>>> >>>> >>>> >>>> On 10/18/18, 4:09 AM, "Gary Adams" <gary.ad...@oracle.com> wrote: >>>> >>>> I'm not certain that we should be printing locations or prompts for >>>> events when the vm has not been suspended. This looks OK for the >>>> simple test case we are working on here, but in real life there may >>>> be a lot more output produced. >>>> >>>> The user has to select a specific thread before additional commands >>>> can be performed in the correct context. e.g. threads, thread n, >>>> where, ... >>>> So the information is available to the user. It doesn't have to be >>>> produced at the time the event is processed. >>>> >>>> I'm uncomfortable putting too much trust in waiting for the simple >>>> prompt, >>>> because there is too great a chance of false positives on such a small >>>> marker. >>>> >>>> >>>>> On 10/17/18, 8:50 PM, Daniil Titov 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.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 >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>>> >>>>>>> >>>>>>> >>>>> >>>>> >>>>> >>>>> >>>>> >>>> >>>> >>>> >>>> >>> >>> >>> >>> >>> >> >> >> >> >> > > > > >
