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

Mon May 8 18:00:09 UTC 2017

Hi David,

Please see this updated webrev: 
http://cr.openjdk.java.net/~hseigel/bug_8178604.3/webrev/index.html

It contains the changes you requested below.

Note that the _exploded_entries table gets created very early during 
SystemDictionary initialization, before initializing the pre-loaded 
classes.  So, the code in load_class() at line ~1503 can assume that 
_exploded_entries is not null.

Thanks, Harold

On 5/4/2017 5:32 PM, David Holmes wrote:
> Hi Harold,
>
> On 5/05/2017 3:47 AM, 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.
>
> I think this highlights uncertainty about the concurrent behaviour in 
> relation to this code. On the one hand you are worried about an 
> initialization race and so use the lock, but when you do:
>
> 1500       // The exploded build entries can be added to at any time 
> so pass 'true' for
> 1501       // need_lock so that a lock is taken out when searching them.
> 1502       assert(_exploded_entries != NULL, "No exploded build 
> entries present");
> 1503       stream = search_module_entries(_exploded_entries, 
> class_name, file_name, true, CHECK_NULL);
>
> you load _exploded_entries with no lock held and so must be certain 
> that you can not possibly read a null value here.
>
> The needs_lock parameter seems superfluous - you know the condition 
> for locking is that the list is the _exploded_entries, and you assert 
> that. So no need to pass in needs_lock, just grab the lock when 
> dealing with _exploded_entries.
>
> Cheers,
> David
>
>> 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
>>>>>>>
>>>>>>
>>>>>
>>>>
>>