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

[Jiangli Zhou]

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

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

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

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