<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
Hi Chris,<br>
<br>
It looks good.<br>
Thank you for the update!<br>
<br>
Thanks,<br>
Serguei<br>
<br>
<br>
<div class="moz-cite-prefix">On 2/22/19 12:24 PM, Chris Plummer
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:198a5a7d-365c-0a26-873c-e925041e7f4d@oracle.com">
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<div class="moz-cite-prefix">Please review the updated webrev:<br>
<br>
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ecjplummer/8219143/webrev.02/index.html"
moz-do-not-send="true">http://cr.openjdk.java.net/~cjplummer/8219143/webrev.02/index.html</a><br>
<br>
They syntax is now:<br>
<br>
       stop [go|thread] [<thread_id>] <at|in>
<location><br>
<br>
Basically I just removed the need to specify "threadid" before
<thread_id>.<br>
<br>
Testing is in progress.<br>
<br>
thanks,<br>
<br>
Chris<br>
<br>
On 2/21/19 6:56 PM, Chris Plummer wrote:<br>
</div>
<blockquote type="cite"
cite="mid:69eb2e2c-01f9-5882-85f9-232d252bd502@oracle.com">
<meta http-equiv="Content-Type" content="text/html;
charset=utf-8">
<div class="moz-cite-prefix">On 2/21/19 5:02 PM, <a
class="moz-txt-link-abbreviated"
href="mailto:serguei.spitsyn@oracle.com"
moz-do-not-send="true">serguei.spitsyn@oracle.com</a> wrote:<br>
</div>
<blockquote type="cite"
cite="mid:385bd38c-c28e-9048-9fc5-015319d17670@oracle.com">
<meta http-equiv="Content-Type" content="text/html;
charset=utf-8">
<div class="moz-cite-prefix">Chris,<br>
<br>
New spec for stop command is:<br>
<span class="new">Â Â "Usage: stop [go|thread] [threadid
<thread_id>] <at|in> <location>\n"</span><span
class="changed"><br>
 ...<br>
 253 " If \"thread\" is specified, only suspend the thread
we stop in\n" +</span><span class="changed"><br>
 ...</span><span class="changed"><br>
 255 " If [threadid <thread_id>] is specified, only
stop in the specified thread" +</span><br>
<span class="new"></span><br>
<span class="new"><br>
I wonder if it can be changed to something like this:</span><br>
<span class="new"></span><span class="new">Â Â "Usage: stop
[go|thread </span><span class="new"><span class="new">[<thread_id>]</span>]
<at|in> <location>\n"</span><br>
</div>
</blockquote>
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:<br>
<br>
   stop threadid 5 Main:3<br>
<br>
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:<br>
<br>
<span class="new"></span><span class="new">Â Â "Usage: stop
[go|thread] </span><span class="new"><span class="new">[<thread_id>]</span>
<at|in> <location>\n"<br>
<br>
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.<br>
<br>
Chris<br>
</span>
<blockquote type="cite"
cite="mid:385bd38c-c28e-9048-9fc5-015319d17670@oracle.com">
<div class="moz-cite-prefix"> <span class="new"><span
class="changed">Â ...<br>
 253 " If \"thread\" is specified, only suspend the
thread we stop in\n" +</span><span class="changed"><br>
 ...</span><span class="changed"><br>
 255 " If [<thread_id>] is specified, only stop in
