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

[coleen.phillimore at oracle.com]

This looks good, particularly the removal of unlink class.  Thank you 
for confirming where it's set back to loaded.
Coleen


On 9/29/18 7:32 PM, Jiangli Zhou wrote:
> Hi Coleen,
>
> Thanks a lot for the review! A shared class is set to 'loaded' by 
> SystemDictionary::add_to_hierarchy() via the same call path for both 
> dynamically loaded class and shared classes. If we set 'loaded' state 
> in restore_unshareable_info, we then would need to add CDS specifics 
> to SystemDictionary::add_to_hierarchy to avoid resetting it.
>
> #0  SystemDictionary::add_to_hierarchy (k=0x800006488, 
> __the_thread__=0x7ffff001b000)
> #1  SystemDictionary::define_instance_class (k=0x800006488, 
> __the_thread__=0x7ffff001b000)
> #2  SystemDictionary::find_or_define_instance_class 
> (class_name=0x800419080, class_loader=..., k=0x800006488, 
> __the_thread__=0x7ffff001b000)
> #3  SystemDictionary::load_instance_class (class_name=0x800419080, 
> class_loader=..., __the_thread__=0x7ffff001b000)
> #4  SystemDictionary::resolve_instance_class_or_null 
> (name=0x800419080, class_loader=..., protection_domain=..., 
> __the_thread__=0x7ffff001b000)
> #5  SystemDictionary::resolve_instance_class_or_null_helper 
> (class_name=0x800419080, class_loader=..., protection_domain=..., 
> __the_thread__=0x7ffff001b000)
> #6  SystemDictionary::resolve_or_null (class_name=0x800419080, 
> class_loader=..., protection_domain=..., __the_thread__=0x7ffff001b000)
>
> ...
>
> I updated the webrev: 
> http://cr.openjdk.java.net/~jiangli/8210926/webrev.01/ to remove 
> unlink_class() and added more comments. Please let me know if that 
> looks okay to you.
>
> Thanks,
> Jiangli
>
> On 9/28/18 1:58 PM, coleen.phillimore at oracle.com wrote:
>>
>> On second thought, where does this set the shared class to loaded? 
>> Should that be done in restore_unshareable_info?
>> thanks,
>> Coleen
>>
>> On 9/28/18 4:43 PM, coleen.phillimore at oracle.com wrote:
>>> This looks good!
>>> Coleen
>>>
>>> On 9/28/18 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
>>>>
>>>>
>>>
>>
>