0005766: MSVC port broken following switch to winsock2

(0008157)
frisch (developer)
2012-09-25 01:07

Same for mingw 64 port.

(0008161)
vouillon (reporter)
2012-09-25 11:12

The piece of code using SO_SYNCHRONOUS_NONALERT was removed by commit 11966 to fix issue 0005325. It was merged back into trunk from branch 4.00 by commit 12784. (It was left in branch 4.00 as a workaround to issue 0005578.)

If we want to keep this piece of code, we should include mswsock.h (diff below) rather than defining the symbols by hand.

Is it really necessary to include io.h (from directory byterun/, not the windows header file)? This does not seem to be required by the mingw port. Maybe we should instead include windows.h rather than winbase.h in unixsupport.h?

Index: otherlibs/win32unix/unixsupport.h
===================================================================
--- otherlibs/win32unix/unixsupport.h (révision 12952)
+++ otherlibs/win32unix/unixsupport.h (copie de travail)
@@ -21,6 +21,7 @@
#include <process.h>
#include <sys/types.h>
#include <winsock2.h>
+#include <mswsock.h>
#ifdef HAS_IPV6
#include <ws2tcpip.h>
#include <wspiapi.h>

(0008162)
frisch (developer)
2012-09-25 12:30

Thanks. Indeed, including mswsock.h is enough for the mingw port (tested with mingw 64 only). For the msvc port, we need to include something more to define HANDLE, otherwise we get:

cl /nologo -D_CRT_SECURE_NO_DEPRECATE /Ox /MD -I../../byterun -I../unix -c createprocess.c
createprocess.c
c:\cygwin\home\afrisch\ocaml\record-disambiguation\otherlibs\win32unix\unixsupport.h(47) : error C2061: syntax error : identifier 'win_alloc_handle'
....

Including windows.h instead of winbase.h in unixsupport.h (but not io.h) does not seem to work.

(0008163)
vouillon (reporter)
2012-09-25 13:48

It's not 'HANDLE' which is not defined, but 'value'. Hence, mlvalues.h should be included before unixsupport.h in all C files. I'm to blame for this. I have attached a patch that should fix both issues.

With mingw, the Windows header file direct.h included from unixsupport.h includes file io.h (from directory byterun!), which itself includes mlvalues.h...

(0008164)
frisch (developer)
2012-09-26 06:19

Thanks for the patch, I've committed it to the trunk (rev 12956). mswsock.h was also required in socket.c. What about including it from unixsupport.h? Do you see any problem?

(0008165)
vouillon (reporter)
2012-09-26 10:36

I don't think there is any problem with including it in unixsupport.h.

But maybe the right fix is to remove the getsockopt/setsockopt calls (reverting the corresponding part of commit 12784)?

(0008166)
frisch (developer)
2012-09-27 05:45

Well, as long as there is no nice solution implemented for 0005578, I think getsockopt/setsockopt calls have to remain.

(0008177)
doligez (manager)
2012-09-29 22:35

Alain, does your commit of Jerome's patch fix the problem? Should we close this PR?

(0008179)
frisch (developer)
2012-09-29 22:50

I think so, but I haven't tried to compile under mingw 32 bit and msvc 64 bit.

(0008180)
doligez (manager)
2012-09-30 14:33

I've compiled under mingw 32 bit, with a small (unrelated) fix to otherlibs/labltk/lib/Makefile (r 12978).

(0008193)
doligez (manager)
2012-10-02 17:49

For the record, I have also compiled successfully with msvc 64-bit.

