Hi Simon,
I'd like to clarify some points…
Please see my comments inline:
On 11/12/2018 17:16, Alexey Ivanov wrote:
Hi Simon,
Thank you for your comments.
The updated webrev:
http://cr.openjdk.java.net/~aivanov/8214122/webrev.01/
Indeed, it looks much cleaner.
Regards,
Alexey
On 11/12/2018 16:43, Simon Tooke wrote:
On 2018-12-11 10:05 a.m., Alexey Ivanov wrote:
Hi everyone,
I came up with the following patch:
http://cr.openjdk.java.net/~aivanov/8214122/webrev.00/
It specifically addresses the problem in JDK-8214122 where on 32 bit
Windows jdwpTransport_OnLoad can exported with its plain and
__stdcall-mangled name. I used conditional compilation so that for
other platforms the code remains as it is now.
jshell starts successfully with this fix; an IDE debugger works as
well.
I am not a reviewer, but this patch only works for the specific case
under discussion; the '@16' refers to the reserved stack size in the
Pascal calling convention. So, the patch only works for 16 bytes of
parameters. To be generic, the routine needs to have the stack size
passed in by the caller. If a generic fix isn't desired (and I hope it
is), I'd prefer to see the caller simply pass the decorated or
undecorated name depending on the Win32/64 defines.
At this time, it's only one function. So the current approach works well
enough, although it will not scale.
If other similar functions are added in the future, _OnUnload for
example, we should implement approach used in Hotspot as suggested by Bob:
http://mail.openjdk.java.net/pipermail/build-dev/2018-December/024373.html
#if defined(_WIN32) && !defined(_WIN64) onLoad =
(jdwpTransport_OnLoad_t) dbgsysFindLibraryEntry(handle,
"_jdwpTransport_OnLoad@16"); #else onLoad =
(jdwpTransport_OnLoad_t)
dbgsysFindLibraryEntry(handle, "jdwpTransport_OnLoad"); #endif
I believe falling back to undecorated name is the right thing in the
case where the decorated name isn't found. Only the undecorated name has
been looked up before. If a (third-party) JDWP transport DLL exports the
undecorated name only, it will fail to load. With fallback in place, it
will load successfully.
Does it sound reasonable?
Regards,
Alexey
Thanks,
-Simon
Regards,
Alexey
https://bugs.openjdk.java.net/browse/JDK-8214122
On 10/12/2018 15:11, Magnus Ihse Bursie wrote:
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.