Hi Phil/Alexey, thanks for adding the other lists . > Is this the current version of the change : > http://cr.openjdk.java.net/~mbaesken/webrevs/8201226.2/ ?
Yes. Best regards, Matthias > -----Original Message----- > From: Alexey Ivanov [mailto:alexey.iva...@oracle.com] > Sent: Donnerstag, 12. April 2018 23:53 > To: Phil Race <philip.r...@oracle.com>; Baesken, Matthias > <matthias.baes...@sap.com>; Alan Bateman <alan.bate...@oracle.com>; > Magnus Ihse Bursie <magnus.ihse.bur...@oracle.com> > Cc: build-dev@openjdk.java.net; core-libs-...@openjdk.java.net; Doerr, > Martin <martin.do...@sap.com>; 2d-dev <2d-...@openjdk.java.net>; > hotspot-dev <hotspot-...@openjdk.java.net> > Subject: Re: 8201226 missing JNIEXPORT / JNICALL at some places in function > declarations/implementations - was : RE: missing JNIEXPORT / JNICALL at > some places in function declarations/implementations > > > On 12/04/2018 21:42, Phil Race wrote: > > How can JNIEXPORT be different between 32 bit & 64 bit ? > > I'm sure you saw compilation errors but I don't get why it failed for > > 32 only. > > > > JNICALL (_stdcall) may be unnecessary on 64 bit Windows but that doesn't > > explain why the 32 bit compiler would complain about inconsistent > > application > > of __declspec(dllexport) - ie JNIEXPORT. > > > > Or is that part (adding JNIEXPORT) pure clean up and the compilation > > errors were all down to JNICALL ? > > Adding missing JNIEXPORT is for cleanup only. > > The compiler complained about mismatched JNICALL / non-JNICALL > declarations as the macro changes calling convention from the default > __cdecl to __stdcall on 32 bit Windows. > > Another issue is that __stdcall decorates the functions: prefixes with > underscore and postfixes with @ + size of parameters. Because of the > decorations, classLoader.cpp can't lookup the required functions by name > from zip.dll and jimage.dll. The functions are exported but with > different name. > > I hope this information adds more details to the picture. > > > I was a bit puzzled at the removals I saw here : > > > > > http://cr.openjdk.java.net/~mbaesken/webrevs/8201226.2/src/java.deskto > p/share/native/libsplashscreen/splashscreen_impl.h.udiff.html > > > > > > .. I needed to look at the whole file to realise that you were > > removing a duplicate > > declaration. > > That was tricky. I could have been mentioned in the review. > > > Regards, > Alexey > > > > > -phil. > > > > On 04/12/2018 04:04 AM, Baesken, Matthias wrote: > >> Hi Alan , this is the up to date webrev . > >> However we want to add Alexey Ivanov as additional author . > >> > >>> As I read it, this changes the calling convention of these functions on > >>> 32-bit Windows but it will have no impact on 64-bit Windows (as > >>> __stdcall is ignored) or other platforms, is that correct? > >>> > >> The change adds JNIEXPORT at some places where it is ben > >> forgotten , for example : > >> > >> > http://cr.openjdk.java.net/~mbaesken/webrevs/8201226.2/src/java.deskto > p/share/native/libmlib_image/mlib_c_ImageLookUp.c.udiff.html > >> > >> > >> This might have potential impact on other platforms (fixes the > >> mismatches) . > >> > >> Best regards, Matthias > >> > >> > >> > >> > >>> -----Original Message----- > >>> From: Alan Bateman [mailto:alan.bate...@oracle.com] > >>> Sent: Donnerstag, 12. April 2018 12:54 > >>> To: Baesken, Matthias <matthias.baes...@sap.com>; Magnus Ihse > Bursie > >>> <magnus.ihse.bur...@oracle.com> > >>> Cc: build-dev@openjdk.java.net; Doerr, Martin > <martin.do...@sap.com>; > >>> core-libs-...@openjdk.java.net > >>> Subject: Re: 8201226 missing JNIEXPORT / JNICALL at some places in > >>> function > >>> declarations/implementations - was : RE: missing JNIEXPORT / JNICALL at > >>> some places in function declarations/implementations > >>> > >>> Adding core-libs-dev as this is change code in libjava, libzip, > >>> libjimage, ... > >>> > >>> Can you confirm that this is the up to date webrev: > >>> http://cr.openjdk.java.net/~mbaesken/webrevs/8201226.2/ > >>> > >>> As I read it, this changes the calling convention of these functions on > >>> 32-bit Windows but it will have no impact on 64-bit Windows (as > >>> __stdcall is ignored) or other platforms, is that correct? > >>> > >>> -Alan > >>> > >>> > >>> On 06/04/2018 14:20, Baesken, Matthias wrote: > >>>> Hello, I just noticed 2 additonal issues regarding > >>>> mapfile-removal : > >>>> > >>>> > >>>> 1. The follow up change that removed mapfiles for exes > >>>> as well > >>> leads on Win32 bit to this link error : > >>>> Creating library > >>> C:/JVM/jdk_jdk_ntintel/support/native/java.base/java/java.lib and > >>> object > >>> C:/JVM/jdk_jdk_ntintel/support/native/java.base/java/java.exp > >>>> LIBCMT.lib(crt0.obj) : error LNK2019: unresolved external symbol _main > >>> referenced in function ___tmainCRTStartup > >>>> C:/JVM/jdk_jdk_ntintel/support/native/java.base/java_objs/java.exe > : > >>> fatal error LNK1120: 1 unresolved externals > >>>> > >>>> Looks like we cannot have JNICALL in main.c : > >>>> > >>>> diff -r 4f6887eade94 src/java.base/share/native/launcher/main.c > >>>> --- a/src/java.base/share/native/launcher/main.c Thu Apr 05 > >>>> 14:39:04 > >>> 2018 -0700 > >>>> +++ b/src/java.base/share/native/launcher/main.c Fri Apr 06 > >>>> 15:16:40 > >>> 2018 +0200 > >>>> @@ -93,7 +93,7 @@ > >>>> __initenv = _environ; > >>>> > >>>> #else /* JAVAW */ > >>>> -JNIEXPORT int JNICALL > >>>> +JNIEXPORT int > >>>> main(int argc, char **argv) > >>>> { > >>>> int margc; > >>>> > >>>> > >>>> > >>>> 1. Zip-library has runtime issues when symbols are > >>>> resolved in > >>> src/hotspot/share/classfile/classLoader.cpp. > >>>> I added the declaration of the missing symbol, this seems to fix it . > >>>> > >>>> > >>>> diff -r 4f6887eade94 src/java.base/share/native/libzip/zip_util.h > >>>> --- a/src/java.base/share/native/libzip/zip_util.h Thu Apr 05 > >>>> 14:39:04 > >>> 2018 -0700 > >>>> +++ b/src/java.base/share/native/libzip/zip_util.h Fri Apr 06 > >>>> 15:16:40 > >>> 2018 +0200 > >>>> @@ -284,4 +284,7 @@ > >>>> JNIEXPORT jboolean JNICALL > >>>> ZIP_InflateFully(void *inBuf, jlong inLen, void *outBuf, jlong > >>>> outLen, char > >>> **pmsg); > >>>> +JNIEXPORT jint JNICALL > >>>> +ZIP_CRC32(jint crc, const jbyte *buf, jint len); > >>>> + > >>>> > >>>> > >>>> Should I prepare another bug, or add this to the existing one > >>>> and post a > >>> second webrev ? > >>>> Best regards, Matthias > >>>> > >>>> > >>>> From: Baesken, Matthias > >>>> Sent: Freitag, 6. April 2018 09:54 > >>>> To: 'Magnus Ihse Bursie' <magnus.ihse.bur...@oracle.com> > >>>> Cc: build-dev@openjdk.java.net; Doerr, Martin > <martin.do...@sap.com> > >>>> Subject: RFR: 8201226 missing JNIEXPORT / JNICALL at some places in > >>> function declarations/implementations - was : RE: missing JNIEXPORT / > >>> JNICALL at some places in function declarations/implementations > >>>> Hello, please review : > >>>> > >>>> Bug : > >>>> > >>>> https://bugs.openjdk.java.net/browse/JDK-8201226 > >>>> > >>>> change : > >>>> > >>>> http://cr.openjdk.java.net/~mbaesken/webrevs/8201226/ > >>>> > >>>> mostly I added JNIEXPORT / JNICALL at some places where the > win32bit > >>> build missed it . > >>>> A difference is > >>> src/java.desktop/share/native/libsplashscreen/splashscreen_impl.h > >>>> Where I removed a few declarations (one decl with one without > >>> JNIEXPORT / JNICALL – looked a bit strange ) . > >>>> Best regards , Matthias > >>>> > >>>> > >>>> From: Magnus Ihse Bursie [mailto:magnus.ihse.bur...@oracle.com] > >>>> Sent: Donnerstag, 5. April 2018 17:45 > >>>> To: Baesken, Matthias > >>> <matthias.baes...@sap.com<mailto:matthias.baes...@sap.com>> > >>>> Cc: build-dev@openjdk.java.net<mailto:build-dev@openjdk.java.net>; > >>> Doerr, Martin > <martin.do...@sap.com<mailto:martin.do...@sap.com>> > >>>> Subject: Re: missing JNIEXPORT / JNICALL at some places in function > >>> declarations/implementations > >>>> That's most likely a result of the new JNIEXPORT I added as part of > >>>> the > >>> mapfile removal. > >>>> I tried to match header file and C file, but I can certainly have > >>>> missed cases. > >>> If I didn't get any warnings, it was hard to know what I missed. > >>>> Please do submit your patch. > >>>> > >>>> I'm a bit surprised 32-bit Window is still buildable. :) > >>>> > >>>> /Magnus > >>>> > >>>> 5 apr. 2018 kl. 17:20 skrev Baesken, Matthias > >>> <matthias.baes...@sap.com<mailto:matthias.baes...@sap.com>>: > >>>> Hello, we noticed that at a number of places in the coding , the > >>> JNIEXPORT and/or JNICALL modifiers do not match when one > compares > >>> the declaration and > >>>> The implementation of functions. > >>>> While this is ok on most platforms, it seems to fail on Windows 32 > >>>> bit and > >>> leads to errors like this one : > >>>> > >>> > e:/priv/openjdk/repos/jdk/src/java.desktop/share/native/libmlib_image/ml > >>> > >>> ib_ImageConvKernelConvert.c(87) : error C2373: > >>> 'j2d_mlib_ImageConvKernelConvert' : redefinition; different type > >>> modifiers > >>> > e:\priv\openjdk\repos\jdk\src\java.desktop\share\native\libmlib_image\ml > >>> > >>> ib_image_proto.h(2630) : see declaration of > >>> 'j2d_mlib_ImageConvKernelConvert' > >>>> (there are quite a few of these e.g. in mlib / splashscreen etc.) > >>>> > >>>> > >>>> Another example is this one : > >>>> > >>>> diff -r 4d98473ed33e src/java.base/share/native/libjimage/jimage.hpp > >>>> --- a/src/java.base/share/native/libjimage/jimage.hpp Thu Apr 05 > >>>> 09:55:16 > >>> 2018 +0200 > >>>> +++ b/src/java.base/share/native/libjimage/jimage.hpp Thu Apr 05 > >>> 17:07:40 2018 +0200 > >>>> @@ -126,7 +126,7 @@ > >>>> * JImageLocationRef location = (*JImageFindResource)(image, > >>>> * "java.base", "9.0", > >>>> "java/lang/String.class", &size); > >>>> */ > >>>> -extern "C" JNIEXPORT JImageLocationRef > >>> JIMAGE_FindResource(JImageFile* jimage, > >>>> +extern "C" JNIEXPORT JImageLocationRef JNICALL > >>> JIMAGE_FindResource(JImageFile* jimage, > >>>> const char* module_name, const char* version, const > >>>> char* name, > >>>> jlong* size); > >>>> > >>>> > >>>> Is there some generic way to get the same declarations / > >>>> impementations > >>> in the code ? > >>>> Or should I just add a patch with my findings ? > >>>> > >>>> Best regards, Matthias > >>>> > >