RFR (S): 8214086: [TESTBUG] Fix subgraph test cases in ReplaceCriticalClasses.java

[Jiangli Zhou]

I realized that we need '@requires vm.cds.archived.java.heap' for the 
archived subgraph tests. Instead of applying the restriction to all test 
cases in ReplaceCriticalClasses.java, I moved the three '-subgraph' test 
cases out and placed them in ReplaceCriticalClassesForSubgraphs.java. 
ReplaceCriticalClassesForSubgraphs extends from ReplaceCriticalClasses 
and reuses the existing test logic without duplicating:

http://cr.openjdk.java.net/~jiangli/8214086/webrev.02/test/hotspot/jtreg/runtime/SharedArchiveFile/serviceability/ReplaceCriticalClassesForSubgraphs.java.html

Full webrev: http://cr.openjdk.java.net/~jiangli/8214086/webrev.02/

Thanks,
Jiangli

On 11/21/18 5:34 PM, Jiangli Zhou wrote:
> Thanks!
>
> Jiangli
>
>
> On 11/21/18 5:34 PM, Ioi Lam wrote:
>> Looks good. Thanks!
>>
>> - Ioi
>>
>> On 11/21/18 4:34 PM, Jiangli Zhou wrote:
>>> Here is the updated webrev with a new test case suggested by Ioi:
>>>
>>> http://cr.openjdk.java.net/~jiangli/8214086/webrev.01/test/hotspot/jtreg/runtime/SharedArchiveFile/serviceability/ReplaceCriticalClasses.java.frames.html 
>>>
>>>
>>> Full webrev: http://cr.openjdk.java.net/~jiangli/8214086/webrev.01/
>>>
>>> Thanks,
>>> Jiangli
>>>
>>> On 11/21/18 3:23 PM, Jiangli Zhou wrote:
>>>> Hi Ioi,
>>>>
>>>> On 11/21/18 3:08 PM, Ioi Lam wrote:
>>>>> Hi Jiangli,
>>>>>
>>>>> For this test
>>>>>
>>>>> final String subgraphInit = "initialize_from_archived_subgraph " + 
>>>>> subgraphKlass;
>>>>> ...
>>>>> if (checkSubgraph) {
>>>>>      out.shouldNotContain(subgraphInit);
>>>>> }
>>>>>
>>>>> I think you should also test for the reverse condition -- if 
>>>>> java/lang/Long has not been redefined, you should see the 
>>>>> subgraphInit in the output.
>>>>>
>>>>> The reason for this is shouldNotContain() is very lenient. If you 
>>>>> made a mistake in formatting subgraphInit, shouldNotContain will 
>>>>> not complain, and you will end up having a bad test that doesn't 
>>>>> test what you're looking for.
>>>>
>>>> I actually have the reverse check initially, but decided to remove 
>>>> it since we have other tests that explicitly check for archived 
>>>> Long cache and system module subgraphs. I can add one test case for 
>>>> the Long cache (for future proof in case the logging output is 
>>>> changed).
>>>>
>>>> Thanks,
>>>> Jiangli
>>>>
>>>>>
>>>>> Thanks
>>>>>
>>>>> - Ioi
>>>>>
>>>>>
>>>>>
>>>>> On 11/21/18 2:17 PM, Jiangli Zhou wrote:
>>>>>> Please review the following test fix in 
>>>>>> ReplaceCriticalClasses.java. I re-implemented the subgraph check 
>>>>>> to verify a subgraph is not used at runtime if one of it's klass 
>>>>>> (who's object is contained within the archived subgraph) is 
>>>>>> changed by the JVMTI agent.
>>>>>>
>>>>>> I added a test case for the newly archived Long cache subgraph. 
>>>>>> Since both ResolvedModule and Long classes are loaded during the 
>>>>>> primordial phase, '-early -notshared' are set in those test cases.
>>>>>>
>>>>>> As part of the change, I fixed 
>>>>>> HeapShared::initialize_from_archived_subgraph to only do the 
>>>>>> logging output after sbugraph(s) is successfully installed in the 
>>>>>> given Klass.
>>>>>>
>>>>>> webrev: http://cr.openjdk.java.net/~jiangli/8214086/webrev.00
>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8214086
>>>>>>
>>>>>> Tested ReplaceCriticalClasses.java locally on linux-x64. Will run 
>>>>>> tier1-tier2.
>>>>>>
>>>>>> Thanks,
>>>>>> Jiangli
>>>>
>>>
>