RFR(M): 8164011: --patch-module support for CDS
Calvin Cheung
calvin.cheung at oracle.com
Fri Sep 16 17:10:22 UTC 2016
Hi,
One more update to the webrev:
- remove the following else block in classLoader.cpp since the checking
is being done in ClassLoader::setup_patch_mod_path()
1456 } else {
1457 if (DumpSharedSpaces) {
1458 vm_exit_during_initialization("Only jar file in
--patch-module is supported for CDS dumping ", e->name());
1459 }
1460 }
- update the comment at line 660 in the same file.
Only change is in classLoader.cpp:
1,2c1,2
< --- old/src/share/vm/classfile/classLoader.cpp 2016-09-16
09:33:50.598085454 -0700
< +++ new/src/share/vm/classfile/classLoader.cpp 2016-09-16
09:33:50.459075684 -0700
---
> --- old/src/share/vm/classfile/classLoader.cpp 2016-09-15
09:43:56.146724124 -0700
> +++ new/src/share/vm/classfile/classLoader.cpp 2016-09-15
09:43:55.986712875 -0700
53c53
< + if ((st.st_mode & S_IFMT) != S_IFREG) { // is not a regular file
---
> + if ((st.st_mode & S_IFMT) != S_IFREG) { // is directory
92c92
< @@ -1395,6 +1411,57 @@
---
> @@ -1395,6 +1411,61 @@
137a138,141
> + } else {
> + if (DumpSharedSpaces) {
> + vm_exit_during_initialization("Only jar file in
--patch-module is supported for CDS dumping ", e->name());
> + }
new webrev:
http://cr.openjdk.java.net/~ccheung/8164011/webrev.03/
thanks,
Calvin
On 9/15/16, 3:14 PM, Daniel D. Daugherty wrote:
> 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