[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: [PATCH] JDK-8214122 Prevent name mangling of jdwpTransport_OnLoad in Windows 32-bit DLL name dec
From: Magnus Ihse Bursie <magnus.ihse.bursie () oracle ! com>
Date: 2018-11-22 17:29:08
Message-ID: A23FC146-55F6-4249-A045-D9D12C397093 () oracle ! com
[Download RAW message or body]
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.ivanov@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
> > > 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.
> > 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
> [2] 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.ince@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 \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > \
> > > > > --- 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.ivanov@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.ivanov@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
> > > > > > > > > http://mail.openjdk.java.net/pipermail/build-dev/2018-April/021553.html
> > > > > > > > >
> > > > > > > > > https://bugs.openjdk.java.net/browse/JDK-8202476
> > > > > > > > > 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) \
> > > > > > > > > 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, \
>
[Attachment #3 (text/html)]
<html><head><meta http-equiv="content-type" content="text/html; \
charset=utf-8"></head><body dir="auto"><div>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. </div><div \
id="AppleMailSignature"><br></div><div id="AppleMailSignature">If no one knows, I'd \
say, change it, and see if we still pass the relevant tests, and if so, ship \
it. <br><br>/Magnus</div><div><br>22 nov. 2018 kl. 18:04 skrev Alexey Ivanov \
<<a href="mailto:alexey.ivanov@oracle.com">alexey.ivanov@oracle.com</a>>:<br><br></div><blockquote \
type="cite"><div>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
<br>
<div class="moz-cite-prefix">On 21/11/2018 12:16, Magnus Ihse Bursie
wrote:<br>
</div>
<blockquote type="cite" \
cite="mid:17e7873f-f2f4-dc02-82a6-2d7eb1565ca2@oracle.com"> <meta \
content="text/html; charset=UTF-8" http-equiv="Content-Type"> <p></p>On 2018-11-20 \
16:49, Alexey Ivanov wrote:<br> <p></p>
<blockquote cite="mid:01b80c3f-8f71-26ec-304e-a7b245924bd5@oracle.com" \
type="cite"> <meta http-equiv="Content-Type" content="text/html;
charset=UTF-8">
Hi Ali, Magnus,<br>
<br>
I absolutely agree this change has to be reviewed by someone
from serviceability.<br>
<br>
There are several options:<br>
<br>
1. Add -export:jdwpTransport_OnLoad to LDFLAGS for Windows<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext" \
href="http://mail.openjdk.java.net/pipermail/build-dev/2018-November/023935.html">http://mail.openjdk.java.net/pipermail/build-dev/2018-November/023935.html</a><br>
so it partially reverts the changes from<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext" \
href="https://bugs.openjdk.java.net/browse/JDK-8200178">https://bugs.openjdk.java.net/browse/JDK-8200178</a><br>
<br>
2. Remove JNICALL (__stdcall) modifier, eventually changing the
calling convention to __cdecl.<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext" \
href="http://mail.openjdk.java.net/pipermail/build-dev/2018-November/023969.html">http://mail.openjdk.java.net/pipermail/build-dev/2018-November/023969.html</a><br>
<br>
3. Use inline /export option via #pragma:<br>
#pragma comment(linker, "/export:jdwpTransport_OnLoad=
_jdwpTransport_OnLoad@16")<br>
as referenced in<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext" \
href="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</a><br>
<a moz-do-not-send="true" class="moz-txt-link-freetext" \
href="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</a><br>
<br>
The third options still needs to be tested to make sure it does
not break other platforms builds.<br>
</blockquote>
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.</blockquote>
<br>
That is why I proposed removing JNICALL (__stdcall) from the
function declaration as we had done for libzip, libjimage [1] and
mlib_image [2].<br>
<br>
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.<br>
<br>
<br>
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.<br>
<br>
It shouldn't be a problem as long as all function declarations use
the same calling convention, which, I believe, is the case here.<br>
<br>
<blockquote type="cite" \
cite="mid:17e7873f-f2f4-dc02-82a6-2d7eb1565ca2@oracle.com">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.<br>
</blockquote>
<br>
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.<br>
<br>
If we are to use __stdcall, it seems to be the only way to export
the undecorated name.<br>
<br>
<blockquote type="cite" \
cite="mid:17e7873f-f2f4-dc02-82a6-2d7eb1565ca2@oracle.com"> The same goes for def \
files, as suggested by Ali. It's just yet another version of option 1 in \
disguise/map files.<br> </blockquote>
<br>
If option 2 is rejected, I'm for using option 3. If option 3 cannot
be used too, I'm for option 1.<br>
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.<br>
<br>
<br>
Regards,<br>
Alexey<br>
<br>
[1] <a class="moz-txt-link-freetext" \
href="https://bugs.openjdk.java.net/browse/JDK-8201226">https://bugs.openjdk.java.net/browse/JDK-8201226</a><br>
[2] <a class="moz-txt-link-freetext" \
href="https://bugs.openjdk.java.net/browse/JDK-8202476">https://bugs.openjdk.java.net/browse/JDK-8202476</a><br>
<blockquote type="cite" cite="mid:17e7873f-f2f4-dc02-82a6-2d7eb1565ca2@oracle.com"> \
<br> /Magnus<br>
<br>
<blockquote cite="mid:01b80c3f-8f71-26ec-304e-a7b245924bd5@oracle.com" \
type="cite"> <br> <br>
Regards,<br>
Alexey<br>
<br>
<div class="moz-cite-prefix">On 19/11/2018 12:19, Magnus Ihse
Bursie wrote:<br>
</div>
<blockquote type="cite" \
cite="mid:34d2b504-c4eb-6df3-3bd1-e23b9c22ea6a@oracle.com"> <meta \
http-equiv="Content-Type" content="text/html; charset=UTF-8">
<div class="moz-cite-prefix">Hi Ali,<br>
<br>
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. <br>
<br>
I'm cc:ing servicability so they can have a look at it.<br>
<br>
/Magnus<br>
<br>
On 2018-11-18 13:07, Ali İnce wrote:<br>
</div>
<blockquote type="cite" \
cite="mid:B6E023FE-7ED5-4483-AAEC-402DC87C32C1@gmail.com"> <pre wrap="">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 <a class="moz-txt-link-rfc2396E" href="mailto:ali.ince@gmail.com" \
moz-do-not-send="true"><ali.ince@gmail.com></a> # 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)
{
</pre>
<blockquote type="cite">
<pre wrap="">On 16 Nov 2018, at 23:03, Alexey Ivanov <a \
class="moz-txt-link-rfc2396E" href="mailto:alexey.ivanov@oracle.com" \
moz-do-not-send="true"><alexey.ivanov@oracle.com></a> 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 <a class="moz-txt-link-freetext" \
href="https://bugs.openjdk.java.net/browse/JDK-8200178" \
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8200178</a> 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:
</pre>
<blockquote type="cite">
<pre wrap="">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 (<a \
class="moz-txt-link-freetext" \
href="https://docs.microsoft.com/en-gb/cpp/build/reference/decorated-names?view=vs-2017" \
moz-do-not-send="true">https://docs.microsoft.com/en-gb/cpp/build/reference/decorated-names?view=vs-2017</a>). \
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 <a \
class="moz-txt-link-freetext" \
href="https://github.com/AdoptOpenJDK/openjdk-jdk11u/blob/5f01925b80ed851b133ee26fbcb07026ac04149e/src/jdk.jdwp.agent/share/native/libjdwp/transport.c#L99" \
moz-do-not-send="true">https://github.com/AdoptOpenJDK/openjdk-jdk11u/blob/5f01925b80e \
d851b133ee26fbcb07026ac04149e/src/jdk.jdwp.agent/share/native/libjdwp/transport.c#L99</a>.
Any thoughts?
Regards,
Ali
</pre>
<blockquote type="cite">
<pre wrap="">On 15 Nov 2018, at 22:14, Alexey Ivanov <a \
class="moz-txt-link-rfc2396E" href="mailto:alexey.ivanov@oracle.com" \
moz-do-not-send="true"><alexey.ivanov@oracle.com></a> 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
<a class="moz-txt-link-freetext" \
href="https://bugs.openjdk.java.net/browse/JDK-8201226" \
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8201226</a> <a \
class="moz-txt-link-freetext" \
href="http://mail.openjdk.java.net/pipermail/build-dev/2018-April/021553.html" \
moz-do-not-send="true">http://mail.openjdk.java.net/pipermail/build-dev/2018-April/021553.html</a>
<a class="moz-txt-link-freetext" \
href="https://bugs.openjdk.java.net/browse/JDK-8202476" \
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8202476</a> <a \
class="moz-txt-link-freetext" \
href="http://mail.openjdk.java.net/pipermail/build-dev/2018-May/021913.html" \
moz-do-not-send="true">http://mail.openjdk.java.net/pipermail/build-dev/2018-May/021913.html</a>
Regards,
Alexey
On 15/11/2018 21:43, Ali İnce wrote:
</pre>
<blockquote type="cite">
<pre wrap="">Hi,
An issue (<a class="moz-txt-link-freetext" \
href="https://github.com/AdoptOpenJDK/openjdk-build/issues/709" \
moz-do-not-send="true">https://github.com/AdoptOpenJDK/openjdk-build/issues/709</a>) \
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 <a \
class="moz-txt-link-freetext" \
href="https://github.com/AdoptOpenJDK/openjdk-jdk11u/pull/1" \
moz-do-not-send="true">https://github.com/AdoptOpenJDK/openjdk-jdk11u/pull/1</a>.
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, \
</pre>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</div></blockquote></body></html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic