RFR(M): 8164011: --patch-module support for CDS
Calvin Cheung
calvin.cheung at oracle.com
Thu Sep 15 21:42:23 UTC 2016
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.
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