the specified thread" +</span><br>
<br>
<br>
What do you think?<br>
</span><br>
Thanks,<br>
Serguei<br>
<br>
<br>
On 2/21/19 16:46, <a class="moz-txt-link-abbreviated"
href="mailto:serguei.spitsyn@oracle.com"
moz-do-not-send="true">serguei.spitsyn@oracle.com</a>
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:4509c259-dbc8-4f84-6ae5-fed5f3107a00@oracle.com">
<div class="moz-cite-prefix">On 2/21/19 09:19, Chris Plummer
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:f9062fa5-7fc4-f715-ec70-1810f22b116d@oracle.com">
<div class="moz-cite-prefix">On 2/21/19 2:04 AM, <a
class="moz-txt-link-abbreviated"
href="mailto:serguei.spitsyn@oracle.com"
moz-do-not-send="true">serguei.spitsyn@oracle.com</a>
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:02a7ece0-c114-1f9d-4a2a-70e31f0efd18@oracle.com">
<div class="moz-cite-prefix">Hi Chris,<br>
<br>
Not a full review yet.<br>
<br>
Just a couple of quick mismatches.<br>
<br>
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ecjplummer/8219143/webrev.01/src/jdk.jdi/share/classes/com/sun/tools/example/debug/tty/TTYResources.java.frames.html"
moz-do-not-send="true">http://cr.openjdk.java.net/~cjplummer/8219143/webrev.01/src/jdk.jdi/share/classes/com/sun/tools/example/debug/tty/TTYResources.java.frames.html</a><br>
<br>
 Is it okay that the two fragments below describe the
same differently? Do I miss anything.<br>
</div>
</blockquote>
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.<br>
</blockquote>
<br>
My question is that a couple of statements are missed in the<span
class="changed"> printstopcommandusage</span>.<br>
The help output prints these two lines which are not present
in the <span class="changed">printstopcommandusage</span>:<br>
<pre><span class="changed"> 378 " -- set a breakpoint\n" +</span>
<span class="changed"> 379 " -- if no options are given, the current list of breakpoints is printed\n" +</span></pre>
Is it intentional?<br>
<br>
<br>
<blockquote type="cite"
cite="mid:f9062fa5-7fc4-f715-ec70-1810f22b116d@oracle.com">
<blockquote type="cite"
cite="mid:02a7ece0-c114-1f9d-4a2a-70e31f0efd18@oracle.com">
<div class="moz-cite-prefix">
<pre><span class="changed"> 250 {"printstopcommandusage",</span>
<span class="changed"> 251 "Usage: stop [go|thread] [threadid <thread_id>] <at|in> <location>\n" + </span>
<span class="changed"> 252 " If \"go\" is specified, immediately resume after stopping\n" +</span>
<span class="changed"> 253 " If \"thread\" is specified, only suspend the thread we stop in\n" +</span>
<span class="changed"> 254 " If neither \"go\" nor \"thread\" are specified, suspend all threads\n" +</span>
<span class="changed"> 255 " If [threadid <thread_id>] is specified, only stop in the specified thread" +</span>
<span class="changed"> 256 " \"at\" and \"in\" have the same meaning\n" +</span>
<span class="changed"> 257 " <location> can either be a line number or a method:\n" +</span>
<span class="changed"> 258 " <class_id>:<line_number>\n" +</span>
<span class="changed"> 259 " <class_id>.<method>[(argument_type,...)]"</span>
<span class="changed"> 260 },</span></pre>
<pre><span class="changed"> 377 "stop [go|thread] [threadid <thread_id>] <at|in> <location>\n" +</span>
<span class="changed"> 378 " -- set a breakpoint\n" +</span>
<span class="changed"> 379 " -- if no options are given, the current list of breakpoints is printed\n" +</span>
<span class="changed"> 380 " -- if \"go\" is specified, immediately resume after stopping\n" + </span>
<span class="changed"> 381 " -- if \"thread\" is specified, only suspend the thread we stop in\n" +</span>
<span class="changed"> 382 " -- if neither \"go\" nor \"thread\" are specified, suspend all threads\n" +</span>
<span class="changed"> 383 " -- if [threadid <thread_id>] is specified, only stop in the specified thread\n" +</span>
<span class="changed"> 384 " -- \"at\" and \"in\" have the same meaning\n" +</span>
<span class="changed"> 385 " -- <location> can either be a line number or a method:\n" +</span>
<span class="changed"> 386 " -- <class_id>:<line_number>\n" +</span>
<span class="changed"> 387 " -- <class_id>.<method>[(argument_type,...)]\n" +</span>
</pre>
<br>
<br>
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ecjplummer/8219143/webrev.01/src/jdk.jdi/share/classes/com/sun/tools/example/debug/tty/Commands.java.frames.html"
moz-do-not-send="true">http://cr.openjdk.java.net/~cjplummer/8219143/webrev.01/src/jdk.jdi/share/classes/com/sun/tools/example/debug/tty/Commands.java.frames.html</a><br>
<br>
<pre><span class="changed">1162 * Allowed syntax:</span>
<span class="changed">1163 * stop [go|thread] <at|in> [threadid <thread_id>] <location></span>
<span class="changed">1164 * <location> can either be a line number or a method:</span>
<span class="changed">1165 * - <class id>:<line></span>
<span class="changed">1166 * - <class id>.<method>[(argument_type,...)]</span>
<span class="changed">1167 * If "go" is specified, then immediately resume after stopping. No threads are suspended.</span>
<span class="changed">1168 * If "thread" is specified, then only suspend the thread we stop in.</span>
<span class="changed">1169 * If neither "go" nor "thread" are specified, then suspend all threads.</span>
<span class="changed">1170 * If the optional [threadid <thread_id>] is specified, then only stop in the specified thread.</span>
<span class="changed">1171 * If no options are given, the current list of breakpoints is printed,</span>
<span class="changed">1172 */</span><span class="changed">
The above is not aligned with the implementation as </span><span class="changed"><at|in></span> has to be right before <location>.</pre>
</div>
</blockquote>
<blockquote type="cite"
cite="mid:02a7ece0-c114-1f9d-4a2a-70e31f0efd18@oracle.com">
<div class="moz-cite-prefix">
<pre> A dot is needed instead of the last comma.
Should this comment to match one of the above descriptions?</pre>
</div>
</blockquote>
Yes, I'll update this to look like the help output. I
missed it when moving the at/in location.<br>
</blockquote>
<br>
Okay, thanks!<br>
<br>
I'll send full review soon.<br>
<br>
Thanks,<br>
Serguei<br>
<br>
<blockquote type="cite"
cite="mid:f9062fa5-7fc4-f715-ec70-1810f22b116d@oracle.com">
<br>
thanks,<br>
<br>
Chris<br>
<blockquote type="cite"
cite="mid:02a7ece0-c114-1f9d-4a2a-70e31f0efd18@oracle.com">
<div class="moz-cite-prefix">
<pre>Thanks,
Serguei<span class="changed"></span></pre>
<br>
<br>
On 2/20/19 19:56, Chris Plummer wrote:<br>
</div>
<blockquote type="cite"
cite="mid:e3e53c7c-db86-e0d7-d52d-a9d66c880707@oracle.com">Please
review the updated webrev: <br>
<br>
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ecjplummer/8219143/webrev.01/"
moz-do-not-send="true">http://cr.openjdk.java.net/~cjplummer/8219143/webrev.01/</a>
<br>
<br>
The syntax is now: <br>
<br>
   stop [go|thread] [threadid <thread_id>]
