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

Tue May 1 17:29:48 UTC 2018

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