RFR(S): 8200466: Revisit the setting of _transitive_interfaces in InstanceKlass

Calvin Cheung calvin.cheung at oracle.com
Tue May 1 18:07:06 UTC 2018


Hi Coleen,

Thanks for taking a closer look at it.
I'll sync up my repo, do another round of testing, before pushing the 
change.

thanks,
Calvin

On 5/1/18, 10:29 AM, coleen.phillimore at oracle.com wrote:
>
> Hi Calvin,
>
> Ok, I had a look at this.  It seems that the relative initialization 
> orders of _super and the primary superclasses is unfathonably 
> complicated.
> I'm fine with your initial change as is.
>
> Thanks,
> Coleen
>
>
> On 4/30/18 8:24 PM, Calvin Cheung wrote:
>> Hi Coleen,
>>
>> I've tried the following simple change:
>>
>> --- a/src/hotspot/share/classfile/classFileParser.cpp
>> +++ b/src/hotspot/share/classfile/classFileParser.cpp
>> @@ -3592,6 +3592,7 @@
>>    this_klass->set_methods(_methods);
>>    this_klass->set_inner_classes(_inner_classes);
>>    this_klass->set_local_interfaces(_local_interfaces);
>> + this_klass->set_super(const_cast<InstanceKlass*>(_super_klass));
>> this_klass->set_transitive_interfaces(_transitive_interfaces);
>>    this_klass->set_annotations(_combined_annotations);
>>
>> but it crashes in vtableEntry::verify() during fastdebug build:
>>
>>     if (!vtklass->is_subtype_of(method()->method_holder())) {
>> #ifndef PRODUCT
>>       print();
>> #endif
>>       fatal("vtableEntry " PTR_FORMAT ": method is from subclass", 
>> p2i(this)); <<<<-- crashes here
>>     }
>>
>> Here's the call stack from the hs err log:
>>
>> V  [libjvm.so+0xb32bbe]  report_fatal(char const*, int, char const*, 
>> ...)+0xfe
>> V  [libjvm.so+0x1215963]  klassVtable::verify(outputStream*, bool)+0x113
>> V  [libjvm.so+0x1219753]  klassVtable::initialize_vtable(bool, 
>> Thread*)+0x1293
>> V  [libjvm.so+0x1834b6f]  Universe::reinitialize_vtable_of(Klass*, 
>> Thread*)+0x2f
>> V  [libjvm.so+0x1834c2c]  Universe::reinitialize_vtable_of(Klass*, 
>> Thread*)+0xec
>> V  [libjvm.so+0x1834c2c]  Universe::reinitialize_vtable_of(Klass*, 
>> Thread*)+0xec
>> V  [libjvm.so+0x1837b3f]  universe_post_init()+0x1af
>> V  [libjvm.so+0xe99aad]  init_globals()+0xed
>> V  [libjvm.so+0x17f8b2c]  Threads::create_vm(JavaVMInitArgs*, 
>> bool*)+0x2cc
>> V  [libjvm.so+0x1018af8]  JNI_CreateJavaVM+0xa8
>> C  [libjli.so+0x3e81]  JavaMain+0x81
>>
>> I will send you the hs err and build logs off list.
>>
>> thanks,
>> Calvin
>>
>> On 4/30/18, 2:20 PM, coleen.phillimore at oracle.com wrote:
>>>
>>>
>>> On 4/30/18 4:58 PM, Calvin Cheung wrote:
>>>> Hi Coleen,
>>>>
>>>> Thanks for your review.
>>>>
>>>> On 4/30/18, 10:23 AM, coleen.phillimore at oracle.com wrote:
>>>>>
>>>>> This sharing of metadata has been prone to bugs.  Can you have the 
>>>>> same bug with the _secondary_super field? 
>>>>> InstanceKlass::deallocate_contents() checks:
>>>>>
>>>>>       secondary_supers() != transitive_interfaces() &&
>>>>>
>>>>> and will free the field possibly from the super class if 
>>>>> transitive_interfaces() is still NULL.  (If I understand this 
>>>>> correctly).
>>>> If it passes the conditions check in the 'if' block, it will 
>>>> deallocate the _secondary_supers.
>>>> It maybe fine because the crash was in visit_all_interfaces() where 
>>>> it doesn't dereference the _secondary_supers.
>>>
>>> It seems like it would deallocate _secondary_supers if an OOM 
>>> occurred after it's set to the super's transitive_interfaces and 
>>> before _transitive_interfaces is set.  There could be a state that 
>>> could cause this to happen.  I would have to trace through to see 
>>> what sort of crash would happen.
>>>
>>>>>
>>>>> Wouldn't it be easier just to eagerly set the super class, and 
>>>>> keep the call to initialize_supers() where it is?
>>>> Do you mean adding the following in 
>>>> ClassFileParser::apply_parsed_class_metadata() and reverting the 
>>>> rest of the changes?
>>>>
>>>> this_klass->set_super(const_cast<Klass*>(_super_klass));
>>>>
>>>
>>> Yes, can you try this?  This line with a comment would be less 
>>> complicated than passing NULL to initialize_supers() for the 
>>> arrayKlass versions.
>>>
>>>
>>>> BTW, I think the first arg to initialize_supers() should be casted 
>>>> to Klass* like above because the _super_klass is already an 
>>>> InstanceKlass*.
>>>>
>>>> ik->initialize_supers(const_cast<InstanceKlass*>(_super_klass) ...
>>>>
>>>>
>>> I think we still need some cast here and above because _super_klass 
>>> is defined as const, and this function and set_super are both not 
>>> const-safe, which is a bigger change. Or you could make 
>>> ClassFileParser::_super_klass not const since const is cast away.
>>>
>>>>>
>>>>> http://cr.openjdk.java.net/~ccheung/8200466/webrev.00/src/hotspot/share/classfile/classFileParser.cpp.udiff.html 
>>>>>
>>>>>
>>>>> **** 5884,5893 *****
>>>>> *--- 5889,5899 ----*
>>>>> *Annotations::free_contents(_loader_data, _fields_annotations);*
>>>>> *Annotations::free_contents(_loader_data, _fields_type_annotations);*
>>>>> *}*
>>>>> **
>>>>> *clear_class_metadata();*
>>>>> *+ _transitive_interfaces = NULL;*
>>>>> **
>>>>>
>>>>> clear_class_metadata() already sets the _transitive_interfaces to 
>>>>> NULL, so this shouldn't be needed.
>>>> The current change does not set the _trasitive_interfaces to NULL 
>>>> in clear_class_metadata().
>>>>
>>>> From the udiff above:
>>>>
>>>> **** 5832,5842 *****
>>>> *--- 5838,5847 ----*
>>>> *     _cp = NULL;*
>>>> *     _fields = NULL;*
>>>> *     _methods = NULL;*
>>>> *     _inner_classes = NULL;*
>>>> *     _local_interfaces = NULL;*
>>>> *     _transitive_interfaces = NULL;*
>>>> *     _combined_annotations = NULL;*
>>>> *     _annotations = _type_annotations = NULL;*
>>>> *     _fields_annotations = _fields_type_annotations = NULL;*
>>>> *   }*
>>>
>>> Oh I see it now and why you changed it.  Can you see if setting the 
>>> Klass::_super ahead of setting _transitive_interfaces would work 
>>> better?
>>>
>>> thanks,
>>> Coleen
>>>
>>>>
>>>> thanks,
>>>> Calvin
>>>>>
>>>>> thanks,
>>>>> Coleen
>>>>>
>>>>> On 4/27/18 12:46 AM, Calvin Cheung wrote:
>>>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8200466
>>>>>>
>>>>>> webrev: http://cr.openjdk.java.net/~ccheung/8200466/webrev.00/
>>>>>>
>>>>>> This bug was discovered during the investigation of JDK-8200078 
>>>>>> <https://bugs.openjdk.java.net/browse/JDK-8200078>.
>>>>>>
>>>>>> I've included the following comment in classFileParser.cpp to 
>>>>>> summarize the change:
>>>>>>
>>>>>> // Delay the setting of _transitive_interfaces until after 
>>>>>> initialize_supers() in
>>>>>> // fill_instance_klass(). It is because the 
>>>>>> _transitive_interfaces may be shared with
>>>>>> // its _super. If an OOM occurs while loading the current klass, 
>>>>>> its _super field
>>>>>> // may not have been set. When GC tries to free the klass, the 
>>>>>> _transitive_interfaces
>>>>>> // may be deallocated mistakenly in 
>>>>>> InstanceKlass::deallocate_interfaces(). Subsequent
>>>>>> // dereferences to the deallocated _transitive_interfaces will 
>>>>>> result in a crash.
>>>>>>
>>>>>> Testing: (on Oracle platforms)
>>>>>>     hs-tier{1,2,3}
>>>>>>     closed (soon will be open) PCL (parallel class loading) tests
>>>>>>
>>>>>> thanks,
>>>>>> Calvin
>>>>>
>>>
>


More information about the hotspot-runtime-dev mailing list