RFR(M): 8164011: --patch-module support for CDS
Daniel D. Daugherty
daniel.daugherty at oracle.com
Fri Sep 16 17:20:12 UTC 2016
Looks good to me.
Dan
On 9/16/16 11:10 AM, Calvin Cheung wrote:
> 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