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

harold seigel harold.seigel at oracle.com
Tue May 9 13:14:21 UTC 2017 

Hi David,

See comments embedded below.

Thanks, Harold


On 5/8/2017 8:52 PM, David Holmes wrote:
> Hi Harold,
>
> On 9/05/2017 4:00 AM, harold seigel wrote:
>> 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.
>
> Thank you. I will point out that Lois originally suggested the same 
> changes.
>
>> 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.
>
> Which thread executes ClassLoader::add_to_exploded_build_list, and 
> which thread executes load_class() first? If they are guaranteed to be 
> the same thread then you don't need locking on the construction. If 
> they can be different threads then there must be some synchronization 
> between them that ensures _exploded_entries is properly visible after 
> construction.
They are the same thread.  At startup, the thread calls 
SystemDictionary::initialize_preloaded_classes(), which calls 
ClassLoader::classLoader_init2(), which calls add_to_exploded_build_list().

Control then returns back to 
SystemDictionary::initialize_preloaded_classes() which eventually calls 
SystemDictionary::load_instance_class(), which calls 
ClassLoader::load_class().   Here's the call stack showing these calls:

    V  [libjvm.so+0x974c0c] ClassLoader::load_class(Symbol*, bool,
    Thread*)+0x41c
    V  [libjvm.so+0x1648339]
    SystemDictionary::load_instance_class(Symbol*, Handle, Thread*)+0x819
    V  [libjvm.so+0x1648e25]
    SystemDictionary::resolve_instance_class_or_null(Symbol*, Handle,
    Handle, Thread*)+0x985
    V  [libjvm.so+0x16494ba] SystemDictionary::resolve_or_null(Symbol*,
    Handle, Handle, Thread*)+0x5a
    V  [libjvm.so+0x16498fe] SystemDictionary::resolve_or_fail(Symbol*,
    Handle, Handle, bool, Thread*)+0x1e
    V  [libjvm.so+0x1649aa9]
    SystemDictionary::initialize_wk_klass(SystemDictionary::WKID, int,
    Thread*)+0x149
    V  [libjvm.so+0x1649bfe]
    SystemDictionary::initialize_wk_klasses_until(SystemDictionary::WKID,
    SystemDictionary::WKID&, Thread*)+0x5e
    V  [libjvm.so+0x1649dba] 
    SystemDictionary::*initialize_preloaded_classes*(Thread*)+0xea
    V  [libjvm.so+0x164a28a] SystemDictionary::initialize(Thread*)+0x26a
    V  [libjvm.so+0x16cd7e0]  Universe::genesis(Thread*)+0x7c0
    V  [libjvm.so+0x16ce4cc]  universe2_init()+0x2c
    V  [libjvm.so+0xe0f268]  init_globals()+0xa8
    V  [libjvm.so+0x1696e6f]  Threads::create_vm(JavaVMInitArgs*,
    bool*)+0x2df
                                 ...

So, is the existing synchronization in this change sufficient?

Thanks, Harold
>
> Thanks,
> David
> -----
>
>> 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
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>
>>

More information about the hotspot-runtime-dev mailing list