[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR: 6622468 TEST_BUG: Time to retire the @debuggeeVMOptions mechanism used in the com.sun.jdi i
From: "Daniel D. Daugherty" <daniel.daugherty () oracle ! com>
Date: 2014-06-16 17:42:51
Message-ID: 539F2C9B.6000705 () oracle ! com
[Download RAW message or body]
Thanks for closing the loop on my late review.
Thumbs up!
Dan
On 6/16/14 11:39 AM, Staffan Larsen wrote:
>
> On 16 jun 2014, at 17:01, Daniel D. Daugherty
> <daniel.daugherty@oracle.com <mailto:daniel.daugherty@oracle.com>> wrote:
>
> > > updated webrev: http://cr.openjdk.java.net/~sla/6622468/webrev.2.01/
> >
> > test/com/sun/jdi/VMConnection.java
> > line 62: System.out.println("vmOpts: "+vmOpts);
> > line 67: System.out.println("javaOpts: "+javaOpts);
> > line 68: if (javaOpts != null) {
> > Indent off by one in patch view; looks like there are tabs.
> >
> > test/com/sun/jdi/connect/spi/DebugUsingCustomConnector.java
> > line 62: List launchers = vmm.launchingConnectors();
> > Indent off by one in patch view; looks like there is a tab.
>
> Yes, I noticed the tab problem too. Fixed.
>
> > test/com/sun/jdi/sde/MangleStepTest.java
> > old line 85: waitForInput();
> > Why was this line deleted?
>
> The waitForInput() method waits for input on stdin. There is no
> process that provides that input so it’s not necessary. The reason it
> works before my change is that System.in.read() returns -1 (end of
> input) when run in “othervm” mode. With the “driver” change I wanted
> the tests to be able to run in “agentvm” mode and there
> System.in.read() blocks. I haven’t delved into the details of why, but
> since there is no reason to wait for input here I simply removed the
> line. I should have mentioned this in the review request.
>
> Thanks,
> /Staffan
>
>
> >
> > No comments on the other files.
> >
> > Dan
> >
> >
> > On 6/16/14 5:59 AM, Staffan Larsen wrote:
> > > I would appreciate a Review of this change.
> > >
> > > Thanks,
> > > /Staffan
> > >
> > > On 11 jun 2014, at 10:00, Staffan Larsen <staffan.larsen@oracle.com
> > > <mailto:staffan.larsen@oracle.com>> wrote:
> > >
> > > > I realized that the code in VMConnection does not take into account
> > > > the test.java.opts property as it should.
> > > >
> > > > updated webrev:
> > > > http://cr.openjdk.java.net/~sla/6622468/webrev.2.01/
> > > > <http://cr.openjdk.java.net/%7Esla/6622468/webrev.2.01/> (only
> > > > VMConnection changed)
> > > >
> > > > Thanks,
> > > > /Staffan
> > > >
> > > > On 10 jun 2014, at 13:59, Staffan Larsen <staffan.larsen@oracle.com
> > > > <mailto:staffan.larsen@oracle.com>> wrote:
> > > >
> > > > >
> > > > > On 10 jun 2014, at 11:44,serguei.spitsyn@oracle.com
> > > > > <mailto:serguei.spitsyn@oracle.com>wrote:
> > > > >
> > > > > > Staffan,
> > > > > >
> > > > > > It looks good, just one comment.
> > > > > >
> > > > > > test/com/sun/jdi/VMConnection.java
> > > > > > 61 String vmOpts = System.getProperty("test.vm.opts");
> > > > > > 62 if (vmOpts != null) {
> > > > > > 63 retVal += System.getProperty("test.vm.opts");
> > > > > > I wonder why not this:
> > > > > > 63 retVal += vmOpts;
> > > > > Uh. Yeah, I wonder that, too. Fixed. :-)
> > > > >
> > > > > Thanks,
> > > > > /Staffan
> > > > >
> > > > >
> > > > >
> > > > > >
> > > > > > Thanks,
> > > > > > Serguei
> > > > > >
> > > > > >
> > > > > > On 6/10/14 12:58 AM, Staffan Larsen wrote:
> > > > > > > This is a new take on this old bug. Since my previous attempt [0], \
> > > > > > > jtreg has been update with a “driver” feature and this is exactly what \
> > > > > > > these tests need. Specifying “@run driver” (instead of “@run main”) \
> > > > > > > will launch the test with no vm arguments. Whatever arguments were \
> > > > > > > specified in -vmoptions to jtreg will be available in the System \
> > > > > > > property test.vm.opts and the test code can use those arguments when \
> > > > > > > launching other processes.
> > > > > > > For the JDI tests this is a very good match. The tests run two \
> > > > > > > processes: one debugger and one debuggee. It is really the debuggee \
> > > > > > > that is being tested, the the debugger is just driving the testing. So \
> > > > > > > it is the debuggee that should be invoked with the specified \
> > > > > > > -vmoptions.
> > > > > > > In this change I’ve changed all debuggers to be launched with “@run \
> > > > > > > driver” and all debuggees to be launched using the test.vm.opts \
> > > > > > > options. This will remove the need to the esoteric @debuggeeVMOptions \
> > > > > > > file that was previously used to pass arguments to the debuggee.
> > > > > > > webrev:http://cr.openjdk.java.net/~sla/6622468/webrev.2.00/
> > > > > > > bug:https://bugs.openjdk.java.net/browse/JDK-6622468
> > > > > > >
> > > > > > > The webrev is very boring to read, probably best to read the diff file \
> > > > > > > directly. test/com/sun/jdi/VMConnection.java has the only substantial \
> > > > > > > change.
> > > > > > > I have run this through JPRT with no failures.
> > > > > > >
> > > > > > > Thanks,
> > > > > > > /Staffan
> > > > > > >
> > > > > > >
> > > > > > > [0]http://mail.openjdk.java.net/pipermail/serviceability-dev/2013-August/011325.html
> > > > > > >
> > > >
> > >
> >
>
[Attachment #3 (text/html)]
<html>
<head>
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<tt>Thanks for closing the loop on my late review.<br>
<br>
Thumbs up!<br>
<br>
Dan<br>
<br>
<br>
</tt>
<div class="moz-cite-prefix">On 6/16/14 11:39 AM, Staffan Larsen
wrote:<br>
</div>
<blockquote
cite="mid:6DDA690E-2070-4325-B7A2-CAF24C181718@oracle.com"
type="cite">
<meta http-equiv="Content-Type" content="text/html;
charset=windows-1252">
<br>
<div>
<div>On 16 jun 2014, at 17:01, Daniel D. Daugherty <<a
moz-do-not-send="true"
href="mailto:daniel.daugherty@oracle.com">daniel.daugherty@oracle.com</a>>
wrote:</div>
<br class="Apple-interchange-newline">
<blockquote type="cite">
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
<div bgcolor="#FFFFFF" text="#000000"> <tt>> updated
webrev: <a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Esla/6622468/webrev.2.01/">http://cr.openjdk.java.net/~sla/6622468/webrev.2.01/</a><br>
<br>
test/com/sun/jdi/VMConnection.java<br>
line 62: System.out.println("vmOpts:
"+vmOpts);<br>
line 67: System.out.println("javaOpts:
"+javaOpts);<br>
line 68: if (javaOpts != null) {<br>
Indent off by one in patch view; looks like there
are tabs.<br>
<br>
test/com/sun/jdi/connect/spi/DebugUsingCustomConnector.java<br>
line 62: List launchers =
vmm.launchingConnectors();<br>
Indent off by one in patch view; looks like there
is a tab.<br>
</tt></div>
</blockquote>
<div><br>
</div>
<div>Yes, I noticed the tab problem too. Fixed.</div>
<br>
<blockquote type="cite">
<div bgcolor="#FFFFFF" text="#000000"><tt>
test/com/sun/jdi/sde/MangleStepTest.java<br>
old line 85: waitForInput();<br>
Why was this line deleted?<br>
</tt></div>
</blockquote>
<div><br>
</div>
<div>The waitForInput() method waits for input on stdin. There
is no process that provides that input so it’s not necessary.
The reason it works before my change is that System.in.read()
returns -1 (end of input) when run in “othervm” mode. With the
“driver” change I wanted the tests to be able to run in
“agentvm” mode and there System.in.read() blocks. I haven’t
delved into the details of why, but since there is no reason
to wait for input here I simply removed the line. I should
have mentioned this in the review request.</div>
<div><br>
</div>
<div>
<div>Thanks,</div>
<div>/Staffan</div>
</div>
<div><br>
</div>
<br>
<blockquote type="cite">
<div bgcolor="#FFFFFF" text="#000000"><tt> <br>
No comments on the other files.<br>
<br>
Dan<br>
<br>
<br>
</tt>
<div class="moz-cite-prefix">On 6/16/14 5:59 AM, Staffan
Larsen wrote:<br>
</div>
<blockquote
cite="mid:E074C2D0-1097-4EBC-B335-F8EC8F6A6C86@oracle.com"
type="cite">
<meta http-equiv="Content-Type" content="text/html;
charset=windows-1252">
I would appreciate a Review of this change.
<div><br>
</div>
<div>Thanks,</div>
<div>/Staffan<br>
<div><br>
<div>
<div>On 11 jun 2014, at 10:00, Staffan Larsen <<a
moz-do-not-send="true"
\
href="mailto:staffan.larsen@oracle.com">staffan.larsen@oracle.com</a>>
wrote:</div>
<br class="Apple-interchange-newline">
<blockquote type="cite">
<meta http-equiv="Content-Type"
content="text/html; charset=windows-1252">
<div style="word-wrap: break-word;
-webkit-nbsp-mode: space; -webkit-line-break:
after-white-space;">I realized that the code in
VMConnection does not take into account the
test.java.opts property as it should.
<div><br>
</div>
<div>updated webrev: <a moz-do-not-send="true"
\
href="http://cr.openjdk.java.net/%7Esla/6622468/webrev.2.01/">http://cr.openjdk.java.net/~sla/6622468/webrev.2.01/</a>
(only VMConnection changed)</div>
<div><br>
</div>
<div>Thanks,</div>
<div>/Staffan</div>
<div><br>
<div>
<div>On 10 jun 2014, at 13:59, Staffan
Larsen <<a moz-do-not-send="true"
\
href="mailto:staffan.larsen@oracle.com">staffan.larsen@oracle.com</a>>
wrote:</div>
<br class="Apple-interchange-newline">
<blockquote type="cite">
<div style="font-family: Helvetica;
font-size: 16px; font-style: normal;
font-variant: normal; font-weight:
normal; letter-spacing: normal;
line-height: normal; orphans: auto;
text-align: start; text-indent: 0px;
text-transform: none; white-space:
normal; widows: auto; word-spacing: 0px;
-webkit-text-stroke-width: 0px;"><br
class="Apple-interchange-newline">
On 10 jun 2014, at 11:44,<span
class="Apple-converted-space"> </span><a
moz-do-not-send="true"
\
href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a><span
class="Apple-converted-space"> </span>wrote:</div>
<br class="Apple-interchange-newline"
style="font-family: Helvetica;
font-size: 16px; font-style: normal;
font-variant: normal; font-weight:
normal; letter-spacing: normal;
line-height: normal; orphans: auto;
text-align: start; text-indent: 0px;
text-transform: none; white-space:
normal; widows: auto; word-spacing: 0px;
-webkit-text-stroke-width: 0px;">
<blockquote type="cite"
style="font-family: Helvetica;
font-size: 16px; font-style: normal;
font-variant: normal; font-weight:
normal; letter-spacing: normal;
line-height: normal; orphans: auto;
text-align: start; text-indent: 0px;
text-transform: none; white-space:
normal; widows: auto; word-spacing: 0px;
-webkit-text-stroke-width: 0px;">
<div text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">Staffan,<br>
<br>
It looks good, just one comment.<br>
<br>
test/com/sun/jdi/VMConnection.java<br>
<pre><span class="changed"> 61 String \
vmOpts = System.getProperty("test.vm.opts");</span> <span class="changed"> 62 \
if (vmOpts != null) {</span> <span class="changed"> 63 retVal += \
System.getProperty("test.vm.opts");</span></pre> I wonder why not this:<br>
<pre><span class="changed"> 63 \
retVal += vmOpts;</span></pre> </div>
</div>
</blockquote>
<div style="font-family: Helvetica;
font-size: 16px; font-style: normal;
font-variant: normal; font-weight:
normal; letter-spacing: normal;
line-height: normal; orphans: auto;
text-align: start; text-indent: 0px;
text-transform: none; white-space:
normal; widows: auto; word-spacing: 0px;
-webkit-text-stroke-width: 0px;">Uh.
Yeah, I wonder that, too. Fixed. :-)</div>
<div style="font-family: Helvetica;
font-size: 16px; font-style: normal;
font-variant: normal; font-weight:
normal; letter-spacing: normal;
line-height: normal; orphans: auto;
text-align: start; text-indent: 0px;
text-transform: none; white-space:
normal; widows: auto; word-spacing: 0px;
-webkit-text-stroke-width: 0px;"><br>
</div>
<div style="font-family: Helvetica;
font-size: 16px; font-style: normal;
font-variant: normal; font-weight:
normal; letter-spacing: normal;
line-height: normal; orphans: auto;
text-align: start; text-indent: 0px;
text-transform: none; white-space:
normal; widows: auto; word-spacing: 0px;
-webkit-text-stroke-width: 0px;">
<div>Thanks,</div>
<div>/Staffan</div>
</div>
<div style="font-family: Helvetica;
font-size: 16px; font-style: normal;
font-variant: normal; font-weight:
normal; letter-spacing: normal;
line-height: normal; orphans: auto;
text-align: start; text-indent: 0px;
text-transform: none; white-space:
normal; widows: auto; word-spacing: 0px;
-webkit-text-stroke-width: 0px;"><br>
</div>
<div style="font-family: Helvetica;
font-size: 16px; font-style: normal;
font-variant: normal; font-weight:
normal; letter-spacing: normal;
line-height: normal; orphans: auto;
text-align: start; text-indent: 0px;
text-transform: none; white-space:
normal; widows: auto; word-spacing: 0px;
-webkit-text-stroke-width: 0px;"><br>
</div>
<br style="font-family: Helvetica;
font-size: 16px; font-style: normal;
font-variant: normal; font-weight:
normal; letter-spacing: normal;
line-height: normal; orphans: auto;
text-align: start; text-indent: 0px;
text-transform: none; white-space:
normal; widows: auto; word-spacing: 0px;
-webkit-text-stroke-width: 0px;">
<blockquote type="cite"
style="font-family: Helvetica;
font-size: 16px; font-style: normal;
font-variant: normal; font-weight:
normal; letter-spacing: normal;
line-height: normal; orphans: auto;
text-align: start; text-indent: 0px;
text-transform: none; white-space:
normal; widows: auto; word-spacing: 0px;
-webkit-text-stroke-width: 0px;">
<div text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix"><br>
Thanks,<br>
Serguei<br>
<br>
<br>
On 6/10/14 12:58 AM, Staffan Larsen
wrote:<br>
</div>
<blockquote
\
cite="mid:C3F3CC9C-033C-434F-A61B-669B0214E8A3@oracle.com" type="cite">
<pre wrap="">This is a new take on this old bug. \
Since my previous attempt [0], jtreg has been update with a “driver” feature and this \
is exactly what these tests need. Specifying “@run driver” (instead of “@run main”) \
will launch the test with no vm arguments. Whatever arguments were specified in \
-vmoptions to jtreg will be available in the System property test.vm.opts and the \
test code can use those arguments when launching other processes.
For the JDI tests this is a very good match. The tests run two processes: one \
debugger and one debuggee. It is really the debuggee that is being tested, the the \
debugger is just driving the testing. So it is the debuggee that should be invoked \
with the specified -vmoptions.
In this change I’ve changed all debuggers to be launched with “@run driver” and all \
debuggees to be launched using the test.vm.opts options. This will remove the need to \
the esoteric @debuggeeVMOptions file that was previously used to pass arguments to \
the debuggee.
webrev: <a moz-do-not-send="true" class="moz-txt-link-freetext" \
href="http://cr.openjdk.java.net/%7Esla/6622468/webrev.2.00/">http://cr.openjdk.java.net/~sla/6622468/webrev.2.00/</a>
bug: <a moz-do-not-send="true" class="moz-txt-link-freetext" \
href="https://bugs.openjdk.java.net/browse/JDK-6622468">https://bugs.openjdk.java.net/browse/JDK-6622468</a>
The webrev is very boring to read, probably best to read the diff file directly. \
test/com/sun/jdi/VMConnection.java has the only substantial change.
I have run this through JPRT with no failures.
Thanks,
/Staffan
[0] <a moz-do-not-send="true" class="moz-txt-link-freetext" \
href="http://mail.openjdk.java.net/pipermail/serviceability-dev/2013-August/011325.htm \
l">http://mail.openjdk.java.net/pipermail/serviceability-dev/2013-August/011325.html</a></pre>
</blockquote>
</div>
</blockquote>
</blockquote>
</div>
<br>
</div>
</div>
</blockquote>
</div>
<br>
</div>
</div>
</blockquote>
<br>
</div>
</blockquote>
</div>
<br>
</blockquote>
<br>
</body>
</html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic