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

Thu Oct 5 18:02:12 UTC 2017

> On Oct 4, 2017, at 10:02 PM, David Holmes <david.holmes at oracle.com> wrote:
> 
> 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.

I agree with David. The checks for SystemDictionary::jdk_internal_loader_ClassLoaders_AppClassLoader_klass() and SystemDictionary::jdk_internal_loader_ClassLoaders_PlatformClassLoader_klass() look strange. They should be investigated and cleaned up.

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

The following in Modules::define_module() is related. Calvin, maybe the _java_platform_loader is not yet set up by the time this code path is entered?

    // Only modules defined to either the boot or platform class loader, can define a "java/" package.
    if (!h_loader.is_null() &&
        !SystemDictionary::is_platform_class_loader(h_loader()) &&
        (strncmp(package_name, JAVAPKG, JAVAPKG_LEN) == 0 &&
          (package_name[JAVAPKG_LEN] == '/' || package_name[JAVAPKG_LEN] == '\0'))) {
      const char* class_loader_name = SystemDictionary::loader_name(h_loader());
      size_t pkg_len = strlen(package_name);
      char* pkg_name = NEW_RESOURCE_ARRAY_IN_THREAD(THREAD, char, pkg_len);
      strncpy(pkg_name, package_name, pkg_len);
      StringUtils::replace_no_expand(pkg_name, "/", ".");
      const char* msg_text1 = "Class loader (instance of): ";
      const char* msg_text2 = " tried to define prohibited package name: ";
      size_t len = strlen(msg_text1) + strlen(class_loader_name) + strlen(msg_text2) + pkg_len + 1;
      char* message = NEW_RESOURCE_ARRAY_IN_THREAD(THREAD, char, len);
      jio_snprintf(message, len, "%s%s%s%s", msg_text1, class_loader_name, msg_text2, pkg_name);
      THROW_MSG(vmSymbols::java_lang_IllegalArgumentException(), message);
    }

Thanks,
Jiangli

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