RFR(S): 8232980: Cleanup initialization of function pointers into java.base from classloader.cpp

[Calvin Cheung]

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 at oracle.com>
>> Sent: Dienstag, 29. Oktober 2019 16:35
>> To: Langer, Christoph <christoph.langer at sap.com>; hotspot-runtime-
>> dev at openjdk.java.net; Calvin Cheung <calvin.cheung at 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 at openjdk.java.net> On Behalf Of Ioi Lam
>>>> Sent: Donnerstag, 24. Oktober 2019 21:00
>>>> To: hotspot-runtime-dev at 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
>>>>>