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

[Daniel D. Daugherty]

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...

 > 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.

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.
>>>>>
>>>>
>>>
>>
>
>