RFR(M): 8164011: --patch-module support for CDS
Daniel D. Daugherty
daniel.daugherty at oracle.com
Thu Sep 15 02:07:07 UTC 2016
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