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

[Lois Foltan]

On 4/20/2016 4:23 PM, harold seigel wrote:
> Hi Lois,
>
> Your changes look good.

Thank you Harold for the review!
Lois

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