[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-runtime-dev
Subject:    Re: RFR(S): 8232980: Cleanup initialization of function pointers into java.base from classloader.cpp
From:       Ioi Lam <ioi.lam () oracle ! com>
Date:       2019-10-30 19:32:41
Message-ID: c6dcde96-e3e1-93fe-f190-4117838b4a2b () oracle ! com
[Download RAW message or body]

Looks good to me to. Thanks for fixing this!

- Ioi

On 10/30/19 8:54 AM, Calvin Cheung wrote:
> Hi Christoph,
>
> The updated webrev looks good.
>
> thanks,
>
> Calvin
>
> On 10/30/19 12:48 AM, Langer, Christoph wrote:
>> Hi Ioi,
>>
>> you're right, we should prefer methods over macros - that's way nicer 
>> 😊 So, I changed the macro into a method and I also removed the 
>> comments which don't do more than stating the obvious.
>>
>> Please check: http://cr.openjdk.java.net/~clanger/webrevs/8232980.2/
>>
>> Thanks
>> Christoph
>>
>>> -----Original Message-----
>>> From: Ioi Lam <ioi.lam@oracle.com>
>>> Sent: Dienstag, 29. Oktober 2019 16:35
>>> To: Langer, Christoph <christoph.langer@sap.com>; hotspot-runtime-
>>> dev@openjdk.java.net; Calvin Cheung <calvin.cheung@oracle.com>
>>> Subject: Re: RFR(S): 8232980: Cleanup initialization of function 
>>> pointers into
>>> java.base from classloader.cpp
>>>
>>> Hi Christoph,
>>>
>>> I think in general we should avoid macros to improve debuggability. It
>>> may be better to use a helper function. Also, this way you don't 
>>> need to
>>> come up with a different error message for each function.
>>>
>>> void* ClassLoader::dll_lookup(void *lib, const char *name) {
>>>       void func = os::dll_lookup(lib, "Canonicalize"));
>>>       if (func == NULL) {
>>>           vm_exit_during_initialization("function %s not found", name);
>>>       }
>>>       return func;
>>> }
>>>
>>> CanonicalizeEntry = CAST_TO_FN_PTR(canonicalize_fn_t,
>>> dll_lookup(javalib_handle, "Canonicalize"));
>>>
>>>
>>>
>>> Also, I think this comment is not necessary as it's clear what the code
>>> is trying to do:
>>>
>>> // Lookup canonicalize entry in libjava.dll
>>>
>>> Thanks
>>> - Ioi
>>>
>>>
>>> On 10/29/19 3:08 AM, Langer, Christoph wrote:
>>>> Hi Ioi, Calvin,
>>>>
>>>> thanks for looking at my RFR. I've addressed your points in an updated
>>> webrev: http://cr.openjdk.java.net/~clanger/webrevs/8232980.1/
>>>> - I removed ClassLoader::decompress and resolving of ZipInflateFully.
>>>> - For the checking of the resolved symbols, I defined macro
>>> CHECK_RESOLVED_OR_EXIT. It'll check for NULL and in that case, exit via
>>> vm_exit_during_initialization.
>>>> - in the destructor ClassPathZipEntry::~ClassPathZipEntry, ZipClose 
>>>> is called
>>> unconditionally (without checking for ZipClose not being null).
>>>> - ClassLoader::crc32: assert removed, since failing to resolve the 
>>>> Crc32
>>> pointer will cause vm_exit_during_initialization
>>>> Please check the new version 😊
>>>>
>>>> Thanks
>>>> Christoph
>>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: hotspot-runtime-dev <hotspot-runtime-dev-
>>>>> bounces@openjdk.java.net> On Behalf Of Ioi Lam
>>>>> Sent: Donnerstag, 24. Oktober 2019 21:00
>>>>> To: hotspot-runtime-dev@openjdk.java.net
>>>>> Subject: Re: RFR(S): 8232980: Cleanup initialization of function 
>>>>> pointers
>>> into
>>>>> java.base from classloader.cpp
>>>>>
>>>>> Hi Chtistoph,
>>>>>
>>>>> Thanks for fixing this.
>>>>>
>>>>> I think is OK to leave the 3 functions separate, as each of them are
>>>>> non-trivial.
>>>>>
>>>>> In fact, we can probably delay calling of
>>>>> ClassLoader::load_zip_library() in ClassPathZipEntry::open_entry, so
>>>>> apps that don't use JAR files can start up a little faster. I'll 
>>>>> file an
>>>>> REF for this: JDK-8232989.
>>>>>
>>>>> For repeated calls like this:
>>>>>
>>>>>
>>>>> 992 ZipOpen = CAST_TO_FN_PTR(ZipOpen_t, os::dll_lookup(handle,
>>>>> "ZIP_Open"));
>>>>> 993 if (ZipOpen == NULL) {
>>>>> 994 vm_exit_during_initialization("Corrupted ZIP library: ZIP_Open
>>>>> missing", path);
>>>>> 995 }
>>>>> 996 ZipClose = CAST_TO_FN_PTR(ZipClose_t, os::dll_lookup(handle,
>>>>> "ZIP_Close"));
>>>>> 997 if (ZipClose == NULL) {
>>>>> 998 vm_exit_during_initialization("Corrupted ZIP library: ZIP_Close
>>>>> missing", path);
>>>>> 999 }
>>>>>
>>>>>
>>>>> I think it's better to use a utility function to do the check and 
>>>>> exiting.
>>>>>
>>>>> Thanks
>>>>> - Ioi
>>>>>
>>>>> On 10/24/19 8:41 AM, Langer, Christoph wrote:
>>>>>> Hi,
>>>>>>
>>>>>> please help reviewing a cleanup patch to classLoader.cpp.
>>>>>>
>>>>>> The methods load_zip_library() and load_jimage_library() can be
>>> cleaned
>>>>> up a little bit. In my patch, I also extracted the initialization 
>>>>> of the one
>>> symbol
>>>>> coming from libjava to a new method load_java_library(). However, I'm
>>> not
>>>>> fully sure on whether it would be nicer to have all these 3 methods
>>>>> consolidated into one. What do you think?
>>>>>> In my patch I check for all needed symbols since it's all coming 
>>>>>> from the
>>> JDK
>>>>> and we can assume consistency. Should there be a problem in resolving
>>>>> some symbol, then VM initialization should fail.
>>>>>> Furthermore, I'm wondering, whether to use guarantee or
>>>>> vm_exit_during_initialization for the NULL checks of the resolved
>>> symbols.
>>>>> Currently we have both but I think we should use one consistent
>>> approach. I
>>>>> think vm_exit_during_initialization would be the best fit. Opinions?
>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8232980
>>>>>> Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8232980.0/
>>>>>>
>>>>>> Thanks
>>>>>> Christoph
>>>>>>

[prev in list] [next in list] [prev in thread] [next in thread]