RFR (M) JDK-8163406: The fixup_module_list must be protected by Module_lock when inserting new entries

[Coleen Phillimore]

This looks good to me.
Thanks,
Coleen

On 9/16/16 3:35 PM, Lois Foltan wrote:
>
> On 9/13/2016 9:17 PM, Zhengyu Gu wrote:
>> 3) in javaClasses
>>>>>
>>>>> Lines 856, 865: do you need a load_module_acquire when you read
>>>>> javabase_entry->module() in the assertion?
>>>> Completed, thanks for pointing that out.  New webrev based on your and
>>>> Coleen's comments at:
>>>>
>>>> http://cr.openjdk.java.net/~lfoltan/bug_jdk8163406.1/webrev/
>>>
>>> In this updated code:
>>>
>>>  803     assert(Universe::is_module_initialized() ||
>>>  804            (ModuleEntryTable::javabase_defined() &&
>>>  805             (module() == 
>>> JNIHandles::resolve(ModuleEntryTable::javabase_moduleEntry()->module_load_acquire()))),
>>>  806            "Incorrect java.lang.reflect.Module specification 
>>> while creating mirror");
>>>
>>> you are doubling up on the load-acquire because it is already inside 
>>> javabase_defined(). Whether you need the load-acquire in 
>>> javabase_defined() depends on the context in which 
>>> javabase_defined() is called - which isn't completely clear to me.
>>>
>> javabase_defined() itself does not need load-acquire, it looks like 
>> an unexpected side-effect to me. I would rather see explicit
>> module_load_acquire() call than depending on this side-effect to 
>> ensure ordered access.
>
> After further examination and discussion with David, the store-release 
> and load-acquire of java.base's _module field are not needed in the 
> contexts that the method javabase_defined() is currently being called 
> within.  They have been removed.  The latest webrev addresses the 
> primary focus of the bug - the need to ensure that the insertion into 
> the fixup_module_field list in the method 
> java_lang_Class::create_mirror() is protected by Module_lock.  If it 
> is later determined that the ModuleEntry for java.base does need 
> special store-release/load_acquire, a separate issue will be opened to 
> address this.
>
> http://cr.openjdk.java.net/~lfoltan/bug_jdk8163406.2/webrev/
>
> Thank you,
> Lois
>
>>
>> -Zhengyu
>>
>>> David
>>> ----
>>>
>>>> Thank you for reviewing!
>>>> Lois
>>>>
>>>>>
>>>>> thanks,
>>>>> Karen
>>>>>
>>>>>
>>>>>> On Sep 7, 2016, at 11:31 AM, Lois Foltan <lois.foltan at oracle.com> 
>>>>>> wrote:
>>>>>>
>>>>>> Hello,
>>>>>>
>>>>>> Please review the following fix:
>>>>>>
>>>>>> Webrev:
>>>>>> http://cr.openjdk.java.net/~lfoltan/bug_jdk8163406/webrev/
>>>>>>
>>>>>> Bug: The fixup_module_list must be protected by Module_lock when
>>>>>> inserting new entries
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8163406
>>>>>>
>>>>>> Summary:
>>>>>> During bootstrapping, after the load of java.lang.Class, the method
>>>>>> java_lang_Class::create_mirror() is multi-threaded. Before java.base
>>>>>> is defined to the VM, the fixup_module_list stores all classes that
>>>>>> once java.base is defined, must have their module field patched with
>>>>>> java.base's java.lang.reflect.Module.  Insertion of a class into the
>>>>>> fixup_module_list must be protected via Module_lock. Included in 
>>>>>> this
>>>>>> fix is correct order access store and load of the
>>>>>> java.lang.reflect.Module field for java.base's ModuleEntry. Thank
>>>>>> you to David Holmes for guidance on this piece.
>>>>>>
>>>>>> Testing:
>>>>>> Full hotspot nightly testing, JCK vm & lang, several iterations of
>>>>>> jaxp tests to test JDK-8164721
>>>>
>>
>