Issue History
Date Modified	Username	Field	Change
2012-09-25 00:44	frisch	New Issue
2012-09-25 01:07	frisch	Note Added: 0008157
2012-09-25 01:07	frisch	Relationship added	related to 0005676
2012-09-25 11:12	vouillon	Note Added: 0008161
2012-09-25 12:30	frisch	Note Added: 0008162
2012-09-25 13:44	vouillon	File Added: patch.txt
2012-09-25 13:48	vouillon	Note Added: 0008163
2012-09-26 06:19	frisch	Note Added: 0008164
2012-09-26 10:36	vouillon	Note Added: 0008165
2012-09-27 05:45	frisch	Note Added: 0008166
2012-09-29 22:35	doligez	Note Added: 0008177
2012-09-29 22:35	doligez	Status	new => feedback
2012-09-29 22:50	frisch	Note Added: 0008179
2012-09-29 22:50	frisch	Status	feedback => new
2012-09-30 14:33	doligez	Note Added: 0008180
2012-10-02 14:54	frisch	Status	new => resolved
2012-10-02 14:54	frisch	Resolution	open => fixed
2012-10-02 14:54	frisch	Assigned To	=> frisch
2012-10-02 17:49	doligez	Note Added: 0008193

Notes
(0008157) frisch (developer) 2012-09-25 01:07	Same for mingw 64 port.

(0008161) vouillon (reporter) 2012-09-25 11:12	The piece of code using SO_SYNCHRONOUS_NONALERT was removed by commit 11966 to fix issue 0005325. It was merged back into trunk from branch 4.00 by commit 12784. (It was left in branch 4.00 as a workaround to issue 0005578.) If we want to keep this piece of code, we should include mswsock.h (diff below) rather than defining the symbols by hand. Is it really necessary to include io.h (from directory byterun/, not the windows header file)? This does not seem to be required by the mingw port. Maybe we should instead include windows.h rather than winbase.h in unixsupport.h? Index: otherlibs/win32unix/unixsupport.h =================================================================== --- otherlibs/win32unix/unixsupport.h (révision 12952) +++ otherlibs/win32unix/unixsupport.h (copie de travail) @@ -21,6 +21,7 @@ #include <process.h> #include <sys/types.h> #include <winsock2.h> +#include <mswsock.h> #ifdef HAS_IPV6 #include <ws2tcpip.h> #include <wspiapi.h>

(0008162) frisch (developer) 2012-09-25 12:30	Thanks. Indeed, including mswsock.h is enough for the mingw port (tested with mingw 64 only). For the msvc port, we need to include something more to define HANDLE, otherwise we get: cl /nologo -D_CRT_SECURE_NO_DEPRECATE /Ox /MD -I../../byterun -I../unix -c createprocess.c createprocess.c c:\cygwin\home\afrisch\ocaml\record-disambiguation\otherlibs\win32unix\unixsupport.h(47) : error C2061: syntax error : identifier 'win_alloc_handle' .... Including windows.h instead of winbase.h in unixsupport.h (but not io.h) does not seem to work.

(0008163) vouillon (reporter) 2012-09-25 13:48	It's not 'HANDLE' which is not defined, but 'value'. Hence, mlvalues.h should be included before unixsupport.h in all C files. I'm to blame for this. I have attached a patch that should fix both issues. With mingw, the Windows header file direct.h included from unixsupport.h includes file io.h (from directory byterun!), which itself includes mlvalues.h...

(0008164) frisch (developer) 2012-09-26 06:19	Thanks for the patch, I've committed it to the trunk (rev 12956). mswsock.h was also required in socket.c. What about including it from unixsupport.h? Do you see any problem?

(0008165) vouillon (reporter) 2012-09-26 10:36	I don't think there is any problem with including it in unixsupport.h. But maybe the right fix is to remove the getsockopt/setsockopt calls (reverting the corresponding part of commit 12784)?

(0008166) frisch (developer) 2012-09-27 05:45	Well, as long as there is no nice solution implemented for 0005578, I think getsockopt/setsockopt calls have to remain.

(0008177) doligez (manager) 2012-09-29 22:35	Alain, does your commit of Jerome's patch fix the problem? Should we close this PR?

(0008179) frisch (developer) 2012-09-29 22:50	I think so, but I haven't tried to compile under mingw 32 bit and msvc 64 bit.

(0008180) doligez (manager) 2012-09-30 14:33	I've compiled under mingw 32 bit, with a small (unrelated) fix to otherlibs/labltk/lib/Makefile (r 12978).

(0008193) doligez (manager) 2012-10-02 17:49	For the record, I have also compiled successfully with msvc 64-bit.