Re: [9] RFR (M) 8054386: Allow Java debugging when CDS is enabled
On 5/06/2015 9:32 AM, Chris Plummer wrote:
Hi David,
Here's an updated webrev:
http://cr.openjdk.java.net/~cjplummer/8054386/webrev.04/
Thanks - should have said updated webrev not necessary :)
Looks good.
David
thanks,
Chris
On 6/3/15 11:29 PM, David Holmes wrote:
Hi Chris,
Hotspot change is good.
Only a couple of style nits with the tests (where are our Java style
guidelines ???). Taking CDSJDITest.java as an example:
If you are okay with this line length:
115 public static OutputAnalyzer executeAndLog(ProcessBuilder pb,
String logName) throws Exception {
then you can remove a number of line breaks in the headers of other
functions. (I don't follow the 70-80 char line length dogma ;) )
If you do break a header of a function the { still stays on the same
line as the last header component ie:
private static void addToClassList(PrintStream ps, String classes[])
throws IOException {
not:
139 private static void addToClassList(PrintStream ps, String
classes[])
140 throws IOException
141 {
Cheers,
David
On 2/06/2015 5:36 PM, Chris Plummer wrote:
[Adding core-libs-dev@openjdk.java.net since this update includes
changes to jdk/test library code]
Please review the updated webrev:
Webrev: http://cr.openjdk.java.net/~cjplummer/8054386/webrev.02/
Bug: https://bugs.openjdk.java.net/browse/JDK-8054386
There were concerns about the new hotspot tests referencing jdk tests.
One concern was that if the jdk tests change, they could break the
hotspot tests, and this might initially go undetected. The other concern
is that if the jdk and hotspot tests are placed in separate test
bundles, then it would not be possible to run the hotspot tests.
Because of these concerns, I moved the tests that were in
hotspot/test/runtime/CDSJDITests to instead be in
jdk/test/com/sun/jdi/CDSJDITests. There was a slight renaming of the
tests in the process. Also, I had to update the jdk version of
ProcessTool.java to include the createJavaProcessBuilder() variant that
is in the hotspot version of ProcessTool.java.
Lastly, in CDSJITTest.java I changed:
OutputAnalyzer output = new OutputAnalyzer(pb.start());
to instead be:
OutputAnalyzer output = ProcessTools.executeProcess(pb);
I had to do this since the jdk version of the OutputAnalyzer constructor
is not public. The 1st version is what is commonly used in hostspot
tests, and the 2nd version is what is commonly used in jdk tests. I
decided to adopt the jdk way rather than make the OutputAnalyzer
constructors public, although this will probably happen eventually when
the two versions are unified.
thanks,
Chris
On 5/19/15 7:25 AM, Chris Plummer wrote:
Hi,
Please review the following changes for allowing java debugging when
CDS is enabled.
Webrev:http://cr.openjdk.java.net/~cjplummer/8054386/webrev.01/
Bug:https://bugs.openjdk.java.net/browse/JDK-8054386
The VM changes are simple. I removed the check that prevents debugging
with CDS enabled, and added logic that will map the CDS archive RW
when debugging is enabled.
The tests are a bit more complex. There are a bunch of existing JDI
tests for testing debugging support. Rather than start from scratch or
clone them, I instead just wrote wrapper tests that put the relevant
JDI test classes in the archive, and then invoke the JDI test. I did
this for 3 of the JDI tests. If you feel there are others that would
be good candidates, I'd be happy to add them. I'm looking for ones
that would result in modification of the RO class metadata, such as
setting a breakpoint (for which I already added two tests).
Testing done:
-Using JPRT to run the new jtreg tests on all platforms.
-Using JPRT to run all jtreg runtime tests on linux x86 and x_64.
-Regular JPRT -testset hotspot run
-Putting the JCK JVMTI tests in the archive and then running them.
-Putting the nsk jdb, jdwp, jvmti, and jdi tests in the archive and
then running them.
-Putting a simple test class in the archive and then setting a
breakpoint on it using jdb
Some of the above testing resulted in the discovery of bugs that still
need to be addressed: JDK-8078644, JDK-8078730, and JDK-8079181.
I also verified that without the change to map the archive RW, the
above testing resulted in a SEGV, which is what you would expect (and
actually want to see to prove that the testing is effective).
thanks,
Chris
Re: [9] RFR (M) 8054386: Allow Java debugging when CDS is enabled
I guess, there is no need to re-review.
It looks good anyway.
Thanks,
Serguei
On 6/4/15 4:32 PM, Chris Plummer wrote:
Hi David,
Here's an updated webrev:
http://cr.openjdk.java.net/~cjplummer/8054386/webrev.04/
thanks,
Chris
On 6/3/15 11:29 PM, David Holmes wrote:
Hi Chris,
Hotspot change is good.
Only a couple of style nits with the tests (where are our Java style
guidelines ???). Taking CDSJDITest.java as an example:
If you are okay with this line length:
115 public static OutputAnalyzer executeAndLog(ProcessBuilder
pb, String logName) throws Exception {
then you can remove a number of line breaks in the headers of other
functions. (I don't follow the 70-80 char line length dogma ;) )
If you do break a header of a function the { still stays on the same
line as the last header component ie:
private static void addToClassList(PrintStream ps, String
classes[])
throws IOException {
not:
139 private static void addToClassList(PrintStream ps, String
classes[])
140 throws IOException
141 {
Cheers,
David
On 2/06/2015 5:36 PM, Chris Plummer wrote:
[Adding core-libs-dev@openjdk.java.net since this update includes
changes to jdk/test library code]
Please review the updated webrev:
Webrev: http://cr.openjdk.java.net/~cjplummer/8054386/webrev.02/
Bug: https://bugs.openjdk.java.net/browse/JDK-8054386
There were concerns about the new hotspot tests referencing jdk tests.
One concern was that if the jdk tests change, they could break the
hotspot tests, and this might initially go undetected. The other
concern
is that if the jdk and hotspot tests are placed in separate test
bundles, then it would not be possible to run the hotspot tests.
Because of these concerns, I moved the tests that were in
hotspot/test/runtime/CDSJDITests to instead be in
jdk/test/com/sun/jdi/CDSJDITests. There was a slight renaming of the
tests in the process. Also, I had to update the jdk version of
ProcessTool.java to include the createJavaProcessBuilder() variant that
is in the hotspot version of ProcessTool.java.
Lastly, in CDSJITTest.java I changed:
OutputAnalyzer output = new OutputAnalyzer(pb.start());
to instead be:
OutputAnalyzer output = ProcessTools.executeProcess(pb);
I had to do this since the jdk version of the OutputAnalyzer
constructor
is not public. The 1st version is what is commonly used in hostspot
tests, and the 2nd version is what is commonly used in jdk tests. I
decided to adopt the jdk way rather than make the OutputAnalyzer
constructors public, although this will probably happen eventually when
the two versions are unified.
thanks,
Chris
On 5/19/15 7:25 AM, Chris Plummer wrote:
Hi,
Please review the following changes for allowing java debugging when
CDS is enabled.
Webrev:http://cr.openjdk.java.net/~cjplummer/8054386/webrev.01/
Bug:https://bugs.openjdk.java.net/browse/JDK-8054386
The VM changes are simple. I removed the check that prevents debugging
with CDS enabled, and added logic that will map the CDS archive RW
when debugging is enabled.
The tests are a bit more complex. There are a bunch of existing JDI
tests for testing debugging support. Rather than start from scratch or
clone them, I instead just wrote wrapper tests that put the relevant
JDI test classes in the archive, and then invoke the JDI test. I did
this for 3 of the JDI tests. If you feel there are others that would
be good candidates, I'd be happy to add them. I'm looking for ones
that would result in modification of the RO class metadata, such as
setting a breakpoint (for which I already added two tests).
Testing done:
-Using JPRT to run the new jtreg tests on all platforms.
-Using JPRT to run all jtreg runtime tests on linux x86 and x_64.
-Regular JPRT -testset hotspot run
-Putting the JCK JVMTI tests in the archive and then running them.
-Putting the nsk jdb, jdwp, jvmti, and jdi tests in the archive and
then running them.
-Putting a simple test class in the archive and then setting a
breakpoint on it using jdb
Some of the above testing resulted in the discovery of bugs that still
need to be addressed: JDK-8078644, JDK-8078730, and JDK-8079181.
I also verified that without the change to map the archive RW, the
above testing resulted in a SEGV, which is what you would expect (and
actually want to see to prove that the testing is effective).
thanks,
Chris
Re: [9] RFR (M) 8054386: Allow Java debugging when CDS is enabled
Hi David,
Here's an updated webrev:
http://cr.openjdk.java.net/~cjplummer/8054386/webrev.04/
thanks,
Chris
On 6/3/15 11:29 PM, David Holmes wrote:
Hi Chris,
Hotspot change is good.
Only a couple of style nits with the tests (where are our Java style
guidelines ???). Taking CDSJDITest.java as an example:
If you are okay with this line length:
115 public static OutputAnalyzer executeAndLog(ProcessBuilder pb,
String logName) throws Exception {
then you can remove a number of line breaks in the headers of other
functions. (I don't follow the 70-80 char line length dogma ;) )
If you do break a header of a function the { still stays on the same
line as the last header component ie:
private static void addToClassList(PrintStream ps, String classes[])
throws IOException {
not:
139 private static void addToClassList(PrintStream ps, String
classes[])
140 throws IOException
141 {
Cheers,
David
On 2/06/2015 5:36 PM, Chris Plummer wrote:
[Adding core-libs-dev@openjdk.java.net since this update includes
changes to jdk/test library code]
Please review the updated webrev:
Webrev: http://cr.openjdk.java.net/~cjplummer/8054386/webrev.02/
Bug: https://bugs.openjdk.java.net/browse/JDK-8054386
There were concerns about the new hotspot tests referencing jdk tests.
One concern was that if the jdk tests change, they could break the
hotspot tests, and this might initially go undetected. The other concern
is that if the jdk and hotspot tests are placed in separate test
bundles, then it would not be possible to run the hotspot tests.
Because of these concerns, I moved the tests that were in
hotspot/test/runtime/CDSJDITests to instead be in
jdk/test/com/sun/jdi/CDSJDITests. There was a slight renaming of the
tests in the process. Also, I had to update the jdk version of
ProcessTool.java to include the createJavaProcessBuilder() variant that
is in the hotspot version of ProcessTool.java.
Lastly, in CDSJITTest.java I changed:
OutputAnalyzer output = new OutputAnalyzer(pb.start());
to instead be:
OutputAnalyzer output = ProcessTools.executeProcess(pb);
I had to do this since the jdk version of the OutputAnalyzer constructor
is not public. The 1st version is what is commonly used in hostspot
tests, and the 2nd version is what is commonly used in jdk tests. I
decided to adopt the jdk way rather than make the OutputAnalyzer
constructors public, although this will probably happen eventually when
the two versions are unified.
thanks,
Chris
On 5/19/15 7:25 AM, Chris Plummer wrote:
Hi,
Please review the following changes for allowing java debugging when
CDS is enabled.
Webrev:http://cr.openjdk.java.net/~cjplummer/8054386/webrev.01/
Bug:https://bugs.openjdk.java.net/browse/JDK-8054386
The VM changes are simple. I removed the check that prevents debugging
with CDS enabled, and added logic that will map the CDS archive RW
when debugging is enabled.
The tests are a bit more complex. There are a bunch of existing JDI
tests for testing debugging support. Rather than start from scratch or
clone them, I instead just wrote wrapper tests that put the relevant
JDI test classes in the archive, and then invoke the JDI test. I did
this for 3 of the JDI tests. If you feel there are others that would
be good candidates, I'd be happy to add them. I'm looking for ones
that would result in modification of the RO class metadata, such as
setting a breakpoint (for which I already added two tests).
Testing done:
-Using JPRT to run the new jtreg tests on all platforms.
-Using JPRT to run all jtreg runtime tests on linux x86 and x_64.
-Regular JPRT -testset hotspot run
-Putting the JCK JVMTI tests in the archive and then running them.
-Putting the nsk jdb, jdwp, jvmti, and jdi tests in the archive and
then running them.
-Putting a simple test class in the archive and then setting a
breakpoint on it using jdb
Some of the above testing resulted in the discovery of bugs that still
need to be addressed: JDK-8078644, JDK-8078730, and JDK-8079181.
I also verified that without the change to map the archive RW, the
above testing resulted in a SEGV, which is what you would expect (and
actually want to see to prove that the testing is effective).
thanks,
Chris
Re: [9] RFR (M) 8054386: Allow Java debugging when CDS is enabled
Hi Chris,
Hotspot change is good.
Only a couple of style nits with the tests (where are our Java style
guidelines ???). Taking CDSJDITest.java as an example:
If you are okay with this line length:
115 public static OutputAnalyzer executeAndLog(ProcessBuilder pb,
String logName) throws Exception {
then you can remove a number of line breaks in the headers of other
functions. (I don't follow the 70-80 char line length dogma ;) )
If you do break a header of a function the { still stays on the same
line as the last header component ie:
private static void addToClassList(PrintStream ps, String classes[])
throws IOException {
not:
139 private static void addToClassList(PrintStream ps, String
classes[])
140 throws IOException
141 {
Cheers,
David
On 2/06/2015 5:36 PM, Chris Plummer wrote:
[Adding core-libs-dev@openjdk.java.net since this update includes
changes to jdk/test library code]
Please review the updated webrev:
Webrev: http://cr.openjdk.java.net/~cjplummer/8054386/webrev.02/
Bug: https://bugs.openjdk.java.net/browse/JDK-8054386
There were concerns about the new hotspot tests referencing jdk tests.
One concern was that if the jdk tests change, they could break the
hotspot tests, and this might initially go undetected. The other concern
is that if the jdk and hotspot tests are placed in separate test
bundles, then it would not be possible to run the hotspot tests.
Because of these concerns, I moved the tests that were in
hotspot/test/runtime/CDSJDITests to instead be in
jdk/test/com/sun/jdi/CDSJDITests. There was a slight renaming of the
tests in the process. Also, I had to update the jdk version of
ProcessTool.java to include the createJavaProcessBuilder() variant that
is in the hotspot version of ProcessTool.java.
Lastly, in CDSJITTest.java I changed:
OutputAnalyzer output = new OutputAnalyzer(pb.start());
to instead be:
OutputAnalyzer output = ProcessTools.executeProcess(pb);
I had to do this since the jdk version of the OutputAnalyzer constructor
is not public. The 1st version is what is commonly used in hostspot
tests, and the 2nd version is what is commonly used in jdk tests. I
decided to adopt the jdk way rather than make the OutputAnalyzer
constructors public, although this will probably happen eventually when
the two versions are unified.
thanks,
Chris
On 5/19/15 7:25 AM, Chris Plummer wrote:
Hi,
Please review the following changes for allowing java debugging when
CDS is enabled.
Webrev:http://cr.openjdk.java.net/~cjplummer/8054386/webrev.01/
Bug:https://bugs.openjdk.java.net/browse/JDK-8054386
The VM changes are simple. I removed the check that prevents debugging
with CDS enabled, and added logic that will map the CDS archive RW
when debugging is enabled.
The tests are a bit more complex. There are a bunch of existing JDI
tests for testing debugging support. Rather than start from scratch or
clone them, I instead just wrote wrapper tests that put the relevant
JDI test classes in the archive, and then invoke the JDI test. I did
this for 3 of the JDI tests. If you feel there are others that would
be good candidates, I'd be happy to add them. I'm looking for ones
that would result in modification of the RO class metadata, such as
setting a breakpoint (for which I already added two tests).
Testing done:
-Using JPRT to run the new jtreg tests on all platforms.
-Using JPRT to run all jtreg runtime tests on linux x86 and x_64.
-Regular JPRT -testset hotspot run
-Putting the JCK JVMTI tests in the archive and then running them.
-Putting the nsk jdb, jdwp, jvmti, and jdi tests in the archive and
then running them.
-Putting a simple test class in the archive and then setting a
breakpoint on it using jdb
Some of the above testing resulted in the discovery of bugs that still
need to be addressed: JDK-8078644, JDK-8078730, and JDK-8079181.
I also verified that without the change to map the archive RW, the
above testing resulted in a SEGV, which is what you would expect (and
actually want to see to prove that the testing is effective).
thanks,
Chris
Re: [9] RFR (M) 8054386: Allow Java debugging when CDS is enabled
Hi Chris, I've replied with a thumbs up on another thread. Thanks, Serguei On 6/3/15 4:23 PM, Chris Plummer wrote: Hi Serguei, I'll take care of the rename. Can I also put you down for the ProcessTool.java changes, which are officially being reviewed on another thread: http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-June/033892.html thanks, Chris On 6/3/15 1:40 AM, serguei.spit...@oracle.com wrote: Chris, It looks good in general. I'd suggest to rename the folder: || test/com/sun/jdi/CDSJDITests to: test/com/sun/jdi/cds There is no need to spell JDI as it is already a sub-folder of the com/sun/jdi and there is no need to spell Tests too as it is in the test repo. Also, all the folders are normally named in the lower case. Thanks, Serguei On 6/2/15 8:25 PM, Chris Plummer wrote: Ok, I'm going to keep this as one webrev, but I did create JDK-8081771 for the ProcessTool.java changes: https://bugs.openjdk.java.net/browse/JDK-8081771 I will commit ProcessTool.java under JDK-8081771, and the rest of the changes under JDK-8054386. Both will then be pushed together. I also started a new thread for the review of JDK-8081771: http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2015-June/014930.html http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-June/033892.html thanks, Chris On 6/2/15 11:55 AM, Chris Plummer wrote: I'm going to have to separate out the ProcessTool.java changes into a separate changeset (and CR). In the meantime, feel free to review what I have below. The code won't be changing at all when I separate out the ProcessTool.java changes. thanks, Chris On 6/2/15 12:36 AM, Chris Plummer wrote: [Adding core-libs-dev@openjdk.java.net since this update includes changes to jdk/test library code] Please review the updated webrev: Webrev: http://cr.openjdk.java.net/~cjplummer/8054386/webrev.02/ Bug: https://bugs.openjdk.java.net/browse/JDK-8054386 There were concerns about the new hotspot tests referencing jdk tests. One concern was that if the jdk tests change, they could break the hotspot tests, and this might initially go undetected. The other concern is that if the jdk and hotspot tests are placed in separate test bundles, then it would not be possible to run the hotspot tests. Because of these concerns, I moved the tests that were in hotspot/test/runtime/CDSJDITests to instead be in jdk/test/com/sun/jdi/CDSJDITests. There was a slight renaming of the tests in the process. Also, I had to update the jdk version of ProcessTool.java to include the createJavaProcessBuilder() variant that is in the hotspot version of ProcessTool.java. Lastly, in CDSJITTest.java I changed: OutputAnalyzer output = new OutputAnalyzer(pb.start()); to instead be: OutputAnalyzer output = ProcessTools.executeProcess(pb); I had to do this since the jdk version of the OutputAnalyzer constructor is not public. The 1st version is what is commonly used in hostspot tests, and the 2nd version is what is commonly used in jdk tests. I decided to adopt the jdk way rather than make the OutputAnalyzer constructors public, although this will probably happen eventually when the two versions are unified. thanks, Chris On 5/19/15 7:25 AM, Chris Plummer wrote: Hi, Please review the following changes for allowing java debugging when CDS is enabled. Webrev:http://cr.openjdk.java.net/~cjplummer/8054386/webrev.01/ Bug:https://bugs.openjdk.java.net/browse/JDK-8054386 The VM changes are simple. I removed the check that prevents debugging with CDS enabled, and added logic that will map the CDS archive RW when debugging is enabled. The tests are a bit more complex. There are a bunch of existing JDI tests for testing debugging support. Rather than start from scratch or clone them, I instead just wrote wrapper tests that put the relevant JDI test classes in the archive, and then invoke the JDI test. I did this for 3 of the JDI tests. If you feel there are others that would be good candidates, I'd be happy to add them. I'm looking for ones that would result in modification of the RO class metadata, such as setting a breakpoint (for which I already added two tests). Testing done: -Using JPRT to run the new jtreg tests on all platforms. -Using JPRT to run all jtreg runtime tests on linux x86 and x_64. -Regular JPRT -testset hotspot run -Putting the JCK JVMTI tests in the archive and then running them. -Putting the nsk jdb, jdwp, jvmti, and jdi tests in the archive and then running them. -Putting a simple test class in the archive and then setting a breakpoint on it using jdb Some of the above testing resulted in the discovery of bugs that still need to be addressed: JDK-8078644, JDK-8078730, and JDK-8079181. I also verified that without the change to map the archive RW, the above testing resulted in a SEGV, which is what you would expect (and actually want to see to prove that the testing is effective). thanks,
Re: [9] RFR (M) 8054386: Allow Java debugging when CDS is enabled
Hi Serguei, I'll take care of the rename. Can I also put you down for the ProcessTool.java changes, which are officially being reviewed on another thread: http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-June/033892.html thanks, Chris On 6/3/15 1:40 AM, serguei.spit...@oracle.com wrote: Chris, It looks good in general. I'd suggest to rename the folder: || test/com/sun/jdi/CDSJDITests to: test/com/sun/jdi/cds There is no need to spell JDI as it is already a sub-folder of the com/sun/jdi and there is no need to spell Tests too as it is in the test repo. Also, all the folders are normally named in the lower case. Thanks, Serguei On 6/2/15 8:25 PM, Chris Plummer wrote: Ok, I'm going to keep this as one webrev, but I did create JDK-8081771 for the ProcessTool.java changes: https://bugs.openjdk.java.net/browse/JDK-8081771 I will commit ProcessTool.java under JDK-8081771, and the rest of the changes under JDK-8054386. Both will then be pushed together. I also started a new thread for the review of JDK-8081771: http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2015-June/014930.html http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-June/033892.html thanks, Chris On 6/2/15 11:55 AM, Chris Plummer wrote: I'm going to have to separate out the ProcessTool.java changes into a separate changeset (and CR). In the meantime, feel free to review what I have below. The code won't be changing at all when I separate out the ProcessTool.java changes. thanks, Chris On 6/2/15 12:36 AM, Chris Plummer wrote: [Adding core-libs-dev@openjdk.java.net since this update includes changes to jdk/test library code] Please review the updated webrev: Webrev: http://cr.openjdk.java.net/~cjplummer/8054386/webrev.02/ Bug: https://bugs.openjdk.java.net/browse/JDK-8054386 There were concerns about the new hotspot tests referencing jdk tests. One concern was that if the jdk tests change, they could break the hotspot tests, and this might initially go undetected. The other concern is that if the jdk and hotspot tests are placed in separate test bundles, then it would not be possible to run the hotspot tests. Because of these concerns, I moved the tests that were in hotspot/test/runtime/CDSJDITests to instead be in jdk/test/com/sun/jdi/CDSJDITests. There was a slight renaming of the tests in the process. Also, I had to update the jdk version of ProcessTool.java to include the createJavaProcessBuilder() variant that is in the hotspot version of ProcessTool.java. Lastly, in CDSJITTest.java I changed: OutputAnalyzer output = new OutputAnalyzer(pb.start()); to instead be: OutputAnalyzer output = ProcessTools.executeProcess(pb); I had to do this since the jdk version of the OutputAnalyzer constructor is not public. The 1st version is what is commonly used in hostspot tests, and the 2nd version is what is commonly used in jdk tests. I decided to adopt the jdk way rather than make the OutputAnalyzer constructors public, although this will probably happen eventually when the two versions are unified. thanks, Chris On 5/19/15 7:25 AM, Chris Plummer wrote: Hi, Please review the following changes for allowing java debugging when CDS is enabled. Webrev:http://cr.openjdk.java.net/~cjplummer/8054386/webrev.01/ Bug:https://bugs.openjdk.java.net/browse/JDK-8054386 The VM changes are simple. I removed the check that prevents debugging with CDS enabled, and added logic that will map the CDS archive RW when debugging is enabled. The tests are a bit more complex. There are a bunch of existing JDI tests for testing debugging support. Rather than start from scratch or clone them, I instead just wrote wrapper tests that put the relevant JDI test classes in the archive, and then invoke the JDI test. I did this for 3 of the JDI tests. If you feel there are others that would be good candidates, I'd be happy to add them. I'm looking for ones that would result in modification of the RO class metadata, such as setting a breakpoint (for which I already added two tests). Testing done: -Using JPRT to run the new jtreg tests on all platforms. -Using JPRT to run all jtreg runtime tests on linux x86 and x_64. -Regular JPRT -testset hotspot run -Putting the JCK JVMTI tests in the archive and then running them. -Putting the nsk jdb, jdwp, jvmti, and jdi tests in the archive and then running them. -Putting a simple test class in the archive and then setting a breakpoint on it using jdb Some of the above testing resulted in the discovery of bugs that still need to be addressed: JDK-8078644, JDK-8078730, and JDK-8079181. I also verified that without the change to map the archive RW, the above testing resulted in a SEGV, which is what you would expect (and actually want to see to prove that the testing is effective). thanks, Chris
Re: [9] RFR (M) 8054386: Allow Java debugging when CDS is enabled
Chris, It looks good in general. I'd suggest to rename the folder: || test/com/sun/jdi/CDSJDITests to: test/com/sun/jdi/cds There is no need to spell JDI as it is already a sub-folder of the com/sun/jdi and there is no need to spell Tests too as it is in the test repo. Also, all the folders are normally named in the lower case. Thanks, Serguei On 6/2/15 8:25 PM, Chris Plummer wrote: Ok, I'm going to keep this as one webrev, but I did create JDK-8081771 for the ProcessTool.java changes: https://bugs.openjdk.java.net/browse/JDK-8081771 I will commit ProcessTool.java under JDK-8081771, and the rest of the changes under JDK-8054386. Both will then be pushed together. I also started a new thread for the review of JDK-8081771: http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2015-June/014930.html http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-June/033892.html thanks, Chris On 6/2/15 11:55 AM, Chris Plummer wrote: I'm going to have to separate out the ProcessTool.java changes into a separate changeset (and CR). In the meantime, feel free to review what I have below. The code won't be changing at all when I separate out the ProcessTool.java changes. thanks, Chris On 6/2/15 12:36 AM, Chris Plummer wrote: [Adding core-libs-dev@openjdk.java.net since this update includes changes to jdk/test library code] Please review the updated webrev: Webrev: http://cr.openjdk.java.net/~cjplummer/8054386/webrev.02/ Bug: https://bugs.openjdk.java.net/browse/JDK-8054386 There were concerns about the new hotspot tests referencing jdk tests. One concern was that if the jdk tests change, they could break the hotspot tests, and this might initially go undetected. The other concern is that if the jdk and hotspot tests are placed in separate test bundles, then it would not be possible to run the hotspot tests. Because of these concerns, I moved the tests that were in hotspot/test/runtime/CDSJDITests to instead be in jdk/test/com/sun/jdi/CDSJDITests. There was a slight renaming of the tests in the process. Also, I had to update the jdk version of ProcessTool.java to include the createJavaProcessBuilder() variant that is in the hotspot version of ProcessTool.java. Lastly, in CDSJITTest.java I changed: OutputAnalyzer output = new OutputAnalyzer(pb.start()); to instead be: OutputAnalyzer output = ProcessTools.executeProcess(pb); I had to do this since the jdk version of the OutputAnalyzer constructor is not public. The 1st version is what is commonly used in hostspot tests, and the 2nd version is what is commonly used in jdk tests. I decided to adopt the jdk way rather than make the OutputAnalyzer constructors public, although this will probably happen eventually when the two versions are unified. thanks, Chris On 5/19/15 7:25 AM, Chris Plummer wrote: Hi, Please review the following changes for allowing java debugging when CDS is enabled. Webrev:http://cr.openjdk.java.net/~cjplummer/8054386/webrev.01/ Bug:https://bugs.openjdk.java.net/browse/JDK-8054386 The VM changes are simple. I removed the check that prevents debugging with CDS enabled, and added logic that will map the CDS archive RW when debugging is enabled. The tests are a bit more complex. There are a bunch of existing JDI tests for testing debugging support. Rather than start from scratch or clone them, I instead just wrote wrapper tests that put the relevant JDI test classes in the archive, and then invoke the JDI test. I did this for 3 of the JDI tests. If you feel there are others that would be good candidates, I'd be happy to add them. I'm looking for ones that would result in modification of the RO class metadata, such as setting a breakpoint (for which I already added two tests). Testing done: -Using JPRT to run the new jtreg tests on all platforms. -Using JPRT to run all jtreg runtime tests on linux x86 and x_64. -Regular JPRT -testset hotspot run -Putting the JCK JVMTI tests in the archive and then running them. -Putting the nsk jdb, jdwp, jvmti, and jdi tests in the archive and then running them. -Putting a simple test class in the archive and then setting a breakpoint on it using jdb Some of the above testing resulted in the discovery of bugs that still need to be addressed: JDK-8078644, JDK-8078730, and JDK-8079181. I also verified that without the change to map the archive RW, the above testing resulted in a SEGV, which is what you would expect (and actually want to see to prove that the testing is effective). thanks, Chris
Re: [9] RFR (M) 8054386: Allow Java debugging when CDS is enabled
[Adding core-libs-dev@openjdk.java.net since this update includes changes to jdk/test library code] Please review the updated webrev: Webrev: http://cr.openjdk.java.net/~cjplummer/8054386/webrev.02/ Bug: https://bugs.openjdk.java.net/browse/JDK-8054386 There were concerns about the new hotspot tests referencing jdk tests. One concern was that if the jdk tests change, they could break the hotspot tests, and this might initially go undetected. The other concern is that if the jdk and hotspot tests are placed in separate test bundles, then it would not be possible to run the hotspot tests. Because of these concerns, I moved the tests that were in hotspot/test/runtime/CDSJDITests to instead be in jdk/test/com/sun/jdi/CDSJDITests. There was a slight renaming of the tests in the process. Also, I had to update the jdk version of ProcessTool.java to include the createJavaProcessBuilder() variant that is in the hotspot version of ProcessTool.java. Lastly, in CDSJITTest.java I changed: OutputAnalyzer output = new OutputAnalyzer(pb.start()); to instead be: OutputAnalyzer output = ProcessTools.executeProcess(pb); I had to do this since the jdk version of the OutputAnalyzer constructor is not public. The 1st version is what is commonly used in hostspot tests, and the 2nd version is what is commonly used in jdk tests. I decided to adopt the jdk way rather than make the OutputAnalyzer constructors public, although this will probably happen eventually when the two versions are unified. thanks, Chris On 5/19/15 7:25 AM, Chris Plummer wrote: Hi, Please review the following changes for allowing java debugging when CDS is enabled. Webrev:http://cr.openjdk.java.net/~cjplummer/8054386/webrev.01/ Bug:https://bugs.openjdk.java.net/browse/JDK-8054386 The VM changes are simple. I removed the check that prevents debugging with CDS enabled, and added logic that will map the CDS archive RW when debugging is enabled. The tests are a bit more complex. There are a bunch of existing JDI tests for testing debugging support. Rather than start from scratch or clone them, I instead just wrote wrapper tests that put the relevant JDI test classes in the archive, and then invoke the JDI test. I did this for 3 of the JDI tests. If you feel there are others that would be good candidates, I'd be happy to add them. I'm looking for ones that would result in modification of the RO class metadata, such as setting a breakpoint (for which I already added two tests). Testing done: -Using JPRT to run the new jtreg tests on all platforms. -Using JPRT to run all jtreg runtime tests on linux x86 and x_64. -Regular JPRT -testset hotspot run -Putting the JCK JVMTI tests in the archive and then running them. -Putting the nsk jdb, jdwp, jvmti, and jdi tests in the archive and then running them. -Putting a simple test class in the archive and then setting a breakpoint on it using jdb Some of the above testing resulted in the discovery of bugs that still need to be addressed: JDK-8078644, JDK-8078730, and JDK-8079181. I also verified that without the change to map the archive RW, the above testing resulted in a SEGV, which is what you would expect (and actually want to see to prove that the testing is effective). thanks, Chris
Re: [9] RFR (M) 8054386: Allow Java debugging when CDS is enabled
I'm going to have to separate out the ProcessTool.java changes into a separate changeset (and CR). In the meantime, feel free to review what I have below. The code won't be changing at all when I separate out the ProcessTool.java changes. thanks, Chris On 6/2/15 12:36 AM, Chris Plummer wrote: [Adding core-libs-dev@openjdk.java.net since this update includes changes to jdk/test library code] Please review the updated webrev: Webrev: http://cr.openjdk.java.net/~cjplummer/8054386/webrev.02/ Bug: https://bugs.openjdk.java.net/browse/JDK-8054386 There were concerns about the new hotspot tests referencing jdk tests. One concern was that if the jdk tests change, they could break the hotspot tests, and this might initially go undetected. The other concern is that if the jdk and hotspot tests are placed in separate test bundles, then it would not be possible to run the hotspot tests. Because of these concerns, I moved the tests that were in hotspot/test/runtime/CDSJDITests to instead be in jdk/test/com/sun/jdi/CDSJDITests. There was a slight renaming of the tests in the process. Also, I had to update the jdk version of ProcessTool.java to include the createJavaProcessBuilder() variant that is in the hotspot version of ProcessTool.java. Lastly, in CDSJITTest.java I changed: OutputAnalyzer output = new OutputAnalyzer(pb.start()); to instead be: OutputAnalyzer output = ProcessTools.executeProcess(pb); I had to do this since the jdk version of the OutputAnalyzer constructor is not public. The 1st version is what is commonly used in hostspot tests, and the 2nd version is what is commonly used in jdk tests. I decided to adopt the jdk way rather than make the OutputAnalyzer constructors public, although this will probably happen eventually when the two versions are unified. thanks, Chris On 5/19/15 7:25 AM, Chris Plummer wrote: Hi, Please review the following changes for allowing java debugging when CDS is enabled. Webrev:http://cr.openjdk.java.net/~cjplummer/8054386/webrev.01/ Bug:https://bugs.openjdk.java.net/browse/JDK-8054386 The VM changes are simple. I removed the check that prevents debugging with CDS enabled, and added logic that will map the CDS archive RW when debugging is enabled. The tests are a bit more complex. There are a bunch of existing JDI tests for testing debugging support. Rather than start from scratch or clone them, I instead just wrote wrapper tests that put the relevant JDI test classes in the archive, and then invoke the JDI test. I did this for 3 of the JDI tests. If you feel there are others that would be good candidates, I'd be happy to add them. I'm looking for ones that would result in modification of the RO class metadata, such as setting a breakpoint (for which I already added two tests). Testing done: -Using JPRT to run the new jtreg tests on all platforms. -Using JPRT to run all jtreg runtime tests on linux x86 and x_64. -Regular JPRT -testset hotspot run -Putting the JCK JVMTI tests in the archive and then running them. -Putting the nsk jdb, jdwp, jvmti, and jdi tests in the archive and then running them. -Putting a simple test class in the archive and then setting a breakpoint on it using jdb Some of the above testing resulted in the discovery of bugs that still need to be addressed: JDK-8078644, JDK-8078730, and JDK-8079181. I also verified that without the change to map the archive RW, the above testing resulted in a SEGV, which is what you would expect (and actually want to see to prove that the testing is effective). thanks, Chris
Re: [9] RFR (M) 8054386: Allow Java debugging when CDS is enabled
Changes look good to me. Misha On 6/2/2015 11:55 AM, Chris Plummer wrote: I'm going to have to separate out the ProcessTool.java changes into a separate changeset (and CR). In the meantime, feel free to review what I have below. The code won't be changing at all when I separate out the ProcessTool.java changes. thanks, Chris On 6/2/15 12:36 AM, Chris Plummer wrote: [Adding core-libs-dev@openjdk.java.net since this update includes changes to jdk/test library code] Please review the updated webrev: Webrev: http://cr.openjdk.java.net/~cjplummer/8054386/webrev.02/ Bug: https://bugs.openjdk.java.net/browse/JDK-8054386 There were concerns about the new hotspot tests referencing jdk tests. One concern was that if the jdk tests change, they could break the hotspot tests, and this might initially go undetected. The other concern is that if the jdk and hotspot tests are placed in separate test bundles, then it would not be possible to run the hotspot tests. Because of these concerns, I moved the tests that were in hotspot/test/runtime/CDSJDITests to instead be in jdk/test/com/sun/jdi/CDSJDITests. There was a slight renaming of the tests in the process. Also, I had to update the jdk version of ProcessTool.java to include the createJavaProcessBuilder() variant that is in the hotspot version of ProcessTool.java. Lastly, in CDSJITTest.java I changed: OutputAnalyzer output = new OutputAnalyzer(pb.start()); to instead be: OutputAnalyzer output = ProcessTools.executeProcess(pb); I had to do this since the jdk version of the OutputAnalyzer constructor is not public. The 1st version is what is commonly used in hostspot tests, and the 2nd version is what is commonly used in jdk tests. I decided to adopt the jdk way rather than make the OutputAnalyzer constructors public, although this will probably happen eventually when the two versions are unified. thanks, Chris On 5/19/15 7:25 AM, Chris Plummer wrote: Hi, Please review the following changes for allowing java debugging when CDS is enabled. Webrev:http://cr.openjdk.java.net/~cjplummer/8054386/webrev.01/ Bug:https://bugs.openjdk.java.net/browse/JDK-8054386 The VM changes are simple. I removed the check that prevents debugging with CDS enabled, and added logic that will map the CDS archive RW when debugging is enabled. The tests are a bit more complex. There are a bunch of existing JDI tests for testing debugging support. Rather than start from scratch or clone them, I instead just wrote wrapper tests that put the relevant JDI test classes in the archive, and then invoke the JDI test. I did this for 3 of the JDI tests. If you feel there are others that would be good candidates, I'd be happy to add them. I'm looking for ones that would result in modification of the RO class metadata, such as setting a breakpoint (for which I already added two tests). Testing done: -Using JPRT to run the new jtreg tests on all platforms. -Using JPRT to run all jtreg runtime tests on linux x86 and x_64. -Regular JPRT -testset hotspot run -Putting the JCK JVMTI tests in the archive and then running them. -Putting the nsk jdb, jdwp, jvmti, and jdi tests in the archive and then running them. -Putting a simple test class in the archive and then setting a breakpoint on it using jdb Some of the above testing resulted in the discovery of bugs that still need to be addressed: JDK-8078644, JDK-8078730, and JDK-8079181. I also verified that without the change to map the archive RW, the above testing resulted in a SEGV, which is what you would expect (and actually want to see to prove that the testing is effective). thanks, Chris
