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

[harold seigel]

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