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

[Langer, Christoph]

Hi Ioi, Calvin,

thanks for reviewing. I've pushed now after running through submit.

Best regards
Christoph

> -----Original Message-----
> From: Calvin Cheung <calvin.cheung at oracle.com>
> Sent: Mittwoch, 30. Oktober 2019 16:55
> To: Langer, Christoph <christoph.langer at sap.com>; Ioi Lam
> <ioi.lam at oracle.com>; hotspot-runtime-dev at openjdk.java.net
> Subject: Re: RFR(S): 8232980: Cleanup initialization of function pointers into
> java.base from classloader.cpp
> 
> 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
> >>>>>