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

[Jiangli Zhou]

Hi Calvin,

> On Oct 10, 2017, at 11:53 AM, Calvin Cheung <calvin.cheung at oracle.com> wrote:
> 
> I ran into some runtime issue when creating the _java_platform_loader before initPhase2.
> I've filed the following to track the above issue:
>    https://bugs.openjdk.java.net/browse/JDK-8189120
> 
> I'm going with the fix similar to version.02 - creating the system and platform loaders after initPhase3.
> updated webrev:
>    http://cr.openjdk.java.net/~ccheung/8185694/webrev.04/ <http://cr.openjdk.java.net/~ccheung/8185694/webrev.04/>

This looks good to me.

SystemDictionary::compyte_java_loader() calls CDS_ONLY(SystemDictionaryShared::initialize()). The function name initialize() is misleading. Since you are touching the related code, could you please file a bug so we can clean up SystemDictionaryShared::initialize() API in the near future?

Thanks,
Jiangli

> 
> thanks,
> Calvin
> 
> On 10/5/17, 10:38 PM, David Holmes wrote:
>> On 6/10/2017 3:28 PM, Calvin Cheung wrote:
>>> On 10/5/17, 6: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 }
>>>> 
>>>> 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.
>>> We're obtaining the _java_system_loader after initPhase3 even before this change. Roughly, the calling sequence of initPhase3 is as follows:
>>> 
>>> call_initPhase3()
>>>     -> ClassLoader.initPhase3()
>>>         -> ClassLoader.initSystemClassLoader()  which contains the following code:
>>> 
>>>         String cn = System.getProperty("java.system.class.loader");
>>>         if (cn != null) {
>>>             try {
>>>                 // custom class loader is only supported to be loaded from unnamed module
>>>                 Constructor<?> ctor = Class.forName(cn, false, builtinLoader)
>>> .getDeclaredConstructor(ClassLoader.class);
>>>                 scl = (ClassLoader) ctor.newInstance(builtinLoader);
>>>             } catch (Exception e) {
>>>                 throw new Error(e);
>>>             }
>>>         } else {
>>>             scl = builtinLoader;
>>>         }
>>>         return scl;
>>> 
>>>         So initSystemClassLoader() will either return the built-in system loader or a custom loader if it exists.
>>> 
>>> We use the getSystemClassLoader API to obtain the _java_system_loader:
>>> 
>>>     public static ClassLoader getSystemClassLoader() {
>>>         switch (VM.initLevel()) {
>>>             case 0:
>>>             case 1:
>>>             case 2:
>>>                 // the system class loader is the built-in app class loader during startup
>>>                 return getBuiltinAppClassLoader();
>>>             case 3:
>>>                 String msg = "getSystemClassLoader should only be called after VM booted";
>>>                 throw new InternalError(msg);
>>>             case 4:
>>>                 // system fully initialized
>>>                 assert VM.isBooted() && scl != null;
>>>                 SecurityManager sm = System.getSecurityManager();
>>>                 if (sm != null) {
>>>                     checkClassLoaderPermission(scl, Reflection.getCallerClass());
>>>                 }
>>>                 return scl;
>>>             default:
>>>                 throw new InternalError("should not reach here");
>>>         }
>>>     }
>>> 
>>>     So the _java_system_loader will either be the built-in system loader or a custom loader. (case 4 in the above)
>>> 
>>>     I don't quite understand why the check in line 184 is required?
>>>     Is it for checking if a given class_loader is the same type (like an instanceof) as the built-in system loader?
>> 
>> I believe it is checking if the loader is the built-in default system loader, both to account for the case where/if SystemDictionary::is_system_class_loader is called prior to initPhase3 completing; and also to account for encountering the default-built-in loader when the custom system loader delegates to it.
>> 
>> I'd have to examine every call path to SystemDictionary::is_system_class_loader to check all the details.
>> 
>> David
>> -----
>> 
>>> thanks,
>>> Calvin
>>>> 
>>>> 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
>>>>>