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

Tue May 1 00:24:22 UTC 2018

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