JDK-8219143: jdb should support breakpoint thread filters
Alex Menkov
alexey.menkov at oracle.com
Thu Feb 21 22:34:45 UTC 2019
Hi Chris,
Looks good to me.
Minor note - JdbStopThreadidTestTarg.MyThread.started should be volatile.
With this update and fix in comment mentioned by Serguei, no need to
send new webrev.
--alex
On 02/21/2019 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.
>> 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.
>
> 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