RFR (M) JDK-8163406: The fixup_module_list must be protected by Module_lock when inserting new entries
david.holmes at oracle.com
Wed Sep 14 00:57:36 UTC 2016
On 14/09/2016 5:28 AM, Lois Foltan wrote:
> On 9/12/2016 3:22 PM, Karen Kinnear wrote:
>> 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:
In this updated code:
803 assert(Universe::is_module_initialized() ||
804 (ModuleEntryTable::javabase_defined() &&
805 (module() ==
806 "Incorrect java.lang.reflect.Module specification while
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.
> Thank you for reviewing!
>>> On Sep 7, 2016, at 11:31 AM, Lois Foltan <lois.foltan at oracle.com> wrote:
>>> Please review the following fix:
>>> Bug: The fixup_module_list must be protected by Module_lock when
>>> inserting new entries
>>> 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.
>>> Full hotspot nightly testing, JCK vm & lang, several iterations of
>>> jaxp tests to test JDK-8164721
More information about the hotspot-runtime-dev