'Re: JDK-8219143: jdb should support breakpoint thread filters'

[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: JDK-8219143: jdb should support breakpoint thread filters
From:       Chris Plummer <chris.plummer () oracle ! com>
Date:       2019-02-23 1:33:33
Message-ID: dacbd869-ca94-875b-7396-eea47722e335 () oracle ! com
[Download RAW message or body]

Thanks Alex!

I'm not too concerned about future extensions. Also it was even 
suggested in a comment in the code to remove the need for at/in. I think 
the main reason for supporting named parameters would be to allow them 
to occur in any order, which isn't supported now.

Chris

On 2/22/19 4:35 PM, Alex Menkov wrote:
> +1 (although I prefer named parameters as unnamed restricts future 
> syntax extensions).
> 
> --alex
> 
> On 02/22/2019 15:13, serguei.spitsyn@oracle.com wrote:
> > Hi Chris,
> > 
> > It looks good.
> > Thank you for the update!
> > 
> > Thanks,
> > Serguei
> > 
> > 
> > On 2/22/19 12:24 PM, Chris Plummer wrote:
> > > Please review the updated webrev:
> > > 
> > > http://cr.openjdk.java.net/~cjplummer/8219143/webrev.02/index.html
> > > 
> > > They syntax is now:
> > > 
> > > stop [go|thread] [<thread_id>] <at|in> <location>
> > > 
> > > Basically I just removed the need to specify "threadid" before 
> > > <thread_id>.
> > > 
> > > Testing is in progress.
> > > 
> > > thanks,
> > > 
> > > Chris
> > > 
> > > On 2/21/19 6:56 PM, Chris Plummer wrote:
> > > > On 2/21/19 5:02 PM, serguei.spitsyn@oracle.com wrote:
> > > > > Chris,
> > > > > 
> > > > > New spec for stop command is:
> > > > > "Usage: stop [go|thread] [threadid <thread_id>] <at|in> 
> > > > > <location>\n"
> > > > > ...
> > > > > 253 " If \"thread\" is specified, only suspend the thread we stop 
> > > > > in\n" +
> > > > > ...
> > > > > 255 " If [threadid <thread_id>] is specified, only stop in the 
> > > > > specified thread" +
> > > > > 
> > > > > 
> > > > > I wonder if it can be changed to something like this:
> > > > > "Usage: stop [go|thread [<thread_id>]] <at|in> <location>\n"
> > > > That's not correct since it requires that you specify "go" or 
> > > > "thread" if you want to specify the thread to stop in. Right now if 
> > > > you do:
> > > > 
> > > > stop threadid 5 Main:3
> > > > 
> > > > It will only stop in threadid 5, but all threads will be suspended. 
> > > > Your syntax does not support this. I could attempt to go with:
> > > > 
> > > > "Usage: stop [go|thread] [<thread_id>] <at|in> <location>\n"
> > > > 
> > > > Basically the same as before, but not require the use of 
> > > > "threadid". I don't think it would be too hard. After dealing with 
> > > > "go" or "thread", if the next token is not "at" or "in", then 
> > > > require that it be a threadid integer.
> > > > 
> > > > Chris
> > > > > ...
> > > > > 253 " If \"thread\" is specified, only suspend the thread we stop 
> > > > > in\n" +
> > > > > ...
> > > > > 255 " If [<thread_id>] is specified, only stop in the specified 
> > > > > thread" +
> > > > > 
> > > > > 
> > > > > What do you think?
> > > > > 
> > > > > Thanks,
> > > > > Serguei
> > > > > 
> > > > > 
> > > > > On 2/21/19 16:46, serguei.spitsyn@oracle.com wrote:
> > > > > > On 2/21/19 09:19, Chris Plummer wrote:
> > > > > > > On 2/21/19 2:04 AM, serguei.spitsyn@oracle.com wrote:
> > > > > > > > Hi Chris,
> > > > > > > > 
> > > > > > > > Not a full review yet.
> > > > > > > > 
> > > > > > > > Just a couple of quick mismatches.
> > > > > > > > 
> > > > > > > > http://cr.openjdk.java.net/~cjplummer/8219143/webrev.01/src/jdk.jdi/share/classes/com/sun/tools/example/debug/tty/TTYResources.java.frames.html \
> > > > > > > >  
> > > > > > > > 
> > > > > > > > Is it okay that the two fragments below describe the same 
> > > > > > > > differently? Do I miss anything.
> > > > > > > The first is the help output when there is an error parsing the 
> > > > > > > "stop" command. The second is the help output when you type 
> > > > > > > "help", which includes help for all commands.
> > > > > > 
> > > > > > My question is that a couple of statements are missed in 
> > > > > > theprintstopcommandusage.
> > > > > > The help output prints these two lines which are not present in 
> > > > > > the printstopcommandusage:
> > > > > > 378 " -- set a breakpoint\n" +
> > > > > > 379 " -- if no options are given, the current list of breakpoints 
> > > > > > is printed\n" +
> > > > > > Is it intentional?
> > > > > > 
> > > > > > 
> > > > > > > > 250 {"printstopcommandusage",
> > > > > > > > 251 "Usage: stop [go|thread] [threadid <thread_id>] <at|in> 
> > > > > > > > <location>\n" +
> > > > > > > > 252 " If \"go\" is specified, immediately resume after 
> > > > > > > > stopping\n" +
> > > > > > > > 253 " If \"thread\" is specified, only suspend the thread we 
> > > > > > > > stop in\n" +
> > > > > > > > 254 " If neither \"go\" nor \"thread\" are specified, suspend 
> > > > > > > > all threads\n" +
> > > > > > > > 255 " If [threadid <thread_id>] is specified, only stop in the 
> > > > > > > > specified thread" +
> > > > > > > > 256 " \"at\" and \"in\" have the same meaning\n" +
> > > > > > > > 257 " <location> can either be a line number or a method:\n" +
> > > > > > > > 258 " <class_id>:<line_number>\n" +
> > > > > > > > 259 " <class_id>.<method>[(argument_type,...)]"
> > > > > > > > 260 },
> > > > > > > > 377 "stop [go|thread] [threadid <thread_id>] <at|in> 
> > > > > > > > <location>\n" +
> > > > > > > > 378 " -- set a breakpoint\n" +
> > > > > > > > 379 " -- if no options are given, the current list of 
> > > > > > > > breakpoints is printed\n" +
> > > > > > > > 380 " -- if \"go\" is specified, immediately resume after 
> > > > > > > > stopping\n" +
> > > > > > > > 381 " -- if \"thread\" is specified, only suspend the thread we 
> > > > > > > > stop in\n" +
> > > > > > > > 382 " -- if neither \"go\" nor \"thread\" are specified, 
> > > > > > > > suspend all threads\n" +
> > > > > > > > 383 " -- if [threadid <thread_id>] is specified, only stop in 
> > > > > > > > the specified thread\n" +
> > > > > > > > 384 " -- \"at\" and \"in\" have the same meaning\n" +
> > > > > > > > 385 " -- <location> can either be a line number or a method:\n" +
> > > > > > > > 386 " -- <class_id>:<line_number>\n" +
> > > > > > > > 387 " -- <class_id>.<method>[(argument_type,...)]\n" +
> > > > > > > > 
> > > > > > > > 
> > > > > > > > http://cr.openjdk.java.net/~cjplummer/8219143/webrev.01/src/jdk.jdi/share/classes/com/sun/tools/example/debug/tty/Commands.java.frames.html \
> > > > > > > >  
> > > > > > > > 
> > > > > > > > 1162 * Allowed syntax:
> > > > > > > > 1163 * stop [go|thread] <at|in> [threadid <thread_id>] <location>
> > > > > > > > 1164 * <location> can either be a line number or a method:
> > > > > > > > 1165 * - <class id>:<line>
> > > > > > > > 1166 * - <class id>.<method>[(argument_type,...)]
> > > > > > > > 1167 * If "go" is specified, then immediately resume after 
> > > > > > > > stopping. No threads are suspended.
> > > > > > > > 1168 * If "thread" is specified, then only suspend the thread 
> > > > > > > > we stop in.
> > > > > > > > 1169 * If neither "go" nor "thread" are specified, then suspend 
> > > > > > > > all threads.
> > > > > > > > 1170 * If the optional [threadid <thread_id>] is specified, 
> > > > > > > > then only stop in the specified thread.
> > > > > > > > 1171 * If no options are given, the current list of breakpoints 
> > > > > > > > is printed,
> > > > > > > > 1172 */The above is not aligned with the implementation as 
> > > > > > > > <at|in>   has to be right before <location>.
> > > > > > > > A dot is needed instead of the last comma.
> > > > > > > > Should this comment to match one of the above descriptions?
> > > > > > > Yes, I'll update this to look like the help output. I missed it 
> > > > > > > when moving the at/in location.
> > > > > > 
> > > > > > Okay, thanks!
> > > > > > 
> > > > > > I'll send full review soon.
> > > > > > 
> > > > > > Thanks,
> > > > > > Serguei
> > > > > > 
> > > > > > > 
> > > > > > > thanks,
> > > > > > > 
> > > > > > > Chris
> > > > > > > > Thanks,
> > > > > > > > Serguei
> > > > > > > > 
> > > > > > > > 
> > > > > > > > On 2/20/19 19:56, Chris Plummer wrote:
> > > > > > > > > Please review the updated webrev:
> > > > > > > > > 
> > > > > > > > > http://cr.openjdk.java.net/~cjplummer/8219143/webrev.01/
> > > > > > > > > 
> > > > > > > > > The syntax is now:
> > > > > > > > > 
> > > > > > > > > stop [go|thread] [threadid <thread_id>] <at|in> <location>
> > > > > > > > > 
> > > > > > > > > I filed JDK-8219507 to cover BreakpointSpec.toString() 
> > > > > > > > > improvements, and other possible improvements to the output 
> > > > > > > > > when listing breakpoints.
> > > > > > > > > 
> > > > > > > > > I did a minor fix in TTYResources.java to address a bug from a 
> > > > > > > > > recent JDK-8218941 commit that added the dbgtrace command. 
> > > > > > > > > There was missing newline. I've added a note to JDK-8218941 
> > > > > > > > > indicating that it is being fixed here.
> > > > > > > > > 
> > > > > > > > > I've added a test. Below is a log of the debug session output 
> > > > > > > > > that might help with reading the source of the test. The test 
> > > > > > > > > creates 3 threads that all execute the same code, and verifies 
> > > > > > > > > that the breakpoint set using the threadid option only stops 
> > > > > > > > > on the one specified thread.
> > > > > > > > > 
> > > > > > > > > thanks,
> > > > > > > > > 
> > > > > > > > > Chris
> > > > > > > > > 
> > > > > > > > > [jdb] Set uncaught java.lang.Throwable
> > > > > > > > > [jdb] Set deferred uncaught java.lang.Throwable
> > > > > > > > > [jdb] Initializing jdb ...
> > > > > > > > > [jdb]
> > > > > > > > > [jdb] VM Started: > No frames on the current call stack
> > > > > > > > > [jdb]
> > > > > > > > > [jdb] main[1]
> > > > > > > > > > stop in JdbStopThreadidTestTarg.brkMethod
> > > > > > > > > [jdb] Deferring breakpoint JdbStopThreadidTestTarg.brkMethod.
> > > > > > > > > [jdb] It will be set after the class is loaded.
> > > > > > > > > [jdb] main[1]
> > > > > > > > > > run
> > > > > > > > > [jdb] > Set deferred breakpoint JdbStopThreadidTestTarg.brkMethod
> > > > > > > > > [jdb]
> > > > > > > > > [jdb] Breakpoint hit: "thread=main", 
> > > > > > > > > JdbStopThreadidTestTarg.brkMethod(), line=80 bci=0
> > > > > > > > > [jdb] 80               }
> > > > > > > > > [jdb]
> > > > > > > > > [jdb] main[1]
> > > > > > > > > > threads
> > > > > > > > > [jdb] Group system:
> > > > > > > > > [jdb] (java.lang.ref.Reference$ReferenceHandler)367 Reference 
> > > > > > > > > Handler running
> > > > > > > > > [jdb] (java.lang.ref.Finalizer$FinalizerThread)368 
> > > > > > > > > Finalizer                 cond. waiting
> > > > > > > > > [jdb]     (java.lang.Thread)369 Signal Dispatcher running
> > > > > > > > > [jdb] Group main:
> > > > > > > > > [jdb]     (java.lang.Thread)1 main running (at breakpoint)
> > > > > > > > > [jdb]     (JdbStopThreadidTestTarg$MyThread)482 
> > > > > > > > > MYTHREAD-1               waiting in a monitor
> > > > > > > > > [jdb]     (JdbStopThreadidTestTarg$MyThread)483 
> > > > > > > > > MYTHREAD-2               waiting in a monitor
> > > > > > > > > [jdb]     (JdbStopThreadidTestTarg$MyThread)484 
> > > > > > > > > MYTHREAD-3               waiting in a monitor
> > > > > > > > > [jdb] Group InnocuousThreadGroup:
> > > > > > > > > [jdb]     (jdk.internal.misc.InnocuousThread)416 
> > > > > > > > > Common-Cleaner       cond. waiting
> > > > > > > > > [jdb] main[1]
> > > > > > > > > > stop threadid 483 in JdbStopThreadidTestTarg$MyThread.brkMethod
> > > > > > > > > [jdb] Set breakpoint JdbStopThreadidTestTarg$MyThread.brkMethod
> > > > > > > > > [jdb] main[1]
> > > > > > > > > > cont
> > > > > > > > > [jdb] >
> > > > > > > > > [jdb] Breakpoint hit: "thread=MYTHREAD-2", 
> > > > > > > > > JdbStopThreadidTestTarg$MyThread.brkMethod(), line=101 bci=0
> > > > > > > > > [jdb] 101                       }
> > > > > > > > > [jdb]
> > > > > > > > > [jdb] MYTHREAD-2[1]
> > > > > > > > > > cont
> > > > > > > > > [jdb] >
> > > > > > > > > [jdb] The application exited
> > > > > > > > > [jdb]
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > On 2/20/19 2:23 PM, Chris Plummer wrote:
> > > > > > > > > > On 2/20/19 2:00 PM, Alex Menkov wrote:
> > > > > > > > > > > Hi Chris,
> > > > > > > > > > > 
> > > > > > > > > > > - New threadid param breaks at/in clause - this doesn't look 
> > > > > > > > > > > good.
> > > > > > > > > > > Maybe it would be better to put threadid before in/at:
> > > > > > > > > > > stop [go|thread] [threadid <thread_id>] <at|in> <location>
> > > > > > > > > > Ok. I'll try moving it.
> > > > > > > > > > > This also would make commandStop/parseBreakpointSpec logic 
> > > > > > > > > > > more clear:
> > > > > > > > > > > commandStop handles go/thread/threadid, parseBreakpointSpec 
> > > > > > > > > > > handles position (part starting from in/at);
> > > > > > > > > > > 
> > > > > > > > > > > - do you think BreakpointSpec toString should contain info 
> > > > > > > > > > > about the thread (i.e. to specified in the breakpoint list)?;
> > > > > > > > > > I had added this at one point, alone with also adding the 
> > > > > > > > > > suspend policy. You then see both of these when you use "stop 
> > > > > > > > > > in/at" or "clear" without any arguments (it lists the 
> > > > > > > > > > breakpoints in this case). I was a little worried that the 
> > > > > > > > > > extra text might break something, so I left it out. Note also 
> > > > > > > > > > that currently the text given when you list breakpoints is 
> > > > > > > > > > exactly the text you need to enter when using the clear 
> > > > > > > > > > command, so that would no longer be true if I added any more 
> > > > > > > > > > text. However, I did envision a better version of this that 
> > > > > > > > > > not only include the suspend policy and threadid, but also a 
> > > > > > > > > > breakpoint index, allowing you to more easily clear one after 
> > > > > > > > > > listing it. Maybe I could add an RFE for this.
> > > > > > > > > > > 
> > > > > > > > > > > - it would be nice to add a test for the new threadid 
> > > > > > > > > > > functionality.
> > > > > > > > > > 
> > > > > > > > > > I've been working on one today. It's near completion.
> > > > > > > > > > 
> > > > > > > > > > Chris
> > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > > --alex
> > > > > > > > > > > 
> > > > > > > > > > > On 02/19/2019 21:57, Chris Plummer wrote:
> > > > > > > > > > > > Hello,
> > > > > > > > > > > > 
> > > > > > > > > > > > Please review the following:
> > > > > > > > > > > > 
> > > > > > > > > > > > http://cr.openjdk.java.net/~cjplummer/8219143/webrev.00/
> > > > > > > > > > > > https://bugs.openjdk.java.net/browse/JDK-8219143
> > > > > > > > > > > > 
> > > > > > > > > > > > Normally when a breakpoint is set in jdb, it is set 
> > > > > > > > > > > > globally (all threads). JDI supports the ability to have 
> > > > > > > > > > > > the breakpoint be automatically filtered so it will only be 
> > > > > > > > > > > > delivered on a specified thread and ignored on all other 
> > > > > > > > > > > > threads. This change allows making use of that JDI feature 
> > > > > > > > > > > > from jdb. So instead of something like:
> > > > > > > > > > > > 
> > > > > > > > > > > > stop at Foo:23
> > > > > > > > > > > > 
> > > > > > > > > > > > You can now do:
> > > > > > > > > > > > 
> > > > > > > > > > > > stop at threadid 7 Foo:23
> > > > > > > > > > > > 
> > > > > > > > > > > > Where 7 is the threadID for the thread as seen in the 
> > > > > > > > > > > > output of the "threads" command. The format of the stop 
> > > > > > > > > > > > command is now:
> > > > > > > > > > > > 
> > > > > > > > > > > > stop [go|thread] <at|in> [threadid <thread_id>] <location>
> > > > > > > > > > > > 
> > > > > > > > > > > > It is still fully backwards compatible.
> > > > > > > > > > > > 
> > > > > > > > > > > > As part of this change I also cleaned up the "stop" command 
> > > > > > > > > > > > parsing and error handling. It was kind of a mess, and was 
> > > > > > > > > > > > near impossible to add the "threadid" option until after I 
> > > > > > > > > > > > did much of the cleanup. I've also cleaned up the help 
> > > > > > > > > > > > output a lot, and added help for the "go" and "thread" 
> > > > > > > > > > > > options. One last change was to remove the distinction 
> > > > > > > > > > > > between "stop at" and "stop in", which was suggested 
> > > > > > > > > > > > already in a comment. They can be used interchangeably now. 
> > > > > > > > > > > > The changes in Commands.java are pretty much all just the 
> > > > > > > > > > > > above cleanup, except for the one short section with the 
> > > > > > > > > > > > 'Handle "threadid" modifier' comment.
> > > > > > > > > > > > 
> > > > > > > > > > > > thanks,
> > > > > > > > > > > > 
> > > > > > > > > > > > Chris
> > > > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > 
> > > > > > > 
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 


[prev in list] [next in list] [prev in thread] [next in thread]
Configure | About | News | Add a list | Sponsored by KoreLogic