RFR(M): 8164011: --patch-module support for CDS
Daniel D. Daugherty
daniel.daugherty at oracle.com
Thu Sep 15 22:14:58 UTC 2016
On 9/15/16 3:42 PM, Calvin Cheung wrote:
>
>
> On 9/15/16, 1:39 PM, Daniel D. Daugherty wrote:
>> On 9/15/16 11:35 AM, Calvin Cheung wrote:
>>> Hi Dan,
>>>
>>> Here's an updated webev:
>>> http://cr.openjdk.java.net/~ccheung/8164011/webrev.02/
>>
>> src/share/vm/classfile/classLoader.cpp
>> 660 if ((st.st_mode & S_IFMT) != S_IFREG) { // is directory
>> The comment is wrong. It needs to be:
>>
>> if ((st.st_mode & S_IFMT) != S_IFREG) { // is not a regular file
> For this fix, I'd prefer to only further update the comment as above
> since I didn't need to modify the function initially until you noticed
> the potential issue of not using S_IFMT.
>
> Hope it's fine with you.
Sure. A took a quick look a few of the os::dir_is_empty() implementations
and it looks like they all return true if the os::opendir() call returns
NULL. os::opendir() returns NULL when it's asked to open something that
is not a directory.
So in a round about way, we're getting the results we want.
Dan
>
> thanks,
> Calvin
>>
>> Of course, that raises the question of what you really want
>> this code to do if given a 'path' that refers to something
>> that is not a directory.
>>
>> Perhaps what you want here is this:
>>
>> if ((st.st_mode & S_IFMT) == S_IFDIR) { // is a directory
>> <snip your existing if-block>
>> } else {
>> // some new error handling for a 'path' that exists, but is
>> // not a directory.
>> }
>>
>> src/share/vm/classfile/classLoader.hpp
>> No comments.
>>
>> src/share/vm/classfile/sharedPathsMiscInfo.cpp
>> No comments.
>>
>> src/share/vm/classfile/sharedPathsMiscInfo.hpp
>> No comments.
>>
>> src/share/vm/memory/filemap.cpp
>> No comments.
>>
>> src/share/vm/memory/filemap.hpp
>> No comments.
>>
>> src/share/vm/runtime/arguments.cpp
>> No comments.
>>
>> test/runtime/modules/PatchModule/PatchModuleCDS.java
>> No comments.
>>
>>
>> Dan
>>
>>
>>>
>>> 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