> On 10 Dec 2018, at 15:11, Magnus Ihse Bursie <magnus.ihse.bur...@oracle.com>
> wrote:
>
>
>
> 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..?
A couple of MSFT quotes on name decoration from
https://docs.microsoft.com/en-gb/cpp/build/reference/decorated-names?view=vs-2017
says;
> Normally, you don't have to know the decorated name to write code that
> compiles and links successfully. Decorated names are an implementation detail
> internal to the compiler and linker.
> Name decoration is also important when linking to code written in other
> programming languages or using other compilers. Different compilers use
> different name decoration conventions. When your executable links to code
> written in another language, special care must be taken to match the exported
> and imported names and calling conventions.
And we should also keep in mind that MSFT itself doesn’t use name mangling in
the core windows API.
I still see this |jdwpTransport_OnLoad| not much different than
|JNI_CreateJavaVM| in the context of it’s availability and accessibility from
outer world.
>>
>> 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.
Does keeping name mangling in-place gives anyone any value? It would probably
only add the overhead of making the callers compute the stack size of the
parameters. And I couldn’t find any sign or history of changes but name
mangling, theoretically, it could change over versions of compilers.
The pragma-comment approach could also be overly simplified as shown
https://stackoverflow.com/a/41910450, which will leave us with a single line
addition to the function body as;
#pragma EXPORT
which is from my point of view a simpler (and maintainable) approach than
adding a decorated name lookup logic.
Regards,
Ali
>>
>>
>> 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-...@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, \
>>>>>>>>>>>>
>>>
>>
>
