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