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
         >>>       >>
         >>>       >
         >>>
         >>>
         >>
         >>






Reply via email to