Thank you, Daniel, for your quick review.
On 19/12/2018 15:05, Daniel D. Daugherty wrote:
On 12/19/18 9:23 AM, Alexey Ivanov wrote:
Any volunteers from core-libs and serviceability to review?
How about former Serviceability Team members? :-) Alan B. and I
both used to be on the Serviceability Team... And Alan B. is a
current member of Core Libs...
That's fine, I guess. :-)
> webrev: http://cr.openjdk.java.net/~aivanov/8214122/webrev.01/
src/jdk.jdwp.agent/share/native/libjdwp/transport.c
No comments.
The code appears to do what is described in this (very long) review
thread. I don't believe we (Oracle) have any Win32 test setups for
the jdk/jdk repo so this isn't a change that someone from Oracle can
do additional testing on.
That's true.
I ran all the builds with tier3 testing, just in case.
I also verified locally that jshell.exe and debugger start successfully
in Win32 build.
--
Alexey
Thumbs up!
Dan
Regards,
Alexey
On 12/12/2018 16:43, Magnus Ihse Bursie wrote:
On 2018-12-12 17:38, Alexey Ivanov wrote:
Hi all,
I have updated the summary of JDK-8214122 and the subject accordingly.
Could you please review the following fix?
https://bugs.openjdk.java.net/browse/JDK-8214122
webrev: http://cr.openjdk.java.net/~aivanov/8214122/webrev.01/
Looks good to me.
/Magnus
*Root cause*
jdwpTransport_OnLoad is exported as _jdwpTransport_OnLoad@16 on 32
bit Windows according to the name decorations [1] for __stdcall [2]
calling conventions.
*Fix*
On 32 bit Windows, look up the decorated name,
_jdwpTransport_OnLoad@16, first; if not found, look up the
undecorated name, jdwpTransport_OnLoad.
Regards,
Alexey
[1]
https://docs.microsoft.com/en-us/cpp/build/reference/decorated-names?view=vs-2017#FormatC
[2] https://docs.microsoft.com/en-ie/cpp/cpp/stdcall?view=vs-2017
On 12/12/2018 11:19, Magnus Ihse Bursie wrote:
On 2018-12-11 18: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.
Yes! This seems like the correct fix.
Ship it! :)
/Magnus
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.
#if defined(_WIN32) && !defined(_WIN64) onLoad =
(jdwpTransport_OnLoad_t) dbgsysFindLibraryEntry(handle,
"_jdwpTransport_OnLoad@16"); #else onLoad =
(jdwpTransport_OnLoad_t)
dbgsysFindLibraryEntry(handle, "jdwpTransport_OnLoad"); #endif
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.
