RFR JDK-8005120

[Chris Hegarty]

Thank you John,

Looks fine to me. I can push it for you.

-Chris.

On 01/17/2013 06:07 PM, John Zavgren wrote:
> Greetings:
> I would like you to review recent changes that I made to our native
> socket code for the purpose of eliminating compiler warnings.
>
> These changes boil down to passing parameters with the correct type to
> various system calls as well as one "opportunistic" change that corrects
> the implementation of the macro: FT2INT64, that's used by windows:
>   #define FT2INT64(ft) \
> -        ((long)(ft).dwHighDateTime << 32 | (long)(ft).dwLowDateTime)
> +        ((INT64)(ft).dwHighDateTime << 32 | (INT64)(ft).dwLowDateTime)
>
> The webrev image is located at:
> http://cr.openjdk.java.net/~jzavgren/8005120/webrev.07/
> <http://cr.openjdk.java.net/%7Ejzavgren/8005120/webrev.07/>
>
> Thank you!
> John Zavgren
> john.zavgren at oracle.com
>
> On 12/20/2012 05:47 PM, John Zavgren wrote:
>> Greetings:
>>
>> I modified my changes so that windows knows the definition of the POSIX data type: socklen_t, and now all the system calls are using the "doctrinaire" data types.
>>
>> Please consider the following update.
>>
>> http://cr.openjdk.java.net/~mullan/webrevs/jzavgren/8005120/webrev.01/
>>
>> Thanks!
>> John Zavgren
>>
>>
>>
>> On 20/12/2012 13:49, John Zavgren wrote:
>>> Greetings:
>>>
>>> I agree that the "correct" way to fix this problem is to use POSIX data types, e.g., socklen_t. However, when I switch to the doctrinaire data type, the build fails on windows machines:
>>> ------------- build monologue -----
>>> c:\jprt\t\p1\032220.jzavgren\s\jdk\src\share\transport\socket\sysSocket.h(39) : error C2146: syntax error : missing ')' before identifier 'len'
>>> c:\jprt\t\p1\032220.jzavgren\s\jdk\src\share\transport\socket\sysSocket.h(39) : error C2081: 'socklen_t' : name in formal parameter list illegal
>>> c:\jprt\t\p1\032220.jzavgren\s\jdk\src\share\transport\socket\sysSocket.h(39) : error C2061: syntax error : identifier 'len'
>>> c:\jprt\t\p1\032220.jzavgren\s\jdk\src\share\transport\socket\sysSocket.h(39) : error C2059: syntax error : ';'
>>> c:\jprt\t\p1\032220.jzavgren\s\jdk\src\share\transport\socket\sysSocket.h(39) : error C2059: syntax error : ')'
>>> ....
>>> ------------- build monologue -----
>>>
>>> I used alternative types, e.g., uint32_t, etc. as a way to avoid the limitations of windows.
>>> What is the recommended way to accommodate this windows limitation? Shall I use a typedef statement to define "socklen_t"?
>> We don't suffer from this issue in the networking native code. The unix
>> and windows implementations are distinct.
>>
>> I see the vm defines socklen_t in a windows specific header,
>> hotspot/src/os/windows/vm/jvm_windows.h, as
>>      typedef int socklen_t;
>>
>>    ...and it is then used in shared code, like jvm.cpp, and the hpi, by
>> optionally including
>>
>>      #ifdef TARGET_OS_FAMILY_windows
>>      # include "jvm_windows.h"
>>      #endif
>>
>> We could use a similar, but more simplistic, approach here.
>>
>> -Chris.
>>
>>> Thanks!
>>>
>>>
>>> ----- Original Message -----
>>> From:chris.hegarty at oracle.com
>>> To:david.holmes at oracle.com
>>> Cc:Alan.Bateman at oracle.com,serviceability-dev at openjdk.java.net,john.zavgren at oracle.com,net-dev at openjdk.java.net
>>> Sent: Thursday, December 20, 2012 7:41:07 AM GMT -05:00 US/Canada Eastern
>>> Subject: Re: RFR JDK-8005120
>>>
>>> On 19/12/2012 20:52, David Holmes wrote:
>>>> Real sense of deja-vu here. Didn't we go through this same thing with
>>>> the HPI socket routines?
>>> Yes, and the networking native code too.
>>>
>>> I think it is best to use socklen_t for the unix code. From what I can
>>> see making these changes, to use socklen_t, should be relatively localized.
>>>
>>> -Chris.
>>>
>>>> Depending on the OS (and version?) we should be using socklen_t not int
>>>> and not uint32_t.
>>>>
>>>> David
>>>>
>>>> On 20/12/2012 2:35 AM, Chris Hegarty wrote:
>>>>> John,
>>>>>
>>>>> I grabbed your patch, and with it I now see different warnings.
>>>>>
>>>>> ../../../../src/share/transport/socket/socketTransport.c: In function
>>>>> 'socketTransport_startListening':
>>>>> ../../../../src/share/transport/socket/socketTransport.c:310:40:
>>>>> warning: pointer targets in passing argument 3 of 'dbgsysGetSocketName'
>>>>> differ in signedness [-Wpointer-sign]
>>>>> ../../../../src/share/transport/socket/sysSocket.h:58:5: note: expected
>>>>> 'uint32_t *' but argument is of type 'int *'
>>>>> ../../../../src/share/transport/socket/socketTransport.c: In function
>>>>> 'socketTransport_accept':
>>>>> ../../../../src/share/transport/socket/socketTransport.c:371:33:
>>>>> warning: pointer targets in passing argument 3 of 'dbgsysAccept' differ
>>>>> in signedness [-Wpointer-sign]
>>>>> ../../../../src/share/transport/socket/sysSocket.h:41:5: note: expected
>>>>> 'uint32_t *' but argument is of type 'int *'
>>>>>
>>>>> Do you see these in your build?
>>>>>
>>>>> -Chris.
>>>>>
>>>>> On 12/19/2012 03:42 PM, Alan Bateman wrote:
>>>>>> John - this is the debugger socket transport so cc'ing the
>>>>>> serviceability-dev list as that is where this code is maintained.
>>>>>>
>>>>>> On 19/12/2012 15:36, John Zavgren wrote:
>>>>>>> Greetings:
>>>>>>> Please consider the following change to the two files:
>>>>>>> src/share/transport/socket/sysSocket.h
>>>>>>> src/solaris/transport/socket/socket_md.c
>>>>>>> that eliminate compiler warnings that stem from the fact that the
>>>>>>> variables that the native code passes to various system calls were not
>>>>>>> declared correctly. They were declared as integers, but they must be
>>>>>>> "unsigned" integers because they are used to define buffer lengths.
>>>>>>> Were one to supply a negative value as an argument, it would be cast
>>>>>>> into an unsigned "Martian" value and there'd be (hopefully) a system
>>>>>>> call error.
>>>>>>>
>>>>>>> Thanks!
>>>>>>> John Zavgren
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~mullan/webrevs/jzavgren/8005120/
>
>
> --
> John Zavgren
> john.zavgren at oracle.com
> 603-821-0904
> US-Burlington-MA
>