RFR(S): 8202289: Non-empty directory in module path is not handled properly at CDS/AppCDS dump time

Ioi Lam ioi.lam at oracle.com
Thu May 10 03:51:15 UTC 2018 

Looks good. Thanks!

- Ioi


On 5/9/18 5:37 PM, Calvin Cheung wrote:
>
>
> On 5/8/18, 2:24 PM, Ioi Lam wrote:
>> I think we do need to check the results of get_canonical_path. On 
>> Linux, the canonical path can be longer than the source path, which 
>> means ENAMETOOLONG may be returned.
>>
>> http://hg.openjdk.java.net/jdk/hs/file/tip/src/java.base/unix/native/libjava/canonicalize_md.c#l253 
>>
>>
>> So instead of assert, it's best to check for failure and exit the 
>> dumping process.
> I've made the above change.
> Updated webrev:
>
>     http://cr.openjdk.java.net/~ccheung/8202289/webrev.04/
>
> thanks,
> Calvin
>>
>> The rest of the changes look OK to me.
>>
>> Thanks
>>
>> - Ioi
>>
>>
>>
>> On 5/8/18 10:41 AM, Calvin Cheung wrote:
>>>
>>>
>>> On 5/7/18, 2:58 PM, Jiangli Zhou wrote:
>>>>
>>>>> On May 7, 2018, at 11:33 AM, Calvin Cheung 
>>>>> <calvin.cheung at oracle.com <mailto:calvin.cheung at oracle.com>> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 5/7/18, 10:28 AM, Jiangli Zhou wrote:
>>>>>> Hi Calvin,
>>>>>>
>>>>>> -classLoader.cpp
>>>>>>
>>>>>> The get_canonical_path() call for ‘path’ should be moved out of 
>>>>>> the ‘for' loop.
>>>>>>
>>>>>> 1569           get_canonical_path(path, canonical_src_path, 
>>>>>> JVM_MAXPATHLEN)) {
>>>>> Fixed.
>>>>>> To make the code more readable, please rename ‘canonical_path’ to 
>>>>>> ‘canonical_path_table_entry’ (or ‘canonical_path_table_entry_name’)
>>>>> I chose the shorter one.
>>>>>>   and ‘canonical_src_path’ to ‘canonical_class_src_path’.
>>>>> Renamed.
>>>>>> BTW, when we create the path entries, the paths are already 
>>>>>> converted to canonical forms. I think there is no need to call 
>>>>>> get_canonical_path() on ‘ent->name()’. Can you please double check?
>>>>> The call to get_canonical_path() is only for the non-directory 
>>>>> case (please refer to ClassLoader::create_class_path_entry() for 
>>>>> details).
>>>>> I did try get_canonical_path() for the directory case but it 
>>>>> didn't work during runtime; it failed in 
>>>>> SystemDictionaryShared::is_shared_class_visible_for_classloader() 
>>>>> in the following check:
>>>>>
>>>>> (strcmp(ent->name(), 
>>>>> ClassLoader::skip_uri_protocol(mod_entry->location()->as_C_string())) 
>>>>> == 0)) {
>>>>>
>>>>> The location() in the mod_entry isn't in canonical form. If we 
>>>>> convert it to canonical form during runtime, there maybe 
>>>>> performance impact.
>>>>> I think the proper way of fixing it is storing the location() in 
>>>>> canonical form when a module entry is being defined. I'm not sure 
>>>>> if it is possible for the core lib to pass the location in 
>>>>> canonical form to jvm when a module entry is being defined.
>>>>> I can file a follow-up RFE to address that.
>>>>
>>>> Please file a RFE.
>>> I've filed JDK-8202750 
>>> <https://bugs.openjdk.java.net/browse/JDK-8202750>
>>>>
>>>> There is no need to check ‘res’ in very iteration since it doesn’t 
>>>> change after being initialized by get_canonical_path(). Also, I 
>>>> think we are not handling get_canonical_path() (both calls) failure 
>>>> properly. However, I question why get_canonical_path() would fail 
>>>> when given a valid path in this particular case. So I think we 
>>>> should either assert get_canonical_path() returns true, or report a 
>>>> fatal error for dumping.
>>> I've made the change and added assert after get_canonical_path() at 
>>> both calls.
>>>
>>> Updated webrev:
>>>
>>>     http://cr.openjdk.java.net/~ccheung/8202289/webrev.03/
>>>
>>> thanks,
>>> Calvin
>>>>>> - os_windows.cpp
>>>>>>
>>>>>> Please fix indentation at line 4390&  4391.
>>>>> That was done on purpose because the condition in line 4390 is 
>>>>> '||' with line 4391.
>>>>> I'm further indenting line 4391 to make it clearer.
>>>>>
>>>>> 4389   if (search_path[1] == ':'&&
>>>>> 4390        (search_path[2] == '\0' ||
>>>>> 4391          (search_path[2] == '\\' <smb://%27> &&  
>>>>> search_path[3] == '\0'))) {
>>>>
>>>>
>>>> Looks better.
>>>>
>>>>> Updated webrev:
>>>>>
>>>>> http://cr.openjdk.java.net/~ccheung/8202289/webrev.02/
>>>>
>>>>
>>>> Thanks,
>>>> Jiangli
>>>>
>>>>>
>>>>> thanks,
>>>>> Calvin
>>>>>
>>>>>> Thanks,
>>>>>> Jiangli
>>>>>>
>>>>>>
>>>>>>> On May 7, 2018, at 9:12 AM, Calvin 
>>>>>>> Cheung<calvin.cheung at oracle.com>  wrote:
>>>>>>>
>>>>>>> Hi Jiangli,
>>>>>>>
>>>>>>> On 5/3/18, 5:33 PM, Jiangli Zhou wrote:
>>>>>>>> Hi Calvin,
>>>>>>>>
>>>>>>>>
>>>>>>>>> On May 3, 2018, at 4:18 PM, Calvin 
>>>>>>>>> Cheung<calvin.cheung at oracle.com> wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 5/3/18, 3:42 PM, Jiangli Zhou wrote:
>>>>>>>>>> Hi Calvin,
>>>>>>>>>>
>>>>>>>>>> I think we don’t need to check the module path entries if 
>>>>>>>>>> only boot classes are archived. The ‘end’ is set to 
>>>>>>>>>> app_class_paths_start_index when there are only boot classes 
>>>>>>>>>> are archived. If there are app module classes loaded from 
>>>>>>>>>> jars/directories at dump time, has_platform_or_app_classes() 
>>>>>>>>>> should return true and ‘end’ is set to the end of the shared 
>>>>>>>>>> path table, which includes all module path entries. The 
>>>>>>>>>> following loop is not needed. The existing loop in the code 
>>>>>>>>>> covers all cases with different ‘end’ value based on the type 
>>>>>>>>>> of classes loaded in the archive.
>>>>>>>>>>   366   // may need to check module path entries.
>>>>>>>>>>   367   if ((end == 
>>>>>>>>>> ClassLoaderExt::app_class_paths_start_index())&& 
>>>>>>>>>> (ClassLoader::num_module_path_entries()> 0)) {
>>>>>>>>>>   368     for (int i = 
>>>>>>>>>> ClassLoaderExt::app_module_paths_start_index(); i<  
>>>>>>>>>> _shared_path_table_size; i++) {
>>>>>>>>>>   369       SharedClassPathEntry* e = shared_path(i);
>>>>>>>>>>   370       has_nonempty_dir = check_nonempty_dir(e);
>>>>>>>>>>
>>>>>>>>>>   371     }
>>>>>>>>>>   372   }
>>>>>>>>>>
>>>>>>>>> The has_platform_or_app_classes() will return false if 
>>>>>>>>> ClassLoaderExt::record_result() hasn't been called while a 
>>>>>>>>> class is being loaded during dumping.
>>>>>>>>> This could happen before this proposed change if one specifies 
>>>>>>>>> a --module-path with an non-empty directory containing a 
>>>>>>>>> module. While the class will be loaded during dumping but its 
>>>>>>>>> classpath_index will still be -1. Therefore, the 
>>>>>>>>> has_platform_or_app_classes() will return false and the module 
>>>>>>>>> path entries won't be checked without the above block of code.
>>>>>>>> With the module directory path being added to the path table 
>>>>>>>> now, it should record the correct path table index instead of 
>>>>>>>> -1 for module classes loaded from directory at dump time.
>>>>>>>>
>>>>>>>> I applied your patch and stepped through 
>>>>>>>> ClassLoader::record_result() with your test case. The following 
>>>>>>>> path comparison fails due the the extra ‘/‘ in ‘path’. That why 
>>>>>>>> it fails to find the correct entry in the path table, which 
>>>>>>>> causes the class type cannot be correctly identified.
>>>>>>>>
>>>>>>>>          if (strcmp(canonical_path, 
>>>>>>>> os::native_path((char*)path)) == 0) {os::dir_is_empty
>>>>>>>>
>>>>>>>> (gdb) p canonical_path
>>>>>>>> $74 = 0x7ffff0aea0f0 
>>>>>>>> “<snip>/jdk/open/test/hotspot/jtreg/runtime/appcds/jigsaw/modulepath/JTwork/scratch/mods/com.simple"
>>>>>>>> (gdb) p os::native_path((char*)path)
>>>>>>>> $75 = 0x7ffff7fd4a65 
>>>>>>>> “<snip>/jdk/open/test/hotspot/jtreg/runtime/appcds/jigsaw/modulepath/JTwork/scratch/mods/com.simple/"
>>>>>>>>
>>>>>>>> The trailing ‘/‘ should be excluded for the path comparison. 
>>>>>>>> JDK-8202519 is a related issue in the same area. It might be 
>>>>>>>> better to fix it together with this one.
>>>>>>> Thanks for debugging the issue.
>>>>>>> I've fixed it by calling get_canonical_path() on the "path" 
>>>>>>> before doing strcmp(). It should also fix the bug in JDK-8202519 
>>>>>>> since a buffer is being allocated for get_canonical_path() to 
>>>>>>> write into. The os::native_path() on linux does nothing and just 
>>>>>>> returns the same string.
>>>>>>>
>>>>>>> I also discovered a problem in os::dir_is_empty() on windows. 
>>>>>>> When a directory path is passed into the function, it may have 
>>>>>>> the following form with forward slashes: C:/somedir
>>>>>>> The fix is to call os::native_path() which essentially converts 
>>>>>>> the forward slashes to back slashes so that subsequent windows 
>>>>>>> api call returns the correct result.
>>>>>>>
>>>>>>>> Once the above issue is fixed, the class type should be 
>>>>>>>> recorded correctly for a module classes loaded from directory 
>>>>>>>> at dump time. Then ClassLoaderExt::record_result() can set 
>>>>>>>> ‘has_app_classes’ flag properly. And, no change should be 
>>>>>>>> needed in FileMapInfo::check_nonempty_dir_in_shared_path_table().
>>>>>>> I don't need to touch filemap.cpp in the following updated webrev:
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~ccheung/8202289/webrev.01/
>>>>>>>
>>>>>>> thanks,
>>>>>>> Calvin
>>>>>>>>>> We also need a unit test for non-empty directory in module 
>>>>>>>>>> path. Please add.
>>>>>>>>> It is already in my webrev:
>>>>>>>>> http://cr.openjdk.java.net/~ccheung/8202289/webrev.00/test/hotspot/jtreg/runtime/appcds/jigsaw/modulepath/MainModuleOnly.java.sdiff.html 
>>>>>>>>>
>>>>>>>> Ok. I was expecting a separate unit test. I’m fine with an 
>>>>>>>> added test case in MainModuleOnly.java.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Jiangli
>>>>>>>>
>>>>>>>>> thanks,
>>>>>>>>> Calvin
>>>>>>>>>> Thanks,
>>>>>>>>>> Jiangli
>>>>>>>>>>
>>>>>>>>>>> On May 3, 2018, at 3:24 PM, Calvin 
>>>>>>>>>>> Cheung<calvin.cheung at oracle.com> wrote:
>>>>>>>>>>>
>>>>>>>>>>> bug:https://bugs.openjdk.java.net/browse/JDK-8202289
>>>>>>>>>>> webrev:http://cr.openjdk.java.net/~ccheung/8202289/webrev.00/
>>>>>>>>>>>
>>>>>>>>>>> In ClassLoaderExt::process_module_table(), it adds all 
>>>>>>>>>>> entries from the ModuleEntryTable with the "file:" protocol 
>>>>>>>>>>> to the _module_path_entries and eventually to the 
>>>>>>>>>>> _shared_path_table.
>>>>>>>>>>> The FileMapInfo::check_nonempty_dir_in_shared_path_table() 
>>>>>>>>>>> will check for non-empty directory in the module path.
>>>>>>>>>>>
>>>>>>>>>>> Ran all CDS and AppCDS tests locally on linux-x64.
>>>>>>>>>>> Will run hs-tier{1,2,3} tests.
>>>>>>>>>>>
>>>>>>>>>>> thanks,
>>>>>>>>>>> Calvin
>>>>
>>

More information about the hotspot-runtime-dev mailing list