<at|in> <location> <br>
<br>
I filed JDK-8219507 to cover BreakpointSpec.toString()
improvements, and other possible improvements to the
output when listing breakpoints. <br>
<br>
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. <br>
<br>
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. <br>
<br>
thanks, <br>
<br>
Chris <br>
<br>
[jdb] Set uncaught java.lang.Throwable <br>
[jdb] Set deferred uncaught java.lang.Throwable <br>
[jdb] Initializing jdb ... <br>
[jdb] <br>
[jdb] VM Started: > No frames on the current call
stack <br>
[jdb] <br>
[jdb] main[1] <br>
> stop in JdbStopThreadidTestTarg.brkMethod <br>
[jdb] Deferring breakpoint
JdbStopThreadidTestTarg.brkMethod. <br>
[jdb] It will be set after the class is loaded. <br>
[jdb] main[1] <br>
> run <br>
[jdb] > Set deferred breakpoint
JdbStopThreadidTestTarg.brkMethod <br>
[jdb] <br>
[jdb] Breakpoint hit: "thread=main",
JdbStopThreadidTestTarg.brkMethod(), line=80 bci=0 <br>
[jdb] 80Â Â Â Â Â Â Â } <br>
[jdb] <br>
[jdb] main[1] <br>
> threads <br>
[jdb] Group system: <br>
[jdb]Â Â (java.lang.ref.Reference$ReferenceHandler)367
Reference Handler running <br>
[jdb]Â Â (java.lang.ref.Finalizer$FinalizerThread)368
Finalizer        cond. waiting <br>
[jdb]Â Â (java.lang.Thread)369Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â
Signal Dispatcher running <br>
[jdb] Group main: <br>
[jdb]  (java.lang.Thread)1 main             running
(at breakpoint) <br>
[jdb]Â Â (JdbStopThreadidTestTarg$MyThread)482
MYTHREAD-1Â Â Â Â Â Â Â waiting in a monitor <br>
[jdb]Â Â (JdbStopThreadidTestTarg$MyThread)483
MYTHREAD-2Â Â Â Â Â Â Â waiting in a monitor <br>
[jdb]Â Â (JdbStopThreadidTestTarg$MyThread)484
MYTHREAD-3Â Â Â Â Â Â Â waiting in a monitor <br>
[jdb] Group InnocuousThreadGroup: <br>
[jdb]Â Â (jdk.internal.misc.InnocuousThread)416
Common-Cleaner   cond. waiting <br>
[jdb] main[1] <br>
> stop threadid 483 in
JdbStopThreadidTestTarg$MyThread.brkMethod <br>
[jdb] Set breakpoint
JdbStopThreadidTestTarg$MyThread.brkMethod <br>
[jdb] main[1] <br>
> cont <br>
[jdb] > <br>
[jdb] Breakpoint hit: "thread=MYTHREAD-2",
JdbStopThreadidTestTarg$MyThread.brkMethod(), line=101
bci=0 <br>
[jdb] 101Â Â Â Â Â Â Â Â Â Â Â } <br>
[jdb] <br>
[jdb] MYTHREAD-2[1] <br>
> cont <br>
[jdb] > <br>
[jdb] The application exited <br>
[jdb] <br>
<br>
<br>
On 2/20/19 2:23 PM, Chris Plummer wrote: <br>
<blockquote type="cite">On 2/20/19 2:00 PM, Alex
Menkov wrote: <br>
<blockquote type="cite">Hi Chris, <br>
<br>
- New threadid param breaks at/in clause - this
doesn't look good. <br>
Maybe it would be better to put threadid before
in/at: <br>
stop [go|thread] [threadid <thread_id>]
<at|in> <location> <br>
</blockquote>
Ok. I'll try moving it. <br>
<blockquote type="cite">This also would make
commandStop/parseBreakpointSpec logic more clear:
<br>
commandStop handles go/thread/threadid,
parseBreakpointSpec handles position (part
starting from in/at); <br>
<br>
- do you think BreakpointSpec toString should
contain info about the thread (i.e. to specified
in the breakpoint list)?; <br>
</blockquote>
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. <br>
<blockquote type="cite"> <br>
- it would be nice to add a test for the new
threadid functionality. <br>
</blockquote>
<br>
I've been working on one today. It's near
completion. <br>
<br>
Chris <br>
<br>
<blockquote type="cite"> <br>
--alex <br>
<br>
On 02/19/2019 21:57, Chris Plummer wrote: <br>
<blockquote type="cite">Hello, <br>
<br>
Please review the following: <br>
<br>
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ecjplummer/8219143/webrev.00/"
moz-do-not-send="true">http://cr.openjdk.java.net/~cjplummer/8219143/webrev.00/</a>
<br>
<a class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8219143"
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8219143</a>
<br>
<br>
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: <br>
<br>
  stop at Foo:23 <br>
<br>
You can now do: <br>
<br>
  stop at threadid 7 Foo:23 <br>
<br>
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: <br>
<br>
   stop [go|thread] <at|in> [threadid
<thread_id>] <location> <br>
<br>
It is still fully backwards compatible. <br>
<br>
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. <br>
<br>
thanks, <br>
<br>
Chris <br>
<br>
</blockquote>
</blockquote>
<br>
<br>
</blockquote>
<br>
<br>
</blockquote>
<br>
</blockquote>
<p><br>
</p>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<p><br>
</p>
</blockquote>
<p><br>
</p>
</blockquote>
<br>
</body>
</html>