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

[Calvin Cheung]

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
>>>>