RFR(S): 8185694: Replace SystemDictionaryShared::_java_platform_loader with SystemDictionary::is_platform_class_loader()
Calvin Cheung
calvin.cheung at oracle.com
Thu Oct 5 19:26:49 UTC 2017
On 10/5/17, 11:02 AM, Jiangli Zhou wrote:
>
>> On Oct 4, 2017, at 10:02 PM, David Holmes <david.holmes at oracle.com
>> <mailto: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/
>>>>>>> <http://cr.openjdk.java.net/%7Eccheung/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/
>>> <http://cr.openjdk.java.net/%7Eccheung/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.
I've tried simplifying the is_system_class_loader() as mentioned above
and it seems to work but more testing is required.
>
>>
>>>>>
>>>>> 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?
Thanks for the hint. The problem is that the following code was called
before SystemDictionary::compute_java_loaders().
I'm experimenting moving the SystemDictionary::compute_java_loaders() up
during vm init prior to call_initPhase2() like the following:
// cache the system and platform class loaders
SystemDictionary::compute_java_loaders(CHECK_JNI_ERR);
// This will initialize the module system. Only java.base classes can be
// loaded until phase 2 completes
call_initPhase2(CHECK_JNI_ERR);
thanks,
Calvin
>
> // 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, "/", ".");
> constchar* msg_text1 = "Class loader (instance of): ";
> constchar* 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/
>>>>>>>>>> <http://cr.openjdk.java.net/%7Eccheung/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
>
More information about the hotspot-runtime-dev
mailing list