RFR(M): 8164011: --patch-module support for CDS

[Daniel D. Daugherty]

On 9/14/16 12:52 PM, Calvin Cheung wrote:
> Hi Ioi,
>
> I think I've addressed all your review comments in this updated webrev:
> http://cr.openjdk.java.net/~ccheung/8164011/webrev.01/

src/share/vm/classfile/classLoader.cpp
     L1708:           if ((st.st_mode & S_IFREG) != S_IFREG) { // is 
directory
     L1709:             vm_exit_during_initialization(
     L1710:               "Cannot have directory in --patch-module 
during dumping", buf);

         This check should be done like this:

             if ((st.st_mode & S_IFMT) != S_IFREG) { // is not a regular 
file
               vm_exit_during_initialization(
                 "--patch-module requires a regular file during 
dumping", buf);

src/share/vm/classfile/classLoader.hpp
     No comments.

src/share/vm/classfile/sharedPathsMiscInfo.cpp
     L152:         if ((st.st_mode & S_IFREG) != S_IFREG) {
     L153:           return fail("Wanted a file but it has become a 
directory");
     L154:         }

         This check should be done like this:

             if ((st.st_mode & S_IFMT) != S_IFREG) {
               return fail("Did not get a regular file as expected.");
             }

     L181           if ((strncmp(module_name, path, n) != 0 ) ||
         nit: extra blank after '0'.

src/share/vm/classfile/sharedPathsMiscInfo.hpp
     No comments.

src/share/vm/memory/filemap.cpp
     L952:   // More check will be performed on individual 
--patch-module entry in the
         typo: 'check' -> 'checks'

src/share/vm/memory/filemap.hpp
     No comments.

src/share/vm/runtime/arguments.cpp
     No comments.

test/runtime/modules/PatchModule/PatchModuleCDS.java
     L43:         // Case 1: Test that --patch-module and -Xshare:dump 
are compatibable
         Typo: 'compatibable' -> 'compatible'

     L76:             .shouldContain("Cannot have directory in 
--patch-module during dumping");
         Update to the new error message if you choose to change it above.

Dan

>
> The jdk changes for exporting the ZIP_FreeEntry function:
> http://cr.openjdk.java.net/~ccheung/8164011/jdk/webrev.00/
>
> thanks,
> Calvin
>
>
> On 9/12/16, 9:00 PM, Calvin Cheung wrote:
>>
>>
>> On 9/12/16, 3:20 PM, Ioi Lam wrote:
>>> Hi Calvin,
>>> *
>>> **src/share/vm/classfile/classLoader.cpp*
>>>
>>>    There's a memory leak here - the entry is not freed:
>>>
>>>      322 bool ClassPathZipEntry::stream_exists(const char* name) {
>>>      323   // enable call to C land
>>>      324   JavaThread* thread = JavaThread::current();
>>>      325   ThreadToNativeFromVM ttn(thread);
>>>      326   // check whether zip archive contains name
>>>      327   jint name_len, filesize;
>>>      328   jzentry* entry = (*FindEntry)(_zip, name, &filesize, 
>>> &name_len);
>>>      329   return (entry != NULL) ? true : false;
>>>      330 }
>>>
>>>    I think you need to add a new function pointer like this, and call
>>>    it if <entry> is not NULL.
>>>       FreeEntry    = CAST_TO_FN_PTR(FreeEntry_t, os::dll_lookup(handle,
>>>    "ZIP_FreeEntry"));
>>>
>>>    But, ZIP_FreeEntry (in
>>>    jdk/src/java.base/share/native/libzip/zip_util.c) is not exported.
>>>    To export it, you need to edit all the files under
>>>    jdk/make/mapfiles/libzip (grep for ZIP_FindEntry as an example).
>> It turns out changing only jdk/make/mapfiles/libzip/mapfile-vers is 
>> enough.
>> The other three files already contain ZIP_FreeEntry.
>>
>> While testing my change, I noticed the ReadMappedEntry in 
>> ClassLoader::load_zip_library() is NULL:
>>   ReadMappedEntry = CAST_TO_FN_PTR(ReadMappedEntry_t, 
>> os::dll_lookup(handle, "ZIP_ReadMappedEntry"));
>>
>> It will be used in ClassPathZipEntry::open_entry(). I did a search in 
>> the jdk/src/java.base/share/native/libzip and couldn't find the 
>> function. This is unrelated to the current change. I can file a 
>> separate bug if needed.
>>>
>>>    This line can be removed
>>>    1416   bool found;
>>>
>>>    and these lines:
>>>    1444           found = e->stream_exists(file_name);
>>>    1445           if (found) {
>>>    can be changed to "if (e->stream_exists(file_name) {"
>>>
>>>    Typo on line 1489: exploeded -> exploded
>>>
>>>    This variable seems to be redundant:
>>>    1504   bool found_patch_mod_entry = false;
>>>
>>>    I think it's better to move 1523~1525 into here:
>>>
>>>    1517 #if INCLUDE_CDS
>>>    1518       if (is_in_patch_module(file_name)) {
>>>                       tty->print_cr("Preload Warning: Skip archiving
>>>    class %s found in --patch-module entry", class_name);
>>>                       return NULL;
>>>                    }
>>>    1519 #endif
>> I've fixed the above.
>>>
>>> *test/runtime/modules/PatchModule/PatchModuleCDS.java**
>>> *
>>>
>>>    I think this line should be removed. High level description of the
>>>    test should be included as @summary in the header.
>>>    40         System.out.println("Test that --patch-module and
>>>    -Xshare:dump are compatibable");
>>>
>>>    Is mods/java.naming supposed to be a directory that doesn't exist? I
>>>    don't see any code in the test that creates this. If so, it's better
>>>    to use something like "java.naming=no/such/directory"
>>>    46 "--patch-module=java.naming=mods/java.naming",
>>
>> I will fix the above and send a new webrev after more testing.
>>
>> thanks,
>> Calvin
>>>
>>> Thanks
>>> - Ioi
>>>
>>> On 9/12/16 11:01 AM, Calvin Cheung wrote:
>>>> Hi,
>>>>
>>>> Please review the following change to allow the use of the 
>>>> --patch-module option with CDS.
>>>>
>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8164011
>>>> webrev: http://cr.openjdk.java.net/~ccheung/8164011/webrev.00/
>>>>
>>>> During dump time, this change checks if a class can be found in the 
>>>> jar entries of the _patch_mod_entries. If so, the class won't be 
>>>> included in the shared archive. It currently does not support 
>>>> non-jar entries in the _patch_mod_entries.
>>>>
>>>> Tested with JPRT and hotspot/runtime tests.
>>>>
>>>> thanks,
>>>> Calvin
>>>