LGTM as well,
Jc
On Mon, Oct 22, 2018 at 5:47 PM Alex Menkov <***@oracle.com>
wrote:
> I'm fine with it
>
> --alex
>
> On 10/22/2018 17:26, Daniil Titov 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" <***@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" <***@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" <
> ***@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, "***@oracle.com" <
> ***@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" <
> ***@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"<
> ***@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"<***@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"<***@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<
> ***@google.com>
> > > > > > >>> >> *Date: *Thursday,
> October 11, 2018 at 7:17 PM
> > > > > > >>> >> *To: *<
> ***@oracle.com>
> > > > > > >>> >> *Cc: *<
> serviceability-***@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
> > > > > > >>> >> <
> ***@oracle.com
> > > > > > >>> <mailto:
> ***@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