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

[Lois Foltan]

On 4/20/2016 8:24 AM, Coleen Phillimore wrote:
>
>
> On 4/20/16 6:42 AM, Lois Foltan wrote:
>>
>> On 4/19/2016 4:24 PM, 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.
>>>
>>> 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?
>>
>> Hi Coleen,
>>
>> I will add the comment before I commit.  I might expand a bit on your 
>> last sentence a bit.
>
> Apparently the comment is false because we don't need the alive count 
> for non-anonymous ClassLoaderData ... there are more replies to this 
> thread.
>
> So maybe take out my last suggested sentence.

Right, saw that.  Will do.  Thanks again.
Lois

>
> Coleen
>>
>>>
>>> // 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.
>>
>> This change ensures that java.base will not be multiply defined and 
>> thus only one call to ModuleEntryTable::patch_javabase_entries() will 
>> occur.  A decrement of the reference count happens when an anonymous 
>> class is on the fixup module list post patching it with java.base
>>
>>>
>>> Otherwise, the change looks good.
>>
>> Thanks again!
>>
>>>
>>> Thanks,
>>> Coleen
>>>
>>>
>>>>>>
>>>>>> Retesting in progress.
>>>>>>
>>>>>> Thanks,
>>>>>> Lois
>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Lois
>>>>>>>
>>>>>>>>
>>>>>>>> thanks,
>>>>>>>> StefanK
>>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>> -Alan
>>>>>>>>
>>>>>>>
>>>>>>
>>>>
>>>
>>
>