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

[Jiangli Zhou]

On 11/25/18 9:15 PM, Ioi Lam wrote:
> Hi Jiangli,
>
> The new tests looks good.
Thanks for taking another look.

Thanks,
Jiangli
>
> Thanks
>
> - Ioi
>
>
> On 11/25/18 4:06 PM, Jiangli Zhou wrote:
>> 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
>>>>>>
>>>>>
>>>
>>
>