[9] RFR (M) 8054386: Allow Java debugging when CDS is enabled

[serguei.spitsyn at oracle.com]

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.spitsyn at 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 at 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
>>>>>>
>>>>>
>>>>
>>>
>>
>