RFR (M) JDK-8163406: The fixup_module_list must be protected by Module_lock when inserting new entries
Lois Foltan
lois.foltan at oracle.com
Fri Sep 9 22:29:23 UTC 2016
On 9/8/2016 7:15 PM, Coleen Phillimore wrote:
>
> http://cr.openjdk.java.net/~lfoltan/bug_jdk8163406/webrev/src/share/vm/classfile/javaClasses.cpp.udiff.html
>
>
> + assert(!Universe::is_module_initialized(), "Incorrect
> java.lang.reflect.Module pre module system initialization");
>
>
> Can you assert this here? Can another thread initialize the module
> before this line? It's not locked.
Hi Coleen,
The only time within java_lang_Class::create_mirror() a module should be
NULL is during pre module system initialization, so it is important to
make this assertion. How is this any different than checking for
Universe::is_fully_initialized()?
>
> Also, could you make the module fixup list logic a function called
> from create_mirror()? It's too long with too many different things
> happening. We'd gotten it nice, short and concise at one point!
>
> http://cr.openjdk.java.net/~lfoltan/bug_jdk8163406/webrev/src/share/vm/classfile/moduleEntry.cpp.html
>
Will do.
>
> Can you add a comment before
>
> ModuleEntryTable::patch_javabase_entries
>
> to make it clear that there's a module field in java.lang.Class
> instances?
Will do.
>
> Also, this line should be pulled out of the loop:
>
> 399 Thread* THREAD = Thread::current();
> 400 KlassHandle kh(THREAD, k);
>
>
> Or better yet, remove both and use the implicit constructor for
> KlassHandles in the call to fixup_module_field (KlassHandle doesn't
> use THREAD for anything).
Ok, this is very similar to the code that is used in
Universe::fixup_mirrors which I suspect this code was copied from. Would
you like me to change fixup_mirrors as well?
>
> I think everything except these small comments looks good.
Thanks for the review!
Lois
>
> Coleen
>
> On 9/7/16 11:31 AM, Lois Foltan 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
>
More information about the hotspot-runtime-dev
mailing list