Hi Alexey, Magnus, Thanks for all of your comments on the possible options.
Another option would be to use def files which contain an EXPORTS section that lists the exported functions ( https://docs.microsoft.com/en-gb/cpp/build/reference/exports?view=vs-2017). These files can lie next to the native source files and will require a /DEF:<file-name> option passed to LDFLAGS_windows. Very similar to Option 1 as Alexey suggested, but could be handy especially on other occurrences of exported functions with JNICALL modifier. I still believe Option 1 or the one I mentioned above will be a more conservative approach to fix this issue but I'm a newcomer so I'll be happy to take the option as suggested. Regards, Ali On Tue, Nov 20, 2018 at 3:49 PM Alexey Ivanov <alexey.iva...@oracle.com> 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 > so it partially reverts the changes from > 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 > > 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/build/reference/exports?view=vs-2017 > > The third options still needs to be tested to make sure it does not break > other platforms builds. > > > 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> <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> > <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 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). > 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. > > Any thoughts? > > Regards, > > Ali > > > On 15 Nov 2018, at 22:14, Alexey Ivanov <alexey.iva...@oracle.com> > <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, > seehttps://bugs.openjdk.java.net/browse/JDK-8201226http://mail.openjdk.java.net/pipermail/build-dev/2018-April/021553.html > https://bugs.openjdk.java.net/browse/JDK-8202476http://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) 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. > > 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, \ > > > >
