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