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

[Langer, Christoph]

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