RFR (S): JDK-8152949: Jigsaw crash when Klass in _fixup_module_field_list is unloaded

Coleen Phillimore coleen.phillimore at oracle.com
Tue Apr 19 21:28:45 UTC 2016 


On 4/19/16 4:56 PM, Stefan Karlsson wrote:
> On 2016-04-19 22:24, Coleen Phillimore wrote:
>>
>> Hi, this is getting long.
>>
>> On 4/19/16 3:48 PM, Lois Foltan wrote:
>>>
>>> On 4/18/2016 10:11 PM, David Holmes wrote:
>>>> Hi Lois,
>>>>
>>>> On 19/04/2016 6:25 AM, Lois Foltan wrote:
>>>>>
>>>>> On 4/18/2016 7:31 AM, Lois Foltan wrote:
>>>>>>
>>>>>> On 4/18/2016 3:06 AM, Stefan Karlsson wrote:
>>>>>>> On 2016-04-15 21:45, Alan Bateman wrote:
>>>>>>>>
>>>>>>>> On 15/04/2016 18:02, Lois Foltan wrote:
>>>>>>>>>
>>>>>>>>> Hi Stefan,
>>>>>>>>>
>>>>>>>>> In start up before module system initialization in complete I
>>>>>>>>> believe the VM is single threaded, so the increment/decrement
>>>>>>>>> reference counts do not need to be atomic.  Adding it is a
>>>>>>>>> defensive move in case the reference count is ever used passed
>>>>>>>>> start up in the future.  It kind of does seem a bit excessive,
>>>>>>>>> sounds like you agree?
>>>>>>>> There will be a number of threads running before the base 
>>>>>>>> module is
>>>>>>>> defined to the VM. As things stand the the java threads at this
>>>>>>>> point will be the Common-Cleaner, Finalizer, Reference Handler and
>>>>>>>> Signal Handler.
>>>>>>>
>>>>>>> So, are you saying that we need the atomics?
>>>>>>>
>>>>>>> The java_lang_Class::create_mirror function isn't multi-thread 
>>>>>>> safe,
>>>>>>> and must already be guarded by a lock (SystemDictionary_lock 
>>>>>>> AFAICT).
>>>>>>> The increment in Unsafe_DefineAnonymousClass0, will only be done
>>>>>>> once, for the single InstanceKlass instance in the CLD. And all 
>>>>>>> reads
>>>>>>> of _keep_alive from the GC are done during safepoints.
>>>>>> The anonymous class is inserted in the fixup mirror and fixup module
>>>>>> lists during java_lang_Class::create_mirror() before it is made 
>>>>>> public
>>>>>> or "published" as loaded.  So the two instances where the reference
>>>>>> count is incremented, Unsafe_DefineAnonymousClass0 and in
>>>>>> java_lang_Class::create_mirror(), are guarded by a lock as well 
>>>>>> as the
>>>>>> decrement in Unsafe_DefineAnonymousClass0.  No other thread has 
>>>>>> access
>>>>>> to the class during this time, as it is being loaded.
>>>>>>>
>>>>>>> How does ModuleEntryTable::patch_javabase_entries guard against
>>>>>>> concurrent inserts into the _fixup_module_field_list list?
>>>>>> That leaves the decrement in
>>>>>> ModuleEntryTable::patch_javabase_entries() as possibly unguarded. 
>>>>>> This
>>>>>> only occurs when the VM is called to define the module java.base.  I
>>>>>> believe this should be okay but will double check.
>>>>>
>>>>> One small change in modules.cpp/define_javabase_module() to ensure 
>>>>> that
>>>>> only one definition attempt of java.base will occur and thus only one
>>>>> call to ModuleEntryTable::patch_javabase_entries().  If a situation
>>>>> arises where java.base is trying to be multiply defined, according to
>>>>> the expected error conditions for JVM_DefineModule(), an
>>>>> IllegalArgumentException should be thrown.
>>>>>
>>>>> I have also added a comment in classfile/classLoaderData.hpp 
>>>>> explaining
>>>>> why _keep_alive does need to be defined volatile or atomic.
>>>>
>>>> Can you add assertions to check that _keep_alive is only modified 
>>>> under the protection of the lock (with a special case perhaps for 
>>>> the unguarded java.base case) ?
>>>
>>> Hi David,
>>>
>>> Thanks for the review.  I misspoke when I indicated that the two 
>>> increments and the one decrement of the reference counter that occur 
>>> during a call to the Unsafe_DefineAnonymous0() method were guarded 
>>> under a lock.  However, due to the way anonymous classes are created 
>>> only a single non-GC thread will have access to the _keep_alive 
>>> field during this time.  And as Stefan indicates above, all reads of 
>>> _keep_alive from the GC are done during safepoints.  Each anonymous 
>>> class, when defined, has a dedicated ClassLoaderData created for 
>>> it.  No other class shares the anonymous class' name or CLD.  Due to 
>>> this uniqueness, no other thread has knowledge about this anonymous 
>>> class while it is being defined.  It is only upon return from 
>>> Unsafe_DefineAnonymous0(), that the anonymous class exists and other 
>>> threads, at that point, can potentially access it.
>>>
>>
>> Ah interesting.  Currently, this is true and why this is safe. If we 
>> change the JVM to have *some* anonymous classes share CLD with their 
>> host_class because the lifetimes are the same, then we'll have to use 
>> atomic operations.
>
> Well, then we wouldn't have to use the _keep_alive field because the 
> lifetime would be tracked by the loader of the host_class. The 
> _keep_alive field was added to handle the short window between the 
> creation of the InstanceKlass and the creation of the mirror 
> (java.lang.Class instances). If we have a class loader, then the 
> anonymous klass will be kept alive automatically, as long as the class 
> loader is reachable.

