JDK-8219143: jdb should support breakpoint thread filters

Chris Plummer chris.plummer at oracle.com
Thu Feb 21 23:29:19 UTC 2019 

Hi Alex,

I'll make the volatile change.

thanks,

Chris

On 2/21/19 2:34 PM, Alex Menkov wrote:
> 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