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

[Lois Foltan]

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