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

[David Holmes]

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.

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
>>>
>>>
>