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

[Lois Foltan]

Thank you for the review Coleen!
Lois

On 9/18/2016 11:23 PM, Coleen Phillimore wrote:
> 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
>>>>>
>>>
>>
>