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

[Calvin Cheung]

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

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
>> webrev: http://cr.openjdk.java.net/~ccheung/8202289/webrev.00/ 
>> <http://cr.openjdk.java.net/%7Eccheung/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
>