RFR(S): 8185694: Replace SystemDictionaryShared::_java_platform_loader with SystemDictionary::is_platform_class_loader()

[David Holmes]

On 6/10/2017 11:38 AM, coleen.phillimore at oracle.com wrote:
> On 10/5/17 9:33 PM, David Holmes wrote:
>> Hi Coleen, Calvin,
>>
>> On 6/10/2017 6:54 AM, coleen.phillimore at oracle.com wrote:
>>> So if you use -Djava.system.loader=myLoader on the command line, 
>>> setting _java_system_loader, then does that mean that the classes 
>>> loaded by 
>>> SystemDictionary::jdk_internal_loader_ClassLoaders_AppClassLoader_klass() 
>>> are not in the system loader?  ie. they can be unloaded?  What is the 
>>> result of the call to SystemDictionary::is_system_class_loader() used 
>>> for?   I guess same question applies to the platform class loader.
>>
>> The classloading delegation hierarchy (as of JDK 9) is:
>> - boot loader (native)
>>   - platform loader (built-in)
>>     - system (aka application) loader (built-in)
>>
>> If the user specifies a custom class for the system loader then it is 
>> loaded by an instance of the default system loader and becomes a 
>> fourth level in the hierarchy, and it is then technically the "system 
>> loader". None of these loaders, or the classes they load can be unloaded.
>>
>> Which is presumably why the code checks both:
>>
>>  180 bool SystemDictionary::is_system_class_loader(oop class_loader) {
>>  181   if (class_loader == NULL) {
>>  182     return false;
>>  183   }
>>  184   return (class_loader->klass() == 
>> SystemDictionary::jdk_internal_loader_ClassLoaders_AppClassLoader_klass() 
>> ||
>>  185           class_loader == _java_system_loader);
>>  186 }
>>
> 
> Okay then this code shouldn't change.

Right.

>> because we need to treat both of these instances as the "system 
>> loader" from a VM perspective? The same does not apply to the platform 
>> loader.
> 
> I don't think we have good tests for this.

There are a few tests that define a custom system loader:

./jdk/java/lang/System/LoggerFinder/
./jdk/java/lang/instrument/CustomSystemLoader/
./jdk/java/lang/ClassLoader/securityManager/ClassLoaderTest.java
./jdk/sun/security/util/Resources/customSysClassLoader/BootMessages.java
./hotspot/jtreg/runtime/modules/ModuleStress/ModuleStress.java
./jdk/java/lang/ClassLoader/CustomSystemLoader/InitSystemLoaderTest.java

but the last one seems to be the extent of actually testing it and it is 
minimal: a check that the system loader is custom loader instance, and 
that the delegation grandparent is the platform loader.

David

> Coleen
>>
>> David
>> -----
>>
>>> thanks,
>>> Coleen
>>>
>>>>
>>>>> The implementation is in closed source.
>>>> Please clean up the closed code to remove those.
>>>>
>>>>
>>>> Thanks,
>>>>
>>>> Jiangli
>>>>
>>>>>> Is this new java_platform_loader function used anywhere?
>>>>> Yes, it is being used in closed source.
>>>>>> Currently 
>>>>>> SystemDictionary::jdk_internal_loader_ClassLoaders_PlatformClassLoader_klass 
>>>>>> is preloaded.  Shouldn't this be removed?  What about 
>>>>>> jdk_internal_loader_ClassLoaders_AppClassLoader?
>>>>> They're being used in lines 184 and 193 in systemDictionary.cpp and 
>>>>> also in closed source.
>>>>>> thread.cpp
>>>>>>
>>>>>> 3752 SystemDictionary::compute_java_loaders(CHECK_(JNI_ERR));
>>>>>>
>>>>>> What is the difference between CHECK_(JNI_ERR) vs CHECK_JNI_ERR? 
>>>>>> Should it simply use CHECK_JNI_ERR as in other places?
>>>>> They are the same, in utilities/exceptions.hpp:
>>>>> #define CHECK_JNI_ERR CHECK_(JNI_ERR)
>>>>>
>>>>> and it expands to the following:
>>>>> __the_thread__); if 
>>>>> ((((ThreadShadow*)__the_thread__)->has_pending_exception())) return 
>>>>> (-1); (void)(0
>>>>>
>>>>> I can change it to CHECK_JNI_ERR.
>>>>>
>>>>> thanks,
>>>>> Calvin
>>>>>> Mandy
>>>
>