RFR 8178604: JVM does not allow defining boot loader modules in exploded build after module system initialization

[harold seigel]

Thanks Lois!

Harold


On 5/4/2017 2:05 PM, Lois Foltan wrote:
> Thank you, looks good.
> Lois
>
> On 5/4/2017 1:47 PM, harold seigel wrote:
>
>> Hi,
>>
>> Please review this updated webrev:
>>
>>     http://cr.openjdk.java.net/~hseigel/bug_8178604.2/webrev/index.html
>>
>> The updated webrev takes out a lock when creating _exploded_entries.  
>> It also contains additional comments and an assert() to address 
>> Lois's concerns with reduced code readability involving what 
>> situations warrant taking out a lock in 
>> ClassLoader::search_module_entries().
>>
>> I don't think there needs to be two different initial entry points as 
>> suggested by David below.  The method behaves slightly differently 
>> depending on the value of its 'need_lock' parameter.  But, there is 
>> enough common code for it to remain one method.
>>
>> Thanks, Harold
>>
>> On 5/3/2017 12:59 AM, David Holmes wrote:
>>> On 3/05/2017 7:11 AM, Lois Foltan wrote:
>>>> On 5/2/2017 9:03 AM, harold seigel wrote:
>>>>> Hi Lois,
>>>>>
>>>>> Thanks for your comments.  Please see my in-line responses.
>>>>>
>>>>> Harold
>>>>>
>>>>>
>>>>> On 5/1/2017 4:14 PM, Lois Foltan wrote:
>>>>>> Hi Harold,
>>>>>>
>>>>>> Looks good. A couple of comments:
>>>>>>
>>>>>> src/share/vm/classfile/classLoader.cpp
>>>>>>     line #828 - please take out the Module_lock when the
>>>>>> _exploded_entries is created
>>>>> This fix does not change the possible need for synchronization when
>>>>> _exploded_entries gets created during module system initialization.
>>>>> Are you saying that the existing code is wrong and should have taken
>>>>> out the Module_lock before creating _exploding_entries?
>>>>
>>>> The method ClassLoader::add_to_exploded_build_list can be called at
>>>> anytime (during module system initialization and post module system
>>>> initialization).  Thus it seems prudent that no matter what the 
>>>> current
>>>> JVM phase, that any access to ClassLoader::_exploding_entries be
>>>> protected via a lock.  That includes creation as well as insertion 
>>>> into
>>>> the list.
>>>
>>> If creation is not guaranteed to occur before other threads that 
>>> will call the same method and use the _exploded_entries list, then 
>>> synchronization of some form is essential. If it is guaranteed then 
>>> we don't need create time sync just to be prudent - but the 
>>> guarantee must be firmly established and clearly documented.
>>>
>>>>>
>>>>>>     line #1402 - I like the new factored out method
>>>>>> find_first_module_cpe(), however, instead of adding a new 
>>>>>> "need_lock"
>>>>>> parameter, I would rather see code
>>>>>>                          in find_first_module_cpe() that takes the
>>>>>> Module_lock if the module_list parameter equals
>>>>>> ClassLoader::_exploded_entries
>>>>> I think your suggestion makes the code less flexible and does not
>>>>> require potential future callers of search_module_entries() to think
>>>>> about synchronization.  So, I'd prefer to leave that change as is.
>>>>
>>>> I see your point.  My point was based on reduced code readability, the
>>>> further away from the decision to establish the lock, the less 
>>>> apparent
>>>> it is within the new method ClassLoader::find_first_module_cpe() as to
>>>> what situations warranted the lock in the first place.
>>>
>>> Further, this:
>>>
>>> 1495       stream = search_module_entries(_exploded_entries, 
>>> class_name, file_name, true, CHECK_NULL);
>>>
>>> is potentially unsafe if we can race with creation because we first 
>>> access _exploded_entries without holding any lock! And the fact the 
>>> method needs to do different things depending on which "list" was 
>>> passed in suggests to me it should be two different initial entry 
>>> points.
>>>
>>> Thanks,
>>> David
>>> -----
>>>
>>>> Thanks,
>>>> Lois
>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> Lois
>>>>>>
>>>>>> On 5/1/2017 3:36 PM, harold seigel wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> Please review this JDK-10 bug fix to allow defining of modules to
>>>>>>> the boot loader in exploded builds after module system
>>>>>>> initialization.  The fix uses the module lock to synchronize access
>>>>>>> to the _exploded_entries data structure (which is only used by
>>>>>>> exploded builds).
>>>>>>>
>>>>>>> Note that the above capability already exists for regular builds.
>>>>>>>
>>>>>>> Open Webrev:
>>>>>>> http://cr.openjdk.java.net/~hseigel/bug_8178604/webrev/index.html
>>>>>>>
>>>>>>> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8178604
>>>>>>>
>>>>>>> The fix was tested with JCK tests, the JTreg hotspot, java/io,
>>>>>>> java/lang, java/util and other tests, the RBT tier2 -tier5 tests,
>>>>>>> the co-located NSK tests, and with JPRT.  The JTReg and JCK tests
>>>>>>> were run with an exploded build.  Also, the example program in the
>>>>>>> JBS bug was run by hand to verify the fix.
>>>>>>>
>>>>>>> Thanks, Harold
>>>>>>>
>>>>>>
>>>>>
>>>>
>>
>