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

[Ioi Lam]

Looks good. Thanks!

- Ioi


On 10/10/17 11:53 AM, 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
>>>>>