Hi Alexey, I’ve searched for |GetProcAddress| usages across source code and couldn’t find (hopefully tbh) other occurrences of such mismatches.
Regards, Ali > On 7 Dec 2018, at 20:24, Alexey Ivanov <alexey.iva...@oracle.com> wrote: > > Hi Ali, > > On 06/12/2018 22:49, Ali İnce wrote: >> Hi Magnus, Alexey, >> >> I believe we won’t be able to get further opinions from serviceability-dev. > > Unfortunately, no one has replied as of now. > Have you found any issues with jdwpTransport_OnLoad after removing JNICALL? > >> Andrew Luo suggested using a similar mechanism as is used for jvm.dll by >> using symbol name files mapped by platform (files are under >> make/hotspot/symbols and interestingly windows is almost the only platform >> for which a symbol file doesn’t exist). > > Andrew says the .def files are auto-generated for Windows. There's a set of > shared functions. > JVM cannot change the calling convention, jvm.dll is the public interface to > JVM. > >> Another issue, again opened against AdoptOpenJDK 32-bit builds is somehow >> related with the same problem - but this time it is jimage.dll depending on >> zip.dll expecting the ZIP_InflateFully function to be decorated with JNICALL >> - whereas it was removed earlier from the implementation and the runtime >> image just fails with access violation just because jimage source code still >> declared it with JNICALL >> (https://github.com/AdoptOpenJDK/openjdk-build/issues/763 >> <https://github.com/AdoptOpenJDK/openjdk-build/issues/763>). > > The usage of ZIP_InflateFully from zip.dll by jimage.dll was overlooked > during work on https://bugs.openjdk.java.net/browse/JDK-8201226 > <https://bugs.openjdk.java.net/browse/JDK-8201226>. > > I can take care of it. > (I could not build 32 bit Windows. It seem to have overcome the problem by > adding --disable-warnings-as-errors option to configure.) > > However, the report says the resulting image crashes in 64 bit windows too > which shouldn't be affected by JNICALL mismatch. > >> I’ve also searched for GetProcAddress calls across the source code - and I >> think it’s important to work on the consistency of JNICALL usages across the >> whole source code. > > There are many places where libraries are loaded dynamically or a function > may be unavailable on older versions of the platform. > Have you identified any other place where functions from JDK DLLs are looked > up by names? > >> Another noteworthy thing I’ve noticed is there are still JNICALL modifiers >> for example in >> https://github.com/AdoptOpenJDK/openjdk-jdk11u/blob/master/src/jdk.crypto.cryptoki/windows/native/libj2pkcs11/j2secmod_md.c#L49 >> >> <https://github.com/AdoptOpenJDK/openjdk-jdk11u/blob/master/src/jdk.crypto.cryptoki/windows/native/libj2pkcs11/j2secmod_md.c#L49> >> - which I guess will also be reported as broken since these functions are >> again name decorated. > > This is a JNI method. It should use JNICALL modifier. If it wasn't found, the > class sun.security.pkcs11.Secmod would fail to load. I guess JVM handles name > mangling somehow for native method implementation. > > Such functions were never explicitly exported using mapfiles or /export > options on Windows, at least in the client area. For Linux and Solaris, > adding or removing a native method required editing mapfiles. > >> If we’re still determined to remove JNICALL, what about just removing >> __stdcall from JNICALL declaration at >> https://github.com/AdoptOpenJDK/openjdk-jdk11u/blob/master/src/java.base/windows/native/include/jni_md.h#L31 >> >> <https://github.com/AdoptOpenJDK/openjdk-jdk11u/blob/master/src/java.base/windows/native/include/jni_md.h#L31> >> ? Wouldn’t that be a more consistent move? > > We can't do that. > Implementation of Java native methods must use __stdcall calling convention. > >> >> Regards, >> >> Ali >> >>> On 22 Nov 2018, at 17:29, Magnus Ihse Bursie <magnus.ihse.bur...@oracle.com >>> <mailto:magnus.ihse.bur...@oracle.com>> wrote: >>> >>> I think we are in complete agreement. What's missing is some expert opinion >>> from serviceability-dev if there is any problem with changing the >>> signature. I'd preferably have that. >>> >>> If no one knows, I'd say, change it, and see if we still pass the relevant >>> tests, and if so, ship it. >>> >>> /Magnus >>> >>> 22 nov. 2018 kl. 18:04 skrev Alexey Ivanov <alexey.iva...@oracle.com >>> <mailto:alexey.iva...@oracle.com>>: >>> >>>> >>>> On 21/11/2018 12:16, Magnus Ihse Bursie wrote: >>>>> >>>>> On 2018-11-20 16:49, Alexey Ivanov wrote: >>>>> >>>>>> Hi Ali, Magnus, >>>>>> >>>>>> I absolutely agree this change has to be reviewed by someone from >>>>>> serviceability. >>>>>> >>>>>> There are several options: >>>>>> >>>>>> 1. Add -export:jdwpTransport_OnLoad to LDFLAGS for Windows >>>>>> http://mail.openjdk.java.net/pipermail/build-dev/2018-November/023935.html >>>>>> >>>>>> <http://mail.openjdk.java.net/pipermail/build-dev/2018-November/023935.html> >>>>>> so it partially reverts the changes from >>>>>> https://bugs.openjdk.java.net/browse/JDK-8200178 >>>>>> <https://bugs.openjdk.java.net/browse/JDK-8200178> >>>>>> >>>>>> 2. Remove JNICALL (__stdcall) modifier, eventually changing the calling >>>>>> convention to __cdecl. >>>>>> http://mail.openjdk.java.net/pipermail/build-dev/2018-November/023969.html >>>>>> >>>>>> <http://mail.openjdk.java.net/pipermail/build-dev/2018-November/023969.html> >>>>>> >>>>>> 3. Use inline /export option via #pragma: >>>>>> #pragma comment(linker, "/export:jdwpTransport_OnLoad= >>>>>> _jdwpTransport_OnLoad@16") >>>>>> as referenced in >>>>>> https://docs.microsoft.com/en-ie/cpp/cpp/dllexport-dllimport?view=vs-2017 >>>>>> >>>>>> <https://docs.microsoft.com/en-ie/cpp/cpp/dllexport-dllimport?view=vs-2017> >>>>>> https://docs.microsoft.com/en-ie/cpp/build/reference/exports?view=vs-2017 >>>>>> >>>>>> <https://docs.microsoft.com/en-ie/cpp/build/reference/exports?view=vs-2017> >>>>>> >>>>>> The third options still needs to be tested to make sure it does not >>>>>> break other platforms builds. >>>>> I'm not fond of either solution 1 or 3. I really think the signature >>>>> should be made correctly at the point of definition in the source code. >>>> >>>> That is why I proposed removing JNICALL (__stdcall) from the function >>>> declaration as we had done for libzip, libjimage [1] and mlib_image [2]. >>>> >>>> Microsoft recommends using __stdcall for DLLs, at the same time it >>>> decorates the function name making it harder to export it by its plain >>>> name. >>>> >>>> >>>> By removing JNICALL / __stdcall, we make the function use __cdecl calling >>>> convention. The difference between them is that with __stdcall the callee >>>> cleans the stack whereas with __cdecl the caller cleans the stack. >>>> >>>> It shouldn't be a problem as long as all function declarations use the >>>> same calling convention, which, I believe, is the case here. >>>> >>>>> We've spent some considerable effort of getting rid of the /export flags >>>>> for Windows (and map files for unix), and I don't want to see them creep >>>>> back in. Note that option 3 is just option 1 in disguise. The only single >>>>> good thing about it is that it allows you to put the compiler option next >>>>> to the signature declaration in the source code. >>>> >>>> At least in this case, the option will be near the function body >>>> definition. It will be easier to update it if the signature changes. >>>> >>>> If we are to use __stdcall, it seems to be the only way to export the >>>> undecorated name. >>>> >>>>> The same goes for def files, as suggested by Ali. It's just yet another >>>>> version of option 1 in disguise/map files. >>>> >>>> If option 2 is rejected, I'm for using option 3. If option 3 cannot be >>>> used too, I'm for option 1. >>>> I think we should not fall back to def files in any case. And Makefile >>>> will have to include the reference to the def file, so it's even worse >>>> than option 1. >>>> >>>> >>>> Regards, >>>> Alexey >>>> >>>> [1] https://bugs.openjdk.java.net/browse/JDK-8201226 >>>> <https://bugs.openjdk.java.net/browse/JDK-8201226> >>>> [2] https://bugs.openjdk.java.net/browse/JDK-8202476 >>>> <https://bugs.openjdk.java.net/browse/JDK-8202476> >>>>> >>>>> /Magnus >>>>> >>>>>> >>>>>> >>>>>> Regards, >>>>>> Alexey >>>>>> >>>>>> On 19/11/2018 12:19, Magnus Ihse Bursie wrote: >>>>>>> Hi Ali, >>>>>>> >>>>>>> From a build perspective this change looks OK. I'm not aware of the >>>>>>> finer details on the OnLoad mechanism, like what calling convention is >>>>>>> to be used. So maybe this is a no-go from that view. >>>>>>> >>>>>>> I'm cc:ing servicability so they can have a look at it. >>>>>>> >>>>>>> /Magnus >>>>>>> >>>>>>> On 2018-11-18 13:07, Ali İnce wrote: >>>>>>>> Hi Alexey, >>>>>>>> >>>>>>>> Below is a new patch content that removes JNICALL modifiers from the >>>>>>>> exported functions of interest. This results in exported functions not >>>>>>>> to be name decorated and use __cdecl calling convention. >>>>>>>> >>>>>>>> Do you have any more suggestions, or would you please guide me on next >>>>>>>> steps? >>>>>>>> >>>>>>>> Thanks, >>>>>>>> >>>>>>>> Ali >>>>>>>> >>>>>>>> # HG changeset patch >>>>>>>> # User Ali Ince <ali.i...@gmail.com> <mailto:ali.i...@gmail.com> >>>>>>>> # Date 1542542415 0 >>>>>>>> # Sun Nov 18 12:00:15 2018 +0000 >>>>>>>> # Node ID a41df44e13e1b761f4b3f05a17ed18953067ae39 >>>>>>>> # Parent 16d5ec7dbbb49ef3f969e34be870e3f917ea89ff >>>>>>>> Remove JNICALL modifiers to prevent name decoration on 32-bit windows >>>>>>>> builds >>>>>>>> >>>>>>>> diff -r 16d5ec7dbbb4 -r a41df44e13e1 >>>>>>>> src/jdk.jdi/share/native/libdt_shmem/shmemBack.c >>>>>>>> --- a/src/jdk.jdi/share/native/libdt_shmem/shmemBack.c Tue Aug 14 >>>>>>>> 14:28:23 2018 +0200 >>>>>>>> +++ b/src/jdk.jdi/share/native/libdt_shmem/shmemBack.c Sun Nov 18 >>>>>>>> 12:00:15 2018 +0000 >>>>>>>> @@ -339,7 +339,7 @@ >>>>>>>> return JDWPTRANSPORT_ERROR_NONE; >>>>>>>> } >>>>>>>> >>>>>>>> -JNIEXPORT jint JNICALL >>>>>>>> +JNIEXPORT jint >>>>>>>> jdwpTransport_OnLoad(JavaVM *vm, jdwpTransportCallback* cbTablePtr, >>>>>>>> jint version, jdwpTransportEnv** result) >>>>>>>> { >>>>>>>> diff -r 16d5ec7dbbb4 -r a41df44e13e1 >>>>>>>> src/jdk.jdwp.agent/share/native/include/jdwpTransport.h >>>>>>>> --- a/src/jdk.jdwp.agent/share/native/include/jdwpTransport.h Tue Aug >>>>>>>> 14 14:28:23 2018 +0200 >>>>>>>> +++ b/src/jdk.jdwp.agent/share/native/include/jdwpTransport.h Sun Nov >>>>>>>> 18 12:00:15 2018 +0000 >>>>>>>> @@ -140,7 +140,7 @@ >>>>>>>> void (*free)(void *buffer); /* Call this for all >>>>>>>> deallocations */ >>>>>>>> } jdwpTransportCallback; >>>>>>>> >>>>>>>> -typedef jint (JNICALL *jdwpTransport_OnLoad_t)(JavaVM *jvm, >>>>>>>> +typedef jint (*jdwpTransport_OnLoad_t)(JavaVM *jvm, >>>>>>>> jdwpTransportCallback >>>>>>>> *callback, >>>>>>>> jint version, >>>>>>>> jdwpTransportEnv** >>>>>>>> env); >>>>>>>> diff -r 16d5ec7dbbb4 -r a41df44e13e1 >>>>>>>> src/jdk.jdwp.agent/share/native/libdt_socket/socketTransport.c >>>>>>>> --- a/src/jdk.jdwp.agent/share/native/libdt_socket/socketTransport.c >>>>>>>> Tue Aug 14 14:28:23 2018 +0200 >>>>>>>> +++ b/src/jdk.jdwp.agent/share/native/libdt_socket/socketTransport.c >>>>>>>> Sun Nov 18 12:00:15 2018 +0000 >>>>>>>> @@ -1019,7 +1019,7 @@ >>>>>>>> return JDWPTRANSPORT_ERROR_NONE; >>>>>>>> } >>>>>>>> >>>>>>>> -JNIEXPORT jint JNICALL >>>>>>>> +JNIEXPORT jint >>>>>>>> jdwpTransport_OnLoad(JavaVM *vm, jdwpTransportCallback* cbTablePtr, >>>>>>>> jint version, jdwpTransportEnv** env) >>>>>>>> { >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>>> On 16 Nov 2018, at 23:03, Alexey Ivanov <alexey.iva...@oracle.com> >>>>>>>>> <mailto:alexey.iva...@oracle.com> wrote: >>>>>>>>> >>>>>>>>> Hi Ali, >>>>>>>>> >>>>>>>>> It's exactly what I referred to. >>>>>>>>> >>>>>>>>> Yes, it does change the calling convention. >>>>>>>>> Yet it's not a (big) problem because both parties will use the same >>>>>>>>> calling convention. >>>>>>>>> >>>>>>>>> Using a DLL from another build will not be possible. But it's not a >>>>>>>>> good idea to do so anyway. >>>>>>>>> >>>>>>>>> >>>>>>>>> Mapfiles and export options have been removed by >>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8200178 >>>>>>>>> <https://bugs.openjdk.java.net/browse/JDK-8200178> to simplify >>>>>>>>> managing exported functions. So that to add or remove an exported >>>>>>>>> function, you don't have modify makefiles and/or mapfiles. You just >>>>>>>>> edit the source files. >>>>>>>>> >>>>>>>>> Regards, >>>>>>>>> Alexey >>>>>>>>> >>>>>>>>> On 16/11/2018 22:42, Ali İnce wrote: >>>>>>>>>> Hi Alexey, >>>>>>>>>> >>>>>>>>>> Thanks for your reply. >>>>>>>>>> >>>>>>>>>> I will definitely give it a try though I’m not sure if that’ll be a >>>>>>>>>> breaking change. This is because JNICALL modifier is defined to be >>>>>>>>>> __stdcall on windows which is both specified on jdwpTransport_OnLoad >>>>>>>>>> exported functions both for dt_socket.dll and dt_shmem.dll. >>>>>>>>>> >>>>>>>>>> The __stdcall is ignored on x64 platforms and also name decoration >>>>>>>>>> is not applied >>>>>>>>>> (https://docs.microsoft.com/en-gb/cpp/build/reference/decorated-names?view=vs-2017 >>>>>>>>>> >>>>>>>>>> <https://docs.microsoft.com/en-gb/cpp/build/reference/decorated-names?view=vs-2017>). >>>>>>>>>> I believe that’s why we don’t experience any problems on x64 builds. >>>>>>>>>> >>>>>>>>>> Removing JNICALL (thus __stdcall) modifier (apparently only applies >>>>>>>>>> to x86 builds) will result in the calling convention to be changed, >>>>>>>>>> and thus will change how parameters are ordered and also the stack >>>>>>>>>> cleanup rules. I think this may result in problems in programs that >>>>>>>>>> are designed to load shared libraries and its exported functions at >>>>>>>>>> runtime (not bound by link time which probably won’t cause any >>>>>>>>>> issues - assuming that they use the shared function signature) - as >>>>>>>>>> in >>>>>>>>>> https://github.com/AdoptOpenJDK/openjdk-jdk11u/blob/5f01925b80ed851b133ee26fbcb07026ac04149e/src/jdk.jdwp.agent/share/native/libjdwp/transport.c#L99 >>>>>>>>>> >>>>>>>>>> <https://github.com/AdoptOpenJDK/openjdk-jdk11u/blob/5f01925b80ed851b133ee26fbcb07026ac04149e/src/jdk.jdwp.agent/share/native/libjdwp/transport.c#L99>. >>>>>>>>>> >>>>>>>>>> Any thoughts? >>>>>>>>>> >>>>>>>>>> Regards, >>>>>>>>>> >>>>>>>>>> Ali >>>>>>>>>> >>>>>>>>>>> On 15 Nov 2018, at 22:14, Alexey Ivanov <alexey.iva...@oracle.com> >>>>>>>>>>> <mailto:alexey.iva...@oracle.com> wrote: >>>>>>>>>>> >>>>>>>>>>> Hi Ali, >>>>>>>>>>> >>>>>>>>>>> Can the issue be resolved by using different call modifiers on the >>>>>>>>>>> affected functions? >>>>>>>>>>> >>>>>>>>>>> Some build / link issues were resolved by adding / removing JNICALL >>>>>>>>>>> modifier, see >>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8201226 >>>>>>>>>>> <https://bugs.openjdk.java.net/browse/JDK-8201226> >>>>>>>>>>> http://mail.openjdk.java.net/pipermail/build-dev/2018-April/021553.html >>>>>>>>>>> >>>>>>>>>>> <http://mail.openjdk.java.net/pipermail/build-dev/2018-April/021553.html> >>>>>>>>>>> >>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8202476 >>>>>>>>>>> <https://bugs.openjdk.java.net/browse/JDK-8202476> >>>>>>>>>>> http://mail.openjdk.java.net/pipermail/build-dev/2018-May/021913.html >>>>>>>>>>> >>>>>>>>>>> <http://mail.openjdk.java.net/pipermail/build-dev/2018-May/021913.html> >>>>>>>>>>> >>>>>>>>>>> Regards, >>>>>>>>>>> Alexey >>>>>>>>>>> >>>>>>>>>>> On 15/11/2018 21:43, Ali İnce wrote: >>>>>>>>>>>> Hi, >>>>>>>>>>>> >>>>>>>>>>>> An issue (https://github.com/AdoptOpenJDK/openjdk-build/issues/709 >>>>>>>>>>>> <https://github.com/AdoptOpenJDK/openjdk-build/issues/709>) raised >>>>>>>>>>>> against AdoptOpenJDK jdk11 32-bit windows builds led us to the >>>>>>>>>>>> name decoration problem on built 32-bit dlls - specifically >>>>>>>>>>>> dt_socket.dll and dt_shmem.dll. Whereas 64-bit MSVC builds don’t >>>>>>>>>>>> apply any name decorations on exported functions, 32-bit builds >>>>>>>>>>>> apply them - which requires either def files or /export flags to >>>>>>>>>>>> be specified on the linker arguments. >>>>>>>>>>>> >>>>>>>>>>>> Although the expected linker arguments were present on previous >>>>>>>>>>>> jdk makefiles, they were removed in jdk11 makefiles. >>>>>>>>>>>> >>>>>>>>>>>> Below is the proposed patch, which can also be browsed at >>>>>>>>>>>> https://github.com/AdoptOpenJDK/openjdk-jdk11u/pull/1 >>>>>>>>>>>> <https://github.com/AdoptOpenJDK/openjdk-jdk11u/pull/1>. >>>>>>>>>>>> >>>>>>>>>>>> Would you please have a look and let me know of any points I may >>>>>>>>>>>> have missed (I don’t have any background information about why the >>>>>>>>>>>> export flags were removed in jdk11)? >>>>>>>>>>>> >>>>>>>>>>>> Regards, >>>>>>>>>>>> >>>>>>>>>>>> Ali >>>>>>>>>>>> >>>>>>>>>>>> diff --git a/make/lib/Lib-jdk.jdi.gmk b/make/lib/Lib-jdk.jdi.gmk >>>>>>>>>>>> index 197b95c2e2..ac6ebf5591 100644 >>>>>>>>>>>> --- a/make/lib/Lib-jdk.jdi.gmk >>>>>>>>>>>> +++ b/make/lib/Lib-jdk.jdi.gmk >>>>>>>>>>>> @@ -37,6 +37,7 @@ ifeq ($(OPENJDK_TARGET_OS), windows) >>>>>>>>>>>> jdk.jdwp.agent:include \ >>>>>>>>>>>> jdk.jdwp.agent:libjdwp/export, \ >>>>>>>>>>>> LDFLAGS := $(LDFLAGS_JDKLIB), \ >>>>>>>>>>>> + LDFLAGS_windows := -export:jdwpTransport_OnLoad, \ >>>>>>>>>>>> LIBS := $(JDKLIB_LIBS), \ >>>>>>>>>>>> )) >>>>>>>>>>>> >>>>>>>>>>>> diff --git a/make/lib/Lib-jdk.jdwp.agent.gmk >>>>>>>>>>>> b/make/lib/Lib-jdk.jdwp.agent.gmk >>>>>>>>>>>> index 0bc93e0d35..0382e55325 100644 >>>>>>>>>>>> --- a/make/lib/Lib-jdk.jdwp.agent.gmk >>>>>>>>>>>> +++ b/make/lib/Lib-jdk.jdwp.agent.gmk >>>>>>>>>>>> @@ -37,6 +37,7 @@ $(eval $(call SetupJdkLibrary, >>>>>>>>>>>> BUILD_LIBDT_SOCKET, \ >>>>>>>>>>>> libjdwp/export, \ >>>>>>>>>>>> LDFLAGS := $(LDFLAGS_JDKLIB) \ >>>>>>>>>>>> $(call SET_SHARED_LIBRARY_ORIGIN), \ >>>>>>>>>>>> + LDFLAGS_windows := -export:jdwpTransport_OnLoad, \ >>>>>>>>>>>> LIBS_linux := -lpthread, \ >>>>>>>>>>>> LIBS_solaris := -lnsl -lsocket, \ >>>>>>>>>>>> LIBS_windows := $(JDKLIB_LIBS) ws2_32.lib, \ >>>>>>> >>>>>> >>>>> >>>> >> >
