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

[David Holmes]

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