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

[David Holmes]

Looks good Lois!

Thanks,
David

On 17/09/2016 5:35 AM, 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
>>>>
>>
>