|
Thanks!
Hi Chris,
It looks good.
Thank you for the update!
Thanks,
Serguei
On 2/22/19 12:24 PM, Chris Plummer
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:
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
the printstopcommandusage.
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
|
