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

[Lois Foltan]

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/

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