RFR: 8210926: vmTestbase/nsk/jvmti/scenarios/allocation/AP11/ap11t001/TestDescription.java failed with JVMTI_ERROR_INVALID_CLASS in CDS mode

[Jiangli Zhou]

On 9/30/18 1:17 PM, David Holmes wrote:

> Hi Jiangli,
>
> The typo "the the" is still present in instanceKlass.cpp:
>
> +   // before the InstanceKlass is added to the the SystemDictionary. 
> Make
>
> Otherwise update looks good. No need for new webrev.

Oops. Fixed now. Thanks for catching it!

Thanks!

Jiangli
>
> Thanks,
> David
>
> On 30/09/2018 9:06 AM, Jiangli Zhou wrote:
>> Hi David,
>>
>> Thanks a lot for the review! Here is the updated webrev: 
>> http://cr.openjdk.java.net/~jiangli/8210926/webrev.01/.
>>
>>
>> On 9/28/18 9:23 AM, David Holmes wrote:
>>> Hi Jiangli,
>>>
>>> This seems a little odd:
>>>
>>> 2303   // Unlink the class
>>> 2304   if (is_linked()) {
>>> 2305     unlink_class();
>>> 2306   }
>>> 2307
>>> 2308   _init_state = allocated;
>>>
>>> unlink_class() will set the _init_state to loaded then we set it 
>>> back to allocated.
>> Thanks for pointing that out. I was not too happy about that either. 
>> The only usage of unlink_class() (which just resets the state back to 
>> 'loaded') is from InstanceKlass::remove_unshareable_info(). I've 
>> updated the webrev to remove unlink_class(). Hope that makes the code 
>> a little more cleaner.
>>>
>>> There's a typo in a comment "the the".
>> Fixed.
>>
>>>
>>> Thanks,
>>> David
>>> (Literally running out the door :) )
>>
>> Hope your flight was enjoyable!
>>
>> Thanks,
>> Jiangli
>>>
>>> On 28/09/2018 10:08 AM, Jiangli Zhou wrote:
>>>> Please review the fix for JDK-8210926. This is a bug in the CDS 
>>>> code that's exposed by JvmtiEnv::GetLoadedClasses(), and can be 
>>>> manifested in different failures with the following tests:
>>>>
>>>>    com/sun/jdi/ClassesByName2Test.java
>>>> vmTestbase/nsk/jvmti/scenarios/allocation/AP11/ap11t001/TestDescription.java 
>>>>
>>>>    runtime/RedefineTests/ModifyAnonymous.java
>>>>
>>>> webrev: http://cr.openjdk.java.net/~jiangli/8210926/webrev.00/
>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8210926
>>>>
>>>> SystemDictionary::add_to_hierarchy() sets a InstanceKlass 
>>>> init_state to ‘loaded’ right before it is added to the the 
>>>> SystemDictionary. JvmtiEnv::GetLoadedClasses() retrieves loaded 
>>>> classes’ (InstanceKlasses in 'loaded' state and arrays) mirrors 
>>>> (Class objects). At CDS dump time, a InstanceKlass::_init_state is 
>>>> reset back to 'loaded' state before writing out the archived data. 
>>>> At runtime during loading of a shared class, there is a 'brief' 
>>>> moment JvmtiEnv::GetLoadedClasses() in another thread could see a 
>>>> shared class in ‘loaded’ state without mirror. NULL mirror is not 
>>>> the only issue, other fields of the shared InstanceKlass may not be 
>>>> setup properly before SystemDictionary::add_to_hierarchy(). To fix 
>>>> the issue, we need to reset to _init_state to 'allocated' state 
>>>> before writing out the archived classes at dump time.
>>>>
>>>> Verified the fix with running ClassesByName2Test.java using mach5 
>>>> (thanks Chris for providing the reproducible case). Tested with 
>>>> tier1 - tier3 in both default CDS mode and no CDS mode.
>>>>
>>>> Thanks,
>>>> Jiangli
>>>>
>>>>
>>