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

[Ioi Lam]

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
>>>