Also, I'm not sure if I said it, but I think your original webrev looks good.

/Magnus

> 9 apr. 2018 kl. 13:38 skrev Baesken, Matthias <matthias.baes...@sap.com>:
> 
> I did  not add   JNICALL decorations  to  any  libzip functions , please see 
> my webrev :
> 
> http://cr.openjdk.java.net/~mbaesken/webrevs/8201226/
> 
>> , the problem here
>> is the added JNICALL decoration, which affects only win32 (and incorrectly,
>> that is).
> 
> so I wonder  which added  JNICALL  decoration   you are refering to .
> 
> Best regards, Matthias 
> 
> 
>> -----Original Message-----
>> From: Magnus Ihse Bursie [mailto:magnus.ihse.bur...@oracle.com]
>> Sent: Montag, 9. April 2018 13:29
>> To: Baesken, Matthias <matthias.baes...@sap.com>
>> Cc: Alexey Ivanov <alexey.iva...@oracle.com>; build-dev <build-
>> d...@openjdk.java.net>; Doerr, Martin <martin.do...@sap.com>
>> 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
>> 
>> Reinstating the -export command line options is not the way forward here.
>> As I understood it from private conversation with Alexey, the problem here
>> is the added JNICALL decoration, which affects only win32 (and incorrectly,
>> that is).
>> 
>> /Magnus
>> 
>> 9 apr. 2018 kl. 12:46 skrev Baesken, Matthias <matthias.baes...@sap.com>:
>> 
>>>> 
>>>> I think adding prototype of ZIP_CRC32 to zip_util.h is unnecessary as
>>>> this function is correctly decorated in CRC32.c and is exported.
>>>> 
>>> 
>>> Hi  Alexey,  you are correct  on this one .   The added  declaration does  
>>> not
>> help  with the  "Corrupted ZIP library"  error .
>>> This  one can be  fixed  by  bringing back  the  exports  in the makefile
>> make/lib/CoreLibraries.gmk        (same for some  JIMAGE functions) :
>>> 
>>> 
>>> --- a/make/lib/CoreLibraries.gmk       Sun Apr 08 17:01:20 2018 +0800
>>> +++ b/make/lib/CoreLibraries.gmk       Mon Apr 09 12:44:08 2018 +0200
>>> @@ -191,6 +191,9 @@
>>>    DISABLED_WARNINGS_gcc := implicit-fallthrough, \
>>>    LDFLAGS := $(LDFLAGS_JDKLIB) \
>>>        $(call SET_SHARED_LIBRARY_ORIGIN), \
>>> +    LDFLAGS_windows := -export:ZIP_Open -export:ZIP_Close -
>> export:ZIP_FindEntry \
>>> +        -export:ZIP_ReadEntry -export:ZIP_GetNextEntry \
>>> +        -export:ZIP_InflateFully -export:ZIP_CRC32 -export:ZIP_FreeEntry, \
>>>    LIBS_unix := -ljvm -ljava $(LIBZ_LIBS), \
>>>    LIBS_windows := jvm.lib $(WIN_JAVA_LIB), \
>>> ))
>>> @@ -221,6 +224,10 @@
>>>    CFLAGS_unix := -UDEBUG, \
>>>    LDFLAGS := $(LDFLAGS_JDKLIB) $(LDFLAGS_CXX_JDK) \
>>>        $(call SET_SHARED_LIBRARY_ORIGIN), \
>>> +    LDFLAGS_windows := -export:JIMAGE_Open -export:JIMAGE_Close \
>>> +        -export:JIMAGE_PackageToModule \
>>> +        -export:JIMAGE_FindResource -export:JIMAGE_GetResource \
>>> +        -export:JIMAGE_ResourceIterator -export:JIMAGE_ResourcePath, \
>>>    LIBS_unix := -ljvm -ldl $(LIBCXX), \
>>>    LIBS_macosx := -lc++, \
>>>    LIBS_windows := jvm.lib, \
>>> 
>>> 
>>> I wonder why the 64bit windows build can  live without the  exports  , any
>> ideas ?
>>> 
>>> 
>>> Best regards , Matthias
>>> 
>>> 
>>>> -----Original Message-----
>>>> From: Alexey Ivanov [mailto:alexey.iva...@oracle.com]
>>>> Sent: Samstag, 7. April 2018 00:14
>>>> To: Magnus Ihse Bursie <magnus.ihse.bur...@oracle.com>; Baesken,
>>>> Matthias <matthias.baes...@sap.com>
>>>> Cc: build-dev <build-dev@openjdk.java.net>; Doerr, Martin
>>>> <martin.do...@sap.com>
>>>> 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
>>>> 
>>>> Hi Magnus, Matthias,
>>>> 
>>>> I tried to build 32 bit Windows but it fails to run for me with
>>>> Corrupted ZIP library:
>>>> c:\work\jdk-dev\build\windows-x86-normal-server-
>> release\jdk\bin\zip.dll
>>>> 
>>>> The problem is that zip.dll exports all symbols as C++ rather than C.
>>>> For example, ZIP_CRC32 is exported as _ZIP_CRC32@12, and
>> classLoader.cpp
>>>> cannot find the function.
>>>> 
>>>> 
>>>> I think adding prototype of ZIP_CRC32 to zip_util.h is unnecessary as
>>>> this function is correctly decorated in CRC32.c and is exported.
>>>> 
>>>> Regards,
>>>> Alexey
>>>> 
>>>>> On 06/04/2018 17:33, Magnus Ihse Bursie wrote:
>>>>> I think it's reasonable to update the existing webrev.
>>>>> 
>>>>> /Magnus
>>>>> 
>>>>>> 6 apr. 2018 kl. 15:20 skrev Baesken, Matthias
>>>> <matthias.baes...@sap.com>:
>>>>>> 
>>>>>> Hello, I just noticed  2  additonal  issues  regarding mapfile-removal :
>>>>>> 
>>>>>> 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;
>>>>>> 
>>>>>> 
>>>>>> 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>
>>>>>> Cc: build-dev@openjdk.java.net; Doerr, Martin
>> <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>:
>>>>>> 
>>>>>> 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