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

Wed Apr 20 10:47:44 UTC 2016

On 4/19/2016 4:37 PM, Christian Thalinger wrote:
>> On Apr 19, 2016, at 10:24 AM, Coleen Phillimore <coleen.phillimore at oracle.com> 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.
>>
>> http://cr.openjdk.java.net/~lfoltan/bug_jdk8152949_1/src/share/vm/classfile/classLoaderData.cpp.udiff.html <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?
> Why don’t we just use atomic operations and be done with it?

Hi Christian,

That was a discussion point earlier on this review thread and there were 
concerns raised that it may be excessive and potentially confusing for 
someone reading the code because it indicates that the VM is in a 
multi-threaded context when defining the anonymous class when it is not.

Thanks for the suggestion and review,
Lois

>
>> // 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/>
>> http://cr.openjdk.java.net/~lfoltan/bug_jdk8152949_1/src/share/vm/classfile/modules.cpp.frames.html <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