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

[David Holmes]

Hi Calvin,

On 5/10/2017 2:45 PM, Calvin Cheung wrote:
> On 10/4/17, 4:30 PM, David Holmes wrote:
>> On 5/10/2017 8:22 AM, Calvin Cheung wrote:
>>> On 10/4/17, 2:41 PM, David Holmes wrote:
>>>> Hi Calvin,
>>>>
>>>> On 5/10/2017 3:12 AM, Calvin Cheung wrote:
>>>>> Mandy, David,
>>>>>
>>>>> Thanks for the review.
>>>>>
>>>>> I've made the change based on your suggestion:
>>>>> http://cr.openjdk.java.net/~ccheung/8185694/webrev.01/
>>>>
>>>> share/classfile/systemDictionary.cpp
>>>>
>>>> There's an existing bug that this local is not used:
>>>>
>>>> 132   Klass* system_klass = WK_KLASS(ClassLoader_klass);
>>>>
>>>> (and the variable is poorly named). Now that you need to use 
>>>> WK_KLASS(ClassLoader_klass) twice you should instead use the local.
>>> How about changing the local variable name to class_loader_klass and 
>>> using it at lines 135 and 143?
>>
>> Yep.
> I've made this change.
> Updated webrev:
> http://cr.openjdk.java.net/~ccheung/8185694/webrev.02/
> 
> It also contains the change in threads.cpp - from CHECK_(JNI_ERR) to 
> CHECK_JNI_ERR.
>>
>>>>
>>>> ---
>>>>
>>>> As Mandy alludes these:
>>>>
>>>> 177 // Returns true if the passed class loader is the builtin 
>>>> application class loader
>>>>  178 // or a custom system class loader. A customer system class 
>>>> loader can be
>>>>  179 // specified via -Djava.system.class.loader.
>>>>  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 }
>>>>  187
>>>>  188 // Returns true if the passed class loader is the platform 
>>>> class loader.
>>>>  189 bool SystemDictionary::is_platform_class_loader(oop 
>>>> class_loader) {
>>>>  190   if (class_loader == NULL) {
>>>>  191     return false;
>>>>  192   }
>>>>  193   return (class_loader->klass() == 
>>>> SystemDictionary::jdk_internal_loader_ClassLoaders_PlatformClassLoader_klass()); 
>>>>
>>>>  194 }
>>>>
>>>> would seem to be checking the wrong things - they should just be 
>>>> checking java_system_loader() and java_platform_loader() shouldn't 
>>>> they?
>>> Do you mean lines 184 and 185 could become the following?
>>>
>>>      return (class_loader == _java_system_loader);
> After a closer look at the comment, I think lines 184 and 185 are correct.
> 
> // Returns true if the passed class loader is the builtin application 
> class loader
> // or a custom system class loader.
> 
> I'm afraid something may break and would like to leave it alone for now.

It needs to be looked at - at a minimum file a follow up bug. Both 
conditions imply the loader IS the system (aka application) class loader.

>>>
>>> and line 193 could become the following?
>>>
>>>      return (class_loader == _java_platform_loader);
> I can't change this one either; I tried it and resulting in the 
> following build error:
> 
> Creating interim jimage
> Error occurred during initialization of boot layer
> java.lang.LayerInstantiationException: Class loader (instance of): 
> jdk/internal/loader/ClassLoaders$PlatformClassLoader tried to define 
> prohibited package name: java.sql

Something very weird going on there that needs to be resolved before 
these changes are pushed IMHO. Sorry but there's obviously more 
happening here than meets the eye.

David

> thanks,
> Calvin
>>
>> Yes - exactly.
>>
>>>>
>>>> In the case of is_system_class_loader() it seems odd that it checks 
>>>> a specific hard-wired class as well as the defined 
>>>> _java_system_loader ??
>>> Not sure. That code has been there for a long time.
>>>
>>> Also, both 
>>> SystemDictionary::jdk_internal_loader_ClassLoaders_AppClassLoader_klass() 
>>> and 
>>> SystemDictionary::jdk_internal_loader_ClassLoaders_PlatformClassLoader_klass() 
>>> are being used in the closed source for JavaCalls.
>>
>> Unless you really need to determine that the loader is of an actual 
>> type, you should only be querying java_system_loader() and 
>> java_platform_loader().
>>
>> Thanks,
>> David
>> -----
>>
>>> thanks,
>>> Calvin
>>>>
>>>> Thanks,
>>>> David
>>>> -----
>>>>
>>>>
>>>>
>>>>> thanks,
>>>>> Calvin
>>>>>
>>>>> On 10/4/17, 8:29 AM, mandy chung wrote:
>>>>>>
>>>>>>
>>>>>> On 10/3/17 11:36 PM, David Holmes wrote:
>>>>>>> Hi Calvin,
>>>>>>>
>>>>>>> On 4/10/2017 7:26 AM, Calvin Cheung wrote:
>>>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8185694
>>>>>>>>
>>>>>>>> The open code change for this fix involves adding the 
>>>>>>>> _java_platform_loader to the SystemDictionary class.
>>>>>>>>
>>>>>>>> webrev: http://cr.openjdk.java.net/~ccheung/8185694/webrev.00/
>>>>>>>
>>>>>>> I think you should be calling getPlatformClassLoader rather than 
>>>>>>> poking at the parent field of the system loader. That way you 
>>>>>>> don't have to worry about whether or not the system loader has 
>>>>>>> been customized by the user.
>>>>>> Agree.  ClassLoader::getPlatformClassLoader is the right way to 
>>>>>> get the platform class loader.
>>>>>>
>>>>>> Mandy