[12] RFR for JDK-8214122: JDWP is broken on 32 bit Windows: transport library missing onLoad entry

[Alexey Ivanov]

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 at 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 at 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 at 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 at 16| for 
>>>>>>>>>> Win32
>>>>>>>>>> with fallback to undecorated one.
>>>>>>>>> Yes.
>>>>>>>>>
>>>>>>>>> I think the correct solution here is 2.
>>>>>>
>>>>>
>>>>
>>>
>>
>>
>