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

[Lois Foltan]

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
>>>>>>
>>>>>
>>>>
>>>
>