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

[David Holmes]

Hi Lois,

On 14/09/2016 5:28 AM, Lois Foltan wrote:
>
> On 9/12/2016 3:22 PM, Karen Kinnear wrote:
>> Lois,
>>
>> Thank you for fixing this.
>>
>> I had a couple of questions/comments.
>>
>> 1) I was delighted with your finding that the hash table entries
>> already handle order access correctly.
>> Thank you for adding the clarifying comment.
> Glad you found that useful!
>
>>
>> 2) In moduleEntry.hpp
>> Did you want to remove module() and set_module() so we always go
>> through the accessors that take order_access into account?
>> When is it safe not to use them?
> In general it is safe to use module() for any ModuleEntry.  When modules
> are defined to the VM, their java.lang.reflect.Module is known at
> definition time, so their _module field within their ModuleEntry is set
> at the time the ModuleEntry is created.  Thus at the point the
> ModuleEntry is "published" to the world, its corresponding _module field
> is valid.  There are two exceptions to this, however:
>
> - java.base
> Its ModuleEntry is created very early in start up prior to the VM
> knowing about its corresponding java.lang.reflect.Module.  Thus the need
> for the changes in this webrev.
>
> - the unnamed module of the boot class loader
> Its ModuleEntry is created at the time the boot loader's
> ModuleEntryTable is created, however its java.lang.reflect.Module is not
> known until the VM is notified via a call to
> JVM_SetBootLoaderUnnamedModule which is done very early in module system
> initialization.  Since no classes are allowed to be loaded pre module
> system initialization that are not defined to java.base, there should
> not be any multi-threaded issues during start up around this unnamed
> ModuleEntry's _module field.
>
>>
>> 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.

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
>