There is no way to have the host_class unloaded while an anonymous class 
is loaded for that host_class.  Yes, this is true.  The class_loader is 
live because the code loading the anonymous class has the host_class 
holder (loader or mirror) as a reference.

Coleen


>
> StefanK
>>
>> http://cr.openjdk.java.net/~lfoltan/bug_jdk8152949_1/src/share/vm/classfile/classLoaderData.cpp.udiff.html 
>>
>>
>> Can you put one comment directly above the inc_keep_alive() and 
>> dec_keep_alive() functions to this effect here, just so we remember?
>>
>> // Anonymous classes have their own ClassLoaderData that is marked to 
>> keep alive while the class is being parsed, and
>> // if the class appears on the module fixup list.
>> // If anonymous classes are changed to share with host_class, this 
>> refcount needs to be changed to use atomic operations.
>>
>> *+ void ClassLoaderData::inc_keep_alive() {*
>> *+ assert(_keep_alive >= 0, "Invalid keep alive count");*
>> *+ _keep_alive++;*
>> *+ }*
>> *+ *
>> *+ void ClassLoaderData::dec_keep_alive() {*
>> *+ assert(_keep_alive > 0, "Invalid keep alive count");*
>> *+ _keep_alive--;*
>> *+ }*
>> *+ *
>>
>> More below.
>>
>>> Thanks,
>>> Lois
>>>
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>>> Please review at:
>>>>>
>>>>>      http://cr.openjdk.java.net/~lfoltan/bug_jdk8152949_1/
>>
>> http://cr.openjdk.java.net/~lfoltan/bug_jdk8152949_1/src/share/vm/classfile/modules.cpp.frames.html 
>>
>>
>> I'm not sure how this relates to the bug.
>>
>> Otherwise, the change looks good.
>>
>> Thanks,
>> Coleen
>>
>>
>>>>>
>>>>> Retesting in progress.
>>>>>
>>>>> Thanks,
>>>>> Lois
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> Lois
>>>>>>
>>>>>>>
>>>>>>> thanks,
>>>>>>> StefanK
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> -Alan
>>>>>>>
>>>>>>
>>>>>
>>>
>>
>

More information about the hotspot-dev mailing list