JDK-8219143: jdb should support breakpoint thread filters
Chris Plummer
chris.plummer at oracle.com
Sat Feb 23 01:33:33 UTC 2019
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 at 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 at 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 at oracle.com wrote:
>>>>>> On 2/21/19 09:19, Chris Plummer wrote:
>>>>>>> On 2/21/19 2:04 AM, serguei.spitsyn at 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
>>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
More information about the serviceability-dev
mailing list