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

[Langer, Christoph]

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