[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR 8211736: jdb doesn't print prompt when breakpoint is hit and suspend policy is STOP_EVENT_TH
From: Gary Adams <gary.adams () oracle ! com>
Date: 2018-10-23 12:40:21
Message-ID: 5BCF16B5.9090000 () oracle ! com
[Download RAW message or body]
Thanks for checking.
I think it is Ok if the tests are only supported in the default locale.
I think that restriction should be documented somewhere.
If there is a simple way to check at runtime, we could
issue a warning message.
Definitely out of scope for this specific changeset.
I wonder if this was discussed when the initial
alternate locale messages were introduced.
On 10/22/18, 5:54 PM, Daniil Titov wrote:
> Hi Gary,
>
> As I see currently multiple tests already use the patterns that in fact are \
> localized messages, e.g. for 'Breakpoint hit' the following tests expect to find \
> this message in the JDB output.
> grep -rn 'Breakpoint hit' open/test/jdk/com/sun/jdi/
> open/test/jdk/com/sun/jdi//JdbMissStep.java:82: \
> .shouldContain("Breakpoint hit"); open/test/jdk/com/sun/jdi//JdbExprTest.java:73: \
> .shouldContain("Breakpoint hit"); \
> open/test/jdk/com/sun/jdi//JdbVarargsTest.java:98: \
> .shouldMatch("Breakpoint hit:.*varString\\(\\)"); \
> open/test/jdk/com/sun/jdi//lib/jdb/Jdb.java:81: public static final String \
> BREAKPOINT_HIT = "Breakpoint hit:"; \
> open/test/jdk/com/sun/jdi//JdbMethodExitTest.java:204: \
> .shouldContain("Breakpoint hit"); \
> open/test/jdk/com/sun/jdi//JdbMethodExitTest.java:303: \
> .shouldContain("Breakpoint hit");
> If we really want to get rid of this (e.g. for the cases when these tests are run \
> with non-default English locale), I would suggest to file a separate issue for that \
> rather than addressing it in the current fix.
> Best regards,
> Daniil
>
> On 10/22/18, 4:50 AM, "Gary Adams"<gary.adams@oracle.com> wrote:
>
>
> Thanks for making the extra effort to avoid dependency on
> the simple prompt matching. One thing to check with the
> large reply matches - make sure the locale translated messages
> do not interfere with the compiled pattern matching.
> See MessageOutput.format().
>
> On 10/19/18, 7: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.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"<daniil.x.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, "gary.adams@oracle.com"<gary.adams@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.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"<daniil.x.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"<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"<alexey.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<jcbeyler@google.com>
> > > > > > > > > > *Date: *Thursday, October 11, 2018 at 7:17 PM
> > > > > > > > > > *To: *<daniil.x.titov@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.titov@oracle.com
> > > > > > > > <mailto:daniil.x.titov@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
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > >
> > > >
> > > >
> > > >
> > >
> > >
> > >
> > >
> > >
> >
> >
> >
> >
> >
>
>
>
>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic