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