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-...@openjdk.java.net; core-libs-dev@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-...@openjdk.java.net; Doerr, Martin
> <martin.do...@sap.com>;
> >>> core-libs-dev@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-...@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-...@openjdk.java.net<mailto:build-...@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
> >>>>
> >

Reply via email to