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.spit...@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.spit...@oracle.com wrote:
On 2/21/19 09:19, Chris Plummer wrote:
On 2/21/19 2:04 AM, serguei.spit...@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
