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

Jiangli Zhou jiangli.zhou at oracle.com
Mon May 7 21:58:35 UTC 2018 

> On May 7, 2018, at 11:33 AM, Calvin Cheung <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.

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.
>> 
>> - 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] == '\\' && search_path[3] == '\0'))) {


Looks better.

> Updated webrev:
> 
>     http://cr.openjdk.java.net/~ccheung/8202289/webrev.02/ <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> <mailto: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> <mailto: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/ <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 <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> <mailto:calvin.cheung at oracle.com>  wrote:
>>>>>>> 
>>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8202289 <https://bugs.openjdk.java.net/browse/JDK-8202289>
>>>>>>> webrev: http://cr.openjdk.java.net/~ccheung/8202289/webrev.00/ <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