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

[Coleen Phillimore]

On 9/9/16 6:29 PM, Lois Foltan wrote:
>
> 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?

No need.   It'll get cleaned up soon.  I just wanted new code to not use 
this idiom.

thanks,
Coleen
>
>>
>> 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
>>
>