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

[Calvin Cheung]

Thanks Coleen.

Calvin

On 10/10/17, 1:21 PM, coleen.phillimore at oracle.com wrote:
>
> This looks good to me.
>
> Coleen
>
> On 10/10/17 2:53 PM, Calvin Cheung 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/
>>
>> 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
>>>>>>
>