Обсуждение: Removing dependency to wsock32.lib when compiling code on WIndows

Hi all,

When doing some work on Windows, I noticed that the mkvc specs in
src/tools/msvc use wsock32.lib, which is as far as I understand an
old, obsolete version of the Windows socket library. Wouldn't it make
sense to update the specs to build only with ws2_32.lib like in the
patch attached?
Regards,
--
Michael

From: "Michael Paquier" <michael.paquier@gmail.com>
> When doing some work on Windows, I noticed that the mkvc specs in
> src/tools/msvc use wsock32.lib, which is as far as I understand an
> old, obsolete version of the Windows socket library. Wouldn't it make
> sense to update the specs to build only with ws2_32.lib like in the
> patch attached?

Yes, that makes sense, because wsock32.dll is an obsolete WinSock 1.1 
library, while ws2_32.dll is a successor WinSock 2.0 library which is fully 
compatible with the old one.

Doing "grep -lir wsock32 ." shows the following files.  Could you modify and 
test these files as well for code cleanup?

./configure
./configure.in
./contrib/pgcrypto/Makefile
./src/interfaces/libpq/Makefile
./src/interfaces/libpq/win32.c
./src/interfaces/libpq/win32.mak
./src/test/thread/README
./src/tools/msvc/Mkvcbuild.pm


Regards
MauMau

On Wed, Jun 18, 2014 at 8:37 PM, MauMau <maumau307@gmail.com> wrote:
> Doing "grep -lir wsock32 ." shows the following files.  Could you modify and
> test these files as well for code cleanup?
> ./configure
> ./configure.in
> ./contrib/pgcrypto/Makefile
> ./src/interfaces/libpq/Makefile
> ./src/interfaces/libpq/win32.c
> ./src/interfaces/libpq/win32.mak
> ./src/test/thread/README
> ./src/tools/msvc/Mkvcbuild.pm
You are right, I only though about the MS scripts when working on this
patch. Updated patch for a complete cleanup is attached (note I
updated manually ./configure for test purposes and did not re-run
autoconf).
Regards,
--
Michael

From: "Michael Paquier" <michael.paquier@gmail.com>
> You are right, I only though about the MS scripts when working on this
> patch. Updated patch for a complete cleanup is attached (note I
> updated manually ./configure for test purposes and did not re-run
> autoconf).

I marked this patch as ready for committer.  I confirmed:

* The patch content is correct.

* The patch applies cleanly.  Running "grep -lir wsock32 ." after applying 
the patch shows that wsock32.lib is no longer used.

* The binaries can be built with MSVC 2008 Express.  I didn't try to build 
on MinGW.

* The regression tests pass ("vcregress check").

However, I wonder if some DLL entries in dlls[] in 
src/interfaces/libpq/win32.c should be removed.  winsock.dll should 
definitely be removed, because it is for 16-bit applications.  I don't know 
about the rest.  Especially,  wsock32n.dll and mswsock.dll may also be 
obsolete libraries from their names.  Could you look into this and revise 
the patch if possible?  But I don't mind if you do so.

Regards
MauMau

On Thu, Jun 19, 2014 at 11:13 PM, MauMau <maumau307@gmail.com> wrote:
> * The patch applies cleanly.  Running "grep -lir wsock32 ." after applying
> the patch shows that wsock32.lib is no longer used.
> However, I wonder if some DLL entries in dlls[] in
> src/interfaces/libpq/win32.c should be removed.  winsock.dll should
> definitely be removed, because it is for 16-bit applications.  I don't know
> about the rest.  Especially,  wsock32n.dll and mswsock.dll may also be
> obsolete libraries from their names.  Could you look into this and revise
> the patch if possible?  But I don't mind if you do so.
(google-sensei..) msock32n stands for Hummingbird Socks V4 Winsock
while mswsock implements the Windows socket SPI interface. I think we
should keep them and not risking opening a can of worms, but perhaps a
Windows guru has a different opinion on the matter.
-- 
Michael

