Re: [PATCH] Windows 32-bit DLL name decoration

Mon, 10 Dec 2018 07:13:57 -0800 



On 2018-12-10 16:02, Alexey Ivanov wrote:



On 10/12/2018 10:41, Magnus Ihse Bursie wrote:



On 2018-12-07 22:18, Simon Tooke wrote:

Looking at the code, it seems (to me) the "correct" thing to do is to is
to create a Windows-specific version of dbgsysBuildFunName() in
linker_md.c, and have it decorate the function name as appropriate for
32-bit windows.  Note that in transport.c, you'd need to pass in the
parameter stack size (the bit after the @), but it could at least behave
properly on 32 vs 64 bit Windows.

Notice this approach is already used for the library name, via
dbgsysBuildLibName()

If the function is never intended to be called by Java via JNI, then it
should never have been declared JNICALL in the first place - but that
ship has sailed.

I don't think changing the decorated exported name to undecorated is a
good idea, as it obfuscates the calling convention used.

Good analysis, Simon. I agree.


I have now looked more into the situation. In fact, it looks a lot 
like JDWP has been broken on Windows 32-bit for a long time, but we 
have patched around the issue in the JDK by using /export. This is 
the spec we're dealing with: 
https://docs.oracle.com/javase/10/docs/specs/jdwp/jdwp-transport.html. 
I quote:



The transport library must export an/onload/function with the 
following prototype:



|JNIEXPORT jint JNICALL jdwpTransport_OnLoad(JavaVM *jvm, 
jdwpTransportCallback *callback, jint version, jdwpTransportEnv** env);|



This function will be called by the JDWP (or other agent) when the 
library is loaded.






Nothing here says anything that the function should be exported using 
anything other than the normal _stdcall (implied by JNICALL) name 
mangling rules. However, JDWP has not been working according to the 
spec and looking for the correct symbol, and while we could have 
noticed that in the JDK, we didn't, because someone (long ago) added 
/export:jdwpTransport_OnLoad as a workaround to the affected 
libraries, instead of fixing JDWP.



This means that we cannot change the calling convention: it must stay 
as it is now.



I think JDWP has always been working according to the spec. Using 
__stdcall is the recommended calling convention for Win32 and for DLLs 
in particular. Usually function names are exported undecorated in DLL. 
(If my memory serves me well, older Microsoft tools exported |extern 
"C" __stdcall| functions undecorated.)

So then the question becomes: what does the spec mean? That the 
__stdcall convention should be used, or that the function name should be 
explicitly exported under a non __stdcall-convention name? Neither 
behavior seems clearly written out, so this is left to an implicit 
interpretation of what that "usually" means..?



I believe this — exporting the undecorated function names — allows for 
interoperability between 32 bit and 64 bit in cases where you load a 
DLL and dynamically look up a function in it.



There were too many /export options to export Win32 functions 
undecorated for this one to be an accident rather than the intention.

How do you mean?



Now, given that we've shipped code that didn't adhere to the specs 
for so long, we can probably not break that behavior. Instead, I 
propose we amend dbgsysBuildFunName() so that on Windows 32-bit, it 
looks for both the "jdwpTransport_OnLoad", and the symbol as mangled 
by _stdcall rules. I also propose that if both symbols are present, 
the _stdcall named one should take precedence, since that's according 
to the spec (and the other is just a fallback to maintain backwards 
compatibility).


Since removing JNICALL is not an option, there are only two options:


1. Add |/export| option to the Makefile or pragma-comment to the 
source file;
2. Lookup the decorated name |_jdwpTransport_OnLoad@16| for Win32 with 
fallback to undecorated one.

Yes.

I think the correct solution here is 2.




I just wonder how it's possible to implement a generic 
|dbgsysBuildFunName| for an arbitrary function without knowing the 
size of function parameters. Could it be the reason why most DLLs 
export __stdcall functions undecorated?

(I assume you mean dbgsysFindLibraryEntry; there is a dbgsysBuildFunName 
as well but it appears to be dead code.)



The dbgsysFindLibraryEntry does not need to work with an arbitrary 
function. It's implemented in jdk.jdwp.agent, for the sole reason of 
locating the jdwpTransport_OnLoad function.



In general, I assume this to hold true. If you don't know the signature 
of the function you want to call, you're screwed anyway, regardless of 
calling convention.


/Magnus



Regards,
Alexey



/Magnus

-Simon Tooke


On 12/7/2018 2:09 PM, Alexey Ivanov wrote:

Hi Andrew,

Sorry for my belated reply.
Yes, it's also an option… however, this solution seems to be
overcomplicated to export one function or two. The calling convention
of exported functions for JVM cannot be changed, they can be called
from third-party software.

If the function is used internally-only, its calling convention can be
changed as long as all components are updated.

Thank you for the pointer to another approach used to handle name
decorations of __stdcall functions. It looks we have to work out a
common approach to this problem.

Regards,
Alexey

On 27/11/2018 18:49, Andrew Luo wrote:

By the way, in hotspot we are generating a .def file dynamically
while keeping the JNICALL calling convention (for symbols such as
JNI_CreateJavaVM) I believe (just by looking through the code in
http://hg.openjdk.java.net/jdk/jdk/file/44fe5fab538a/make/hotspot/lib/JvmMapfile.gmk,
unless this code is inactive - someone who knows this area better
than me might want to comment...).  Shouldn't the same approach be
taken here as well?

Thanks
Andrew

-----Original Message-----
From: core-libs-dev<core-libs-dev-boun...@openjdk.java.net>  On
Behalf Of Ali Ince
Sent: Monday, November 19, 2018 2:08 PM
To: Alexey Ivanov<alexey.iva...@oracle.com>; build-dev
<build-...@openjdk.java.net>; core-libs<core-libs-dev@openjdk.java.net>
Subject: Re: [PATCH] Windows 32-bit DLL name decoration

Hi Alexey,

I don’t have an account on JBS as I’m not an author yet, my OCA has
just been processed. Would it be possible for someone with an author
status to create an issue?

Regards,

Ali


On 19 Nov 2018, at 12:12, Alexey Ivanov<alexey.iva...@oracle.com>
wrote:

Hi Ali,

The fix looks good to me provided it resolves your problem.
I am not a reviewer so you'll have to get OK from reviewers, likely
from build-dev and from core-libs.

Have you submitted the issue in JBS?

You have to sign OCA to be able to contribute to OpenJDK:
https://openjdk.java.net/contribute/
and also
https://openjdk.java.net/sponsor/

Regards,
Alexey

On 18/11/2018 12: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>
# 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>
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>  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.h
tml

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, \




























					Reply via email to