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

[Calvin Cheung]

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

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