On Thu, Jun 19, 2014 at 4:13 PM, MauMau <maumau307@gmail.com> wrote:
> From: "Michael Paquier" <michael.paquier@gmail.com>
>
>> You are right, I only though about the MS scripts when working on this
>> patch. Updated patch for a complete cleanup is attached (note I
>> updated manually ./configure for test purposes and did not re-run
>> autoconf).
>
>
> I marked this patch as ready for committer.  I confirmed:

I'm pretty sure this is not ready for commiter due to:

> * The binaries can be built with MSVC 2008 Express.  I didn't try to build
> on MinGW.

Somebody needs to test it on mingw first :)

That should be an easy test for someone who has a mingw install
around, so better leave it at "needs review" so more people see it and
can run those tests.


> * The regression tests pass ("vcregress check").
>
> However, I wonder if some DLL entries in dlls[] in
> src/interfaces/libpq/win32.c should be removed.  winsock.dll should
> definitely be removed, because it is for 16-bit applications.  I don't know
> about the rest.  Especially,  wsock32n.dll and mswsock.dll may also be
> obsolete libraries from their names.  Could you look into this and revise
> the patch if possible?  But I don't mind if you do so.

I'm with michael about the "scaryness" of touching this. Also, it is
definitely a separate patch if we do...


-- Magnus HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/

On Tue, Jul 15, 2014 at 8:46 PM, Magnus Hagander <magnus@hagander.net> wrote:
> On Thu, Jun 19, 2014 at 4:13 PM, MauMau <maumau307@gmail.com> wrote:
>> From: "Michael Paquier" <michael.paquier@gmail.com>
>>
>>> You are right, I only though about the MS scripts when working on this
>>> patch. Updated patch for a complete cleanup is attached (note I
>>> updated manually ./configure for test purposes and did not re-run
>>> autoconf).
>>
>>
>> I marked this patch as ready for committer.  I confirmed:
>
> I'm pretty sure this is not ready for commiter due to:
>
>> * The binaries can be built with MSVC 2008 Express.  I didn't try to build
>> on MinGW.
>
> Somebody needs to test it on mingw first :)
>
> That should be an easy test for someone who has a mingw install
> around, so better leave it at "needs review" so more people see it and
> can run those tests.
I vaguely recall that I tested as well with MinGW when writing v2 of
this patch after MauMau reviewed v1. Btw, just in case, I have just
made working my MinGW box a bit more and re-tested it now. And it
worked :)

Of course someone else than the patch author with a MinGW environment
at hand is invited to test.
-- 
Michael

On Tue, Jul 15, 2014 at 2:14 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Tue, Jul 15, 2014 at 8:46 PM, Magnus Hagander <magnus@hagander.net> wrote:
>> On Thu, Jun 19, 2014 at 4:13 PM, MauMau <maumau307@gmail.com> wrote:
>>> From: "Michael Paquier" <michael.paquier@gmail.com>
>>>
>>>> You are right, I only though about the MS scripts when working on this
>>>> patch. Updated patch for a complete cleanup is attached (note I
>>>> updated manually ./configure for test purposes and did not re-run
>>>> autoconf).
>>>
>>>
>>> I marked this patch as ready for committer.  I confirmed:
>>
>> I'm pretty sure this is not ready for commiter due to:
>>
>>> * The binaries can be built with MSVC 2008 Express.  I didn't try to build
>>> on MinGW.
>>
>> Somebody needs to test it on mingw first :)
>>
>> That should be an easy test for someone who has a mingw install
>> around, so better leave it at "needs review" so more people see it and
>> can run those tests.
> I vaguely recall that I tested as well with MinGW when writing v2 of
> this patch after MauMau reviewed v1. Btw, just in case, I have just
> made working my MinGW box a bit more and re-tested it now. And it
> worked :)
>
> Of course someone else than the patch author with a MinGW environment
> at hand is invited to test.

Thanks!

I think that's enough - the functionality has already been verified
elsewhere, and the contents look good :) We'll use the buidlfarm to
see if there are any weird version-dependencies...

Thus - applied!


-- Magnus HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/

On Tue, Jul 15, 2014 at 9:20 PM, Magnus Hagander <magnus@hagander.net> wrote:
> On Tue, Jul 15, 2014 at 2:14 PM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> Of course someone else than the patch author with a MinGW environment
>> at hand is invited to test.
>
> Thanks!
>
> I think that's enough - the functionality has already been verified
> elsewhere, and the contents look good :) We'll use the buidlfarm to
> see if there are any weird version-dependencies...
>
> Thus - applied!
OK. Thanks a lot!
-- 
Michael

On Mon, Aug 11, 2014 at 11:02:48AM -0700, Jeff Janes wrote:
> While trying to test more recent stuff against mingw, I kept running into a
> problem which bisected down to this (a16bac36eca8158cbf78987e953).

> This is the warning/error I get:
> 
> auth.c: In function 'ClientAuthentication':
> auth.c:458:6: warning: implicit declaration of function 'gai_strerror'
> [-Wimplicit-function-declaration]

> libpq/auth.o:auth.c:(.text+0x1949): undefined reference to `gai_strerror'
> libpq/auth.o:auth.c:(.text+0x19c4): undefined reference to `gai_strerror'
> libpq/auth.o:auth.c:(.text+0x1b1a): undefined reference to `gai_strerror'
> libpq/auth.o:auth.c:(.text+0x1cb4): undefined reference to `gai_strerror'
> libpq/auth.o:auth.c:(.text+0x1cdc): undefined reference to `gai_strerror'

> Any suggestions on what to try?  This patch doesn't seem to explicitly
> mention gai_strerror at all, so it must be some indirect effect.

I, too, encountered that.  The patch let "configure" detect HAVE_GETADDRINFO
under MinGW-w64; passing "ac_cv_func_getaddrinfo=no" on the configure command
line is a workaround.  Under !HAVE_GETADDRINFO, "configure" arranges to build
src/port/getaddrinfo.c, and src/include/getaddrinfo.h injects the replacement
gai_strerror() prototype.  That understandably doesn't happen in a
HAVE_GETADDRINFO build, yet src/include/port/win32/sys/socket.h does the
following unconditionally:

/** we can't use the windows gai_strerror{AW} functions because* they are defined inline in the MS header files. So
we'lluse our* own*/
 
#undef gai_strerror

Somehow or other, we must bring these parts into agreement.

-- 
Noah Misch
EnterpriseDB                                 http://www.enterprisedb.com

On Tue, Aug 12, 2014 at 9:43 AM, Noah Misch <noah@leadboat.com> wrote:
>
> Somehow or other, we must bring these parts into agreement.


It is interesting to see that with MinGW-32b getaddrinfo is still set
to no at configure phase. What if we simply wrap "undef gai_strerror"
like in the patch attached? I just set up an environment with MinGW-64
and I was able to build the code (tested as well with MinGW-32 btw).

Another idea would be to enforce getaddrinfo to no at configure for
MinGW environments.
Thoughts?
--
Michael

On Tue, Aug 12, 2014 at 01:58:17PM +0900, Michael Paquier wrote:
> On Tue, Aug 12, 2014 at 9:43 AM, Noah Misch <noah@leadboat.com> wrote:
> >
> > Somehow or other, we must bring these parts into agreement.
> 
> 
> It is interesting to see that with MinGW-32b getaddrinfo is still set
> to no at configure phase. What if we simply wrap "undef gai_strerror"
> like in the patch attached? I just set up an environment with MinGW-64
> and I was able to build the code (tested as well with MinGW-32 btw).

It's easy to devise something that fixes the build.  What is the right fix,
and why?

Note that MinGW-w64 is available in both 32-bit and 64-bit.  It is a fork of
MinGW, which is always 32-bit.  I experienced the problem with 64-bit
MinGW-w64; I don't know how 32-bit MinGW-w64 compares.

> Another idea would be to enforce getaddrinfo to no at configure for
> MinGW environments.

Consistency with the MSVC build is desirable, either HAVE_GETADDRINFO for both
or !HAVE_GETADDRINFO for both.

nm

-- 
Noah Misch
EnterpriseDB                                 http://www.enterprisedb.com

On Mon, Aug 11, 2014 at 10:48 PM, Noah Misch <noah@leadboat.com> wrote:
> Consistency with the MSVC build is desirable, either HAVE_GETADDRINFO for both
> or !HAVE_GETADDRINFO for both.

Hm. Looking here getattrinfo has been added in ws2_32 and was not
present in wsock32:
http://msdn.microsoft.com/en-us/library/windows/desktop/ms738520%28v=vs.85%29.aspx

And this change in configure.in is the root cause of the regression:
-AC_SEARCH_LIBS(socket, [socket wsock32])
+AC_SEARCH_LIBS(socket, [socket ws2_32])

Btw, how do you determine if MSVC is using HAVE_GETADDRINFO? Is it
decided by the inclusion of getaddrinfo.c in @pgportfiles of
Mkvdbuild.pm?
-- 
Michael

On Fri, Aug 15, 2014 at 12:49:36AM -0700, Michael Paquier wrote:
> Btw, how do you determine if MSVC is using HAVE_GETADDRINFO? Is it
> decided by the inclusion of getaddrinfo.c in @pgportfiles of
> Mkvdbuild.pm?

src/include/pg_config.h.win32 dictates it, and we must keep @pgportfiles
synchronized with the former's verdict.

-- 
Noah Misch
EnterpriseDB                                 http://www.enterprisedb.com

On 08/15/2014 11:00 PM, Noah Misch wrote:
> On Fri, Aug 15, 2014 at 12:49:36AM -0700, Michael Paquier wrote:
>> Btw, how do you determine if MSVC is using HAVE_GETADDRINFO? Is it
>> decided by the inclusion of getaddrinfo.c in @pgportfiles of
>> Mkvdbuild.pm?
> src/include/pg_config.h.win32 dictates it, and we must keep @pgportfiles
> synchronized with the former's verdict.
>


What's happening about this? Buildfarm animal jacana is consistently red 
because of this.

cheers

andrew

On Thu, Aug 21, 2014 at 09:18:22AM -0400, Andrew Dunstan wrote:
> 
> On 08/15/2014 11:00 PM, Noah Misch wrote:
> >On Fri, Aug 15, 2014 at 12:49:36AM -0700, Michael Paquier wrote:
> >>Btw, how do you determine if MSVC is using HAVE_GETADDRINFO? Is it
> >>decided by the inclusion of getaddrinfo.c in @pgportfiles of
> >>Mkvdbuild.pm?
> >src/include/pg_config.h.win32 dictates it, and we must keep @pgportfiles
> >synchronized with the former's verdict.
> >
> 
> 
> What's happening about this? Buildfarm animal jacana is consistently
> red because of this.

If nobody plans to do the aforementioned analysis in the next 4-7 days, I
suggest we adopt one of Michael's suggestions: force "configure" to reach its
old conclusion about getaddrinfo() on Windows.  Then the analysis can proceed
on an unhurried schedule.

On Fri, Aug 22, 2014 at 04:58:47PM +0900, Michael Paquier wrote:
> Looking more into that, I am seeing that MinGW-32 is failing to find socket
> at configure, contrary to MinGW-64.
> 
> Here is what happens for MinGW-64 at configure:
> configure:7638: checking for library containing socket
> [...]
> configure:7669: x86_64-w64-mingw32-gcc -o conftest.exe  -Wall
> -Wmissing-prototypes -Wpointer-arith \
> -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute
> -Wformat-security -fno-stri\
> ct-aliasing -fwrapv -fexcess-precision=standard -g
>  -I./src/include/port/win32 -DEXEC_BACKEND   -Wl\
> ,--allow-multiple-definition -Wl,--disable-auto-import  conftest.c -lws2_32
>  -lm  >&5
> configure:7669: $? = 0
> configure:7686: result: -lws2_32
> 
> And for MinGW-32:
> configure:7638: checking for library containing socket
> [...]
> configure:7669: gcc -o conftest.exe  -Wall -Wmissing-prototypes
> -Wpointer-arith -Wdeclaration-after\
> -statement -Wendif-labels -Wmissing-format-attribute -Wformat-security
> -fno-strict-aliasing -fwrapv\
>  -fexcess-precision=standard -g  -I./src/include/port/win32
> -DEXEC_BACKEND   -Wl,--allow-multiple-d\
> efinition -Wl,--disable-auto-import  conftest.c -lws2_32  -lm  >&5
> C:\Users\ioltas\AppData\Local\Temp\cciNV1Y8.o: In function `main':
> c:\Users\ioltas\git\postgres/conftest.c:33: undefined reference to `socket'
> collect2.exe: error: ld returned 1 exit status

I see that ws2_32.def for 64-bit MinGW-w64 exports plain "socket", while
32-bit MinGW-w64 and 32-bit MinGW export "socket@12".  In other words, 64-bit
MinGW-w64 exports a "__cdecl socket" function, and the others export a
"__stdcall socket" function.  AC_SEARCH_LIBS doesn't work with __stdcall.

> I am not sure which way is better, do we want HAVE_GETADDRINFO or
> !HAVE_GETADDRINFO in all Windows builds? If we choose the former, we'll
> need to understand why -lws2_32 is not added to LIBS for MinGW-32. If we
> choose the latter, we would need to remove -lws2_32 from LIBS with MinGW-64
> for consistency with MinGW-32 I think.

HAVE_GETADDRINFO is preferable whenever the OS functions getaddrinfo.h would
replace are fully adequate.  When PostgreSQL established its longstanding
Windows behavior in this area, that was not so.  A few generations of Windows
have since gone out of support, so perhaps the situation has changed.

On Thu, Aug 21, 2014 at 11:29:31PM -0400, Noah Misch wrote:
> On Thu, Aug 21, 2014 at 09:18:22AM -0400, Andrew Dunstan wrote:
> > What's happening about this? Buildfarm animal jacana is consistently
> > red because of this.
> 
> If nobody plans to do the aforementioned analysis in the next 4-7 days, I
> suggest we adopt one of Michael's suggestions: force "configure" to reach its
> old conclusion about getaddrinfo() on Windows.  Then the analysis can proceed
> on an unhurried schedule.

Done.

Incidentally, jacana takes an exorbitant amount of time, most recently 87
minutes, to complete the ecpg-check step.  Frogmouth takes 4 minutes, and none
of the other steps have such a large multiplier between the two animals.  That
pattern isn't new and appears on multiple branches.  I wonder if ecpg tickles
a performance bug in 64-bit MinGW-w64.

On 08/29/2014 10:15 AM, Noah Misch wrote:
> On Thu, Aug 21, 2014 at 11:29:31PM -0400, Noah Misch wrote:
>> On Thu, Aug 21, 2014 at 09:18:22AM -0400, Andrew Dunstan wrote:
>>> What's happening about this? Buildfarm animal jacana is consistently
>>> red because of this.
>> If nobody plans to do the aforementioned analysis in the next 4-7 days, I
>> suggest we adopt one of Michael's suggestions: force "configure" to reach its
>> old conclusion about getaddrinfo() on Windows.  Then the analysis can proceed
>> on an unhurried schedule.
> Done.
>
> Incidentally, jacana takes an exorbitant amount of time, most recently 87
> minutes, to complete the ecpg-check step.  Frogmouth takes 4 minutes, and none
> of the other steps have such a large multiplier between the two animals.  That
> pattern isn't new and appears on multiple branches.  I wonder if ecpg tickles
> a performance bug in 64-bit MinGW-w64.
>


Good pickup. I wonder what it could be.

cheers

andrew