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

Calvin Cheung calvin.cheung at oracle.com
Thu Sep 15 17:35:49 UTC 2016


Hi Dan,

Here's an updated webev:
http://cr.openjdk.java.net/~ccheung/8164011/webrev.02/

It passed all jtreg tests under runtime dir.

thanks,
Calvin

On 9/14/16, 7:07 PM, Daniel D. Daugherty wrote:
> On 9/14/16 6:12 PM, Calvin Cheung wrote:
>>
>>
>> On 9/14/16, 4:32 PM, Daniel D. Daugherty wrote:
>>> 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
>> Either way is fine, I think.
>
> No. The S_IFMT value should be used to mask and then a
> comparison made in order to handle implementations where
> there are overlaps in the bits used...
>
>
>> In /usr/include/sys/stat.h:
>> # define S_IFMT        __S_IFMT
>> # define S_IFDIR    __S_IFDIR
>> # define S_IFCHR    __S_IFCHR
>> # define S_IFBLK    __S_IFBLK
>> # define S_IFREG    __S_IFREG
>>
>> In /usr/include/bits/stat.h:
>> #define    __S_IFMT    0170000    /* These bits determine file type.  */
>>
>> /* File types.  */
>> #define    __S_IFDIR    0040000    /* Directory.  */
>> #define    __S_IFCHR    0020000    /* Character device.  */
>> #define    __S_IFBLK    0060000    /* Block device.  */
>> #define    __S_IFREG    0100000    /* Regular file.  */
>
> Notice that S_IFDIR overlaps with S_IFBLK so this programming
> pattern:
>
>     ((st.st_mode & S_IF<type>) != S_IF<type>)
>
> is not a good thing. Yes, it happens to work with S_IFREG, but...
>
>
>
>>
>> There are 3 other places in classLoader.cpp using the same check as 
>> in my webrev.
>> Should I change those too?
>
> Yes. Please fix those other uses.
>
>
>>> 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'
>> Ok. I will update all the above typos and error messages.
>>>
>>>     L76:             .shouldContain("Cannot have directory in 
>>> --patch-module during dumping");
>>>         Update to the new error message if you choose to change it 
>>> above.
>> Yes, I can update that too.
>>
>> I will post an updated webrev after testing.
>>
>> Thanks for your review.
>
> No problem. Sorry it took me a while to get to it.
>
> Dan
>
>
>>
>> Calvin
>>>
>>> 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
>>>>>>
>>>
>


More information about the hotspot-runtime-dev mailing list