JDK-8219143: jdb should support breakpoint thread filters
Alex Menkov
alexey.menkov at oracle.com
Sat Feb 23 00:35:23 UTC 2019
+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