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