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