RFR 8163969: Cyclic interface initialization causes JVM crash
Coleen Phillimore
coleen.phillimore at oracle.com
Wed Sep 21 13:50:14 UTC 2016
Thank you!
Coleen
On 9/20/16 5:50 PM, Karen Kinnear wrote:
> Thank you Coleen - the set_initialization_state_and_notify_impl is even better than I was suggesting.
>
> All sounds good to go!
>
> many thanks,
> Karen
>
>> On Sep 20, 2016, at 4:19 PM, Coleen Phillimore <coleen.phillimore at oracle.com> wrote:
>>
>>
>>
>> On 9/20/16 1:39 PM, Karen Kinnear wrote:
>>> Coleen,
>>>
>>> Code looks good. Kudos to you and David Holmes for extremely careful reading of the specification.
>> Thank you for the consultation on this bug!
>>> In an earlier web rev you had also made a defensive fix to set_initialization_state_and_notify_impl.
>>> Could you possibly add a fix for that?
>>>
>>> 1) assertion if init_lock is null
>>> 2) for production time: if init_lock is null, at least do not try to dereference ObjectLocker.
>> I rewrote the function to assert if init_lock is null but handle the case in product for robustness.
>>
>> void InstanceKlass::set_initialization_state_and_notify_impl(instanceKlassHandle this_k, ClassState state, TRAPS) {
>> oop init_lock = this_k->init_lock();
>> if (init_lock != NULL) {
>> ObjectLocker ol(init_lock, THREAD);
>> this_k->set_init_state(state);
>> this_k->fence_and_clear_init_lock();
>> ol.notify_all(CHECK);
>> } else {
>> assert(init_lock != NULL, "The initialization state should never be set twice");
>> this_k->set_init_state(state);
>> }
>> }
>>> If UseBiasedLocking is disabled, there is a risk of dereferencing null otherwise.
>>>
>>> Thank you for the detailed tests.
>>>
>>> 3) In InterfaceInitializationStates.java - could you possibly explain the Iunlinked test - what it is testing, what the
>>> comment means and what the new bug is?
>> Maybe this is better?
>>
>> // Iunlinked is testing initialization like interface I, except interface I is linked when
>> // ClassLIM is linked.
>> // Iunlinked is not linked already when K gets an initialization error. Linking Iunlinked
>> // should succeed and not get NoClassDefFoundError because it does not depend on the
>> // initialization state of K for linking. There's bug now where it gets this error.
>> // See: https://bugs.openjdk.java.net/browse/JDK-8166203.
>>
>>> A couple of minor comments/suggestions to make it easier for the person who changes it next on the InterfaceInitializationStates.java tests:
>>> 1) line 66 comment refers to ClassL - I think you mean ClassLIM?
>> Fixed.
>>> 2) line 61: “Calling function on class with bad super interface.”
>>> I think you want “on class” -> “on interface”
>> Yes, thanks.
>>> 3) Could you add a comment before the try on Class.forName that the result of this test is NCDFE because
>>> there was an earlier test (ClassLIM) that already initialized K?
>> // Test that K already has initialization error so gets ClassNotFoundException because
>> // initialization was attempted with ClassLIM.
>>> 4) line 143: “K (sub interface)” -> “K (super interface)”
>> Fixed.
>>> 5) lines 155-156 “K, its superclass state” -> “K (its super interface) is in initialization_error state.
>> Yes, that reads better.
>>
>> Thanks for the comments and careful review on this fix!
>>
>> Coleen
>>
>>> thanks
>>> Karen
>>>
>>>
>>>> On Sep 16, 2016, at 11:22 AM, Coleen Phillimore <coleen.phillimore at oracle.com> wrote:
>>>>
>>>> Summary: Fix interface initialization to follow spec: interface initializations do not set initialization state of interfaces that extend them.
>>>>
>>>> Tested with: all hotspot jtreg tests, co-located nsk tests, non-colocated nsk tests, and jck tests.
>>>>
>>>> open webrev at http://cr.openjdk.java.net/~coleenp/8163969.01/webrev
>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8163969
>>>>
>>>> Thanks,
>>>> Coleen
More information about the hotspot-runtime-dev
mailing list