Thread: msys inet_pton strangeness

msys inet_pton strangeness

From
Andrew Dunstan
Date:


A week or so ago I upgraded the msys2 animal fairywren to the latest msys2, and ever since then the build has been failing for Release 15. It's complaining like this:

ccache gcc -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Werror=vla -Wendif-labels -Wmissing-format-attribute -Wimplicit-fallthrough=3 -Wcast-function-type -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -Wno-format-truncation -Wno-stringop-truncation -g -O2  -DFRONTEND -DUNSAFE_STAT_OK -I/home/pgrunner/bf/root/REL_15_STABLE/pgsql.build/../pgsql/src/interfaces/libpq -I../../../src/include -I/home/pgrunner/bf/root/REL_15_STABLE/pgsql.build/../pgsql/src/include  -I../pgsql/src/include/port/win32  -I/c/progra~1/openssl-win64/include "-I/home/pgrunner/bf/root/REL_15_STABLE/pgsql.build/../pgsql/src/include/port/win32" -DWIN32_STACK_RLIMIT=4194304 -I../../../src/port -I/home/pgrunner/bf/root/REL_15_STABLE/pgsql.build/../pgsql/src/port -DSO_MAJOR_VERSION=5  -c -o fe-secure-common.o /home/pgrunner/bf/root/REL_15_STABLE/pgsql.build/../pgsql/src/interfaces/libpq/fe-secure-common.c
C:/tools/xmsys64/home/pgrunner/bf/root/REL_15_STABLE/pgsql/src/interfaces/libpq/fe-secure-common.c: In function 'pq_verify_peer_name_matches_certificate_ip':
C:/tools/xmsys64/home/pgrunner/bf/root/REL_15_STABLE/pgsql/src/interfaces/libpq/fe-secure-common.c:219:21: error: implicit declaration of function 'inet_pton'; did you mean 'inet_aton'? [-Wimplicit-function-declaration]  219 |                 if (inet_pton(AF_INET6, host, &addr) == 1)      |                     ^~~~~~~~~      |                     inet_aton
make[3]: *** [<builtin>: fe-secure-common.o] Error 1

configure has determined that we have inet_pton, and I have repeated the test manually. It's not a ccache issue - I have cleared the cache and the problem persists. The test run by meson on the same animal reports not finding the function.

So I'm a bit flummoxed about how to fix this, and would appreciate any suggestions.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: msys inet_pton strangeness

From
Tom Lane
Date:
Andrew Dunstan <andrew@dunslane.net> writes:
> It's complaining like this:

> C:/tools/xmsys64/home/pgrunner/bf/root/REL_15_STABLE/pgsql/src/interfaces/libpq/fe-secure-common.c:219:21: error:
implicitdeclaration of function 'inet_pton'; did you mean 'inet_aton'? [-Wimplicit-function-declaration] 
>    219 |                 if (inet_pton(AF_INET6, host, &addr) == 1)
>        |                     ^~~~~~~~~

> configure has determined that we have inet_pton, and I have repeated the
> test manually.

configure's test is purely a linker test.  It does not check to see
where/whether the function is declared.  Meanwhile, the compiler is
complaining that it doesn't see a declaration.  So the problem
probably can be fixed by adding an #include, but you'll need to
figure out what.

I see that our other user of inet_pton, fe-secure-openssl.c,
has a rather different #include setup than fe-secure-common.c;
does it compile OK?

            regards, tom lane



Re: msys inet_pton strangeness

From
Andrew Dunstan
Date:


On 2024-09-28 Sa 11:49 AM, Tom Lane wrote:
Andrew Dunstan <andrew@dunslane.net> writes:
It's complaining like this:
C:/tools/xmsys64/home/pgrunner/bf/root/REL_15_STABLE/pgsql/src/interfaces/libpq/fe-secure-common.c:219:21: error: implicit declaration of function 'inet_pton'; did you mean 'inet_aton'? [-Wimplicit-function-declaration]   219 |                 if (inet_pton(AF_INET6, host, &addr) == 1)       |                     ^~~~~~~~~
configure has determined that we have inet_pton, and I have repeated the 
test manually.
configure's test is purely a linker test.  It does not check to see
where/whether the function is declared.  Meanwhile, the compiler is
complaining that it doesn't see a declaration.  So the problem
probably can be fixed by adding an #include, but you'll need to
figure out what.

I see that our other user of inet_pton, fe-secure-openssl.c,
has a rather different #include setup than fe-secure-common.c;
does it compile OK?


I'll try, but this error occurs before we get that far.

We should have included ws2tcpip.h, which includes this:

#define InetPtonA inet_pton
WINSOCK_API_LINKAGE INT WSAAPI InetPtonA(INT Family, LPCSTR pStringBuf, PVOID pAddr);

It's conditioned on (_WIN32_WINNT >= 0x0600), but that should be true.

So I'm still very confused ;-(


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: msys inet_pton strangeness

From
Tom Lane
Date:
Andrew Dunstan <andrew@dunslane.net> writes:
> We should have included ws2tcpip.h, which includes this:

>     #define InetPtonA inet_pton
>     WINSOCK_API_LINKAGE INT WSAAPI InetPtonA(INT Family, LPCSTR pStringBuf, PVOID pAddr);

> It's conditioned on (_WIN32_WINNT >= 0x0600), but that should be true.

> So I'm still very confused ;-(

Me too.  Does this compiler support the equivalent of -E, so
that you can verify that the InetPtonA declaration is being
read?

            regards, tom lane



Re: msys inet_pton strangeness

From
Thomas Munro
Date:
On Sun, Sep 29, 2024 at 6:26 AM Andrew Dunstan <andrew@dunslane.net> wrote:
> We should have included ws2tcpip.h, which includes this:
>
> #define InetPtonA inet_pton
> WINSOCK_API_LINKAGE INT WSAAPI InetPtonA(INT Family, LPCSTR pStringBuf, PVOID pAddr);
>
> It's conditioned on (_WIN32_WINNT >= 0x0600), but that should be true.

Can you print out the value to be sure?  I can't imagine they'd set it
lower themselves or make it go backwards in an upgrade, but perhaps
it's somehow not being set at all, and then we do:

#if defined(_MSC_VER) && _MSC_VER >= 1900
#define MIN_WINNT 0x0600
#else
#define MIN_WINNT 0x0501
#endif

In 16 we don't do that anymore, we just always set it to 0x0A00
(commit 495ed0ef2d72).  And before 15, we didn't want that function
yet (commit c1932e542863).



Re: msys inet_pton strangeness

From
Alexander Lakhin
Date:
Hello Thomas and Andrew,

28.09.2024 23:52, Thomas Munro wrote:
> On Sun, Sep 29, 2024 at 6:26 AM Andrew Dunstan <andrew@dunslane.net> wrote:
>> We should have included ws2tcpip.h, which includes this:
>>
>> #define InetPtonA inet_pton
>> WINSOCK_API_LINKAGE INT WSAAPI InetPtonA(INT Family, LPCSTR pStringBuf, PVOID pAddr);
>>
>> It's conditioned on (_WIN32_WINNT >= 0x0600), but that should be true.
> Can you print out the value to be sure?  I can't imagine they'd set it
> lower themselves or make it go backwards in an upgrade, but perhaps
> it's somehow not being set at all, and then we do:
>
> #if defined(_MSC_VER) && _MSC_VER >= 1900
> #define MIN_WINNT 0x0600
> #else
> #define MIN_WINNT 0x0501
> #endif
>
> In 16 we don't do that anymore, we just always set it to 0x0A00
> (commit 495ed0ef2d72).  And before 15, we didn't want that function
> yet (commit c1932e542863).

FWIW, I'm observing the same here.
For a trivial test.c (compiled with the same command line as
fe-secure-common.c) like:
"===_WIN32"
_WIN32;
"===_WIN32_WINNT";
_WIN32_WINNT;

with gcc -E (from mingw-w64-ucrt-x86_64-gcc 14.2.0-1), I get:
"===_WIN32"
1;
"===_WIN32_WINNT";
_WIN32_WINNT;

That is, _WIN32_WINNT is not defined, but with #include <windows.h> above,
I see:
"===_WIN32_WINNT";
0x603

With #include "postgres_fe.h" (as in fe-secure-common.c) I get:
"===_WIN32_WINNT";
0x0501;

Best regards,
Alexander



Re: msys inet_pton strangeness

From
Andrew Dunstan
Date:


On 2024-09-29 Su 1:00 AM, Alexander Lakhin wrote:
Hello Thomas and Andrew,

28.09.2024 23:52, Thomas Munro wrote:
On Sun, Sep 29, 2024 at 6:26 AM Andrew Dunstan <andrew@dunslane.net> wrote:
We should have included ws2tcpip.h, which includes this:

#define InetPtonA inet_pton
WINSOCK_API_LINKAGE INT WSAAPI InetPtonA(INT Family, LPCSTR pStringBuf, PVOID pAddr);

It's conditioned on (_WIN32_WINNT >= 0x0600), but that should be true.
Can you print out the value to be sure?  I can't imagine they'd set it
lower themselves or make it go backwards in an upgrade, but perhaps
it's somehow not being set at all, and then we do:

#if defined(_MSC_VER) && _MSC_VER >= 1900
#define MIN_WINNT 0x0600
#else
#define MIN_WINNT 0x0501
#endif

In 16 we don't do that anymore, we just always set it to 0x0A00
(commit 495ed0ef2d72).  And before 15, we didn't want that function
yet (commit c1932e542863).

FWIW, I'm observing the same here.
For a trivial test.c (compiled with the same command line as
fe-secure-common.c) like:
"===_WIN32"
_WIN32;
"===_WIN32_WINNT";
_WIN32_WINNT;

with gcc -E (from mingw-w64-ucrt-x86_64-gcc 14.2.0-1), I get:
"===_WIN32"
1;
"===_WIN32_WINNT";
_WIN32_WINNT;

That is, _WIN32_WINNT is not defined, but with #include <windows.h> above,
I see:
"===_WIN32_WINNT";
0x603

With #include "postgres_fe.h" (as in fe-secure-common.c) I get:
"===_WIN32_WINNT";
0x0501;





Yeah, src/include/port/win32/sys/socket.h has:

#include <winsock2.h>
#include <ws2tcpip.h>
#include <windows.h>

I'm inclined to think we might need to reverse the order of the last two. TBH I don't really understand how this has worked up to now.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: msys inet_pton strangeness

From
Tom Lane
Date:
Andrew Dunstan <andrew@dunslane.net> writes:
> Yeah, src/include/port/win32/sys/socket.h has:

>     #include <winsock2.h>
>     #include <ws2tcpip.h>
>     #include <windows.h>

> I'm inclined to think we might need to reverse the order of the last 
> two. TBH I don't really understand how this has worked up to now.

I see the same in src/include/port/win32_port.h ... wouldn't that
get included first?

            regards, tom lane



Re: msys inet_pton strangeness

From
Alexander Lakhin
Date:
29.09.2024 18:47, Andrew Dunstan wrote:
Yeah, src/include/port/win32/sys/socket.h has:
#include <winsock2.h>
#include <ws2tcpip.h>
#include <windows.h>

I'm inclined to think we might need to reverse the order of the last two. TBH I don't really understand how this has worked up to now.


As far as I can see, in my environment  _WIN32_WINNT defined with
#ifndef _WIN32_WINNT
#define _WIN32_WINNT 0x603
#endif

inside C:/msys64/ucrt64/include/_mingw.h, which can be included
                 from C:/msys64/ucrt64/include/corecrt.h:10,
                 from C:/msys64/ucrt64/include/crtdefs.h:10,
                 from ../../../src/include/pg_config_os.h:40,
                 from ../../../src/include/c.h:56,
                 from ../../../src/include/postgres_fe.h:25,
                 from fe-secure-common.c:20

or (if HAVE_CRTDEFS_H is not defined):
                 from C:/msys64/ucrt64/include/corecrt.h:10,
                 from C:/msys64/ucrt64/include/corecrt_stdio_config.h:10,
                 from C:/msys64/ucrt64/include/stdio.h:9,
                 from ../../../src/include/c.h:59,
                 from ../../../src/include/postgres_fe.h:25,
                 from fe-secure-common.c:20

or (if winsock2.h included directly):
                 from C:/msys64/ucrt64/include/windows.h:9,
                 from C:/msys64/ucrt64/include/winsock2.h:23

so including winsock2.h is sufficient to include _mingw.h, but it doesn't
redefine _WIN32_WINNT, unfortunately.

Best regards,
Alexander

Re: msys inet_pton strangeness

From
Thomas Munro
Date:
Just an idea...

--- a/src/include/port/win32.h
+++ b/src/include/port/win32.h
@@ -16,7 +16,7 @@
  * get support for GetLocaleInfoEx() with locales. For everything else
  * the minimum version is Windows XP (0x0501).
  */
-#if defined(_MSC_VER) && _MSC_VER >= 1900
+#if !defined(_MSC_VER) || _MSC_VER >= 1900
 #define MIN_WINNT 0x0600
 #else
 #define MIN_WINNT 0x0501

That was done to reveal the Vista locale stuff, which MingGW certainly
has: we're calling it unconditionally in the master branch in at least
one place (and we should do more of that, to make MSVC and MinGW code
paths the same wherever possible).  In 15 the users of
GetLocaleInfoEx() are guarded with checks that you're on MSVC so it
still wouldn't actually call them anyway.

Obviously it's not good to change the target in the back branches.
But apparently it already changed by accident due to some header order
nonsense (could it be related to MinGW's recent switch to the UCRT by
default?), so changing it again so that it compiles seems OK?  We
don't seem to have a documented MinGW support range, and I've always
sort of assumed that it's just 'recent versions only' because it's
effectively only for developers (cross builds and suchlike).  And it
certainly didn't really intend to be runnable on Windows XP
(PostgreSQL 11 was the last to claim to run on Windows XP (0x0501)).
I doubt anyone's actually going to test this on Vista or other ancient
SDKs either, which is why I was looking for a change that *only*
affects MinGW and doesn't risk changining anything for MSVC users on
the retro-computers and toolchains we claim to support.  For example,
header order dependencies and side effects are a little chaotic on
that OS, so you could easily break something else...

I guess the objection would be that (apparently) some translation
units are being compiled with 0x0603 from system headers, and this one
would use 0x0600, which might be confusing.



Re: msys inet_pton strangeness

From
Alexander Lakhin
Date:
Hello Andrew and Thomas,

29.09.2024 18:47, Andrew Dunstan пишет:
>
> I'm inclined to think we might need to reverse the order of the last two. TBH I don't really understand how this has

> worked up to now.
>

I've looked at the last successful run [1] and discovered that
fe-secure-common.c didn't compile cleanly too:
ccache gcc ... /home/pgrunner/bf/root/REL_15_STABLE/pgsql.build/../pgsql/src/interfaces/libpq/fe-secure-common.c
C:/tools/nmsys64/home/pgrunner/bf/root/REL_15_STABLE/pgsql/src/interfaces/libpq/fe-secure-common.c: In function 
'pq_verify_peer_name_matches_certificate_ip':
C:/tools/nmsys64/home/pgrunner/bf/root/REL_15_STABLE/pgsql/src/interfaces/libpq/fe-secure-common.c:219:21: warning: 
implicit declaration of function 'inet_pton'; did you mean 'inet_aton'? [-Wimplicit-function-declaration]
   219 |                 if (inet_pton(AF_INET6, host, &addr) == 1)
       |                     ^~~~~~~~~
       |                     inet_aton

So it worked just because that missing declaration generated just a
warning, not an error.

30.09.2024 01:28, Thomas Munro wrote:
> Just an idea...
>
> --- a/src/include/port/win32.h
> +++ b/src/include/port/win32.h
> @@ -16,7 +16,7 @@
>    * get support for GetLocaleInfoEx() with locales. For everything else
>    * the minimum version is Windows XP (0x0501).
>    */
> -#if defined(_MSC_VER) && _MSC_VER >= 1900
> +#if !defined(_MSC_VER) || _MSC_VER >= 1900
>   #define MIN_WINNT 0x0600
>   #else
>   #define MIN_WINNT 0x0501

This change works for me in the msys case. I have no VS 2013 on hand to
test the other branch, but it looks like HAVE_INET_PTON set to 1
unconditionally in src/tools/msvc/Solution.pm, so we probably will stumble
upon the same issue with _MSC_VER = 1800. What if we just set
MIN_WINNT 0x0600 for REL_15_STABLE? Or may be it would make sense to get
that old Visual Studio and recheck?

The other question that I still have is: where we expect to get system
_WIN32_WINNT from? As far as I can see, in the fe-secure-common.c case we
have the following include chain:
#include "postgres_fe.h"
     #include "c.h" // no other includes above
         #include "postgres_ext.h"
             #include "pg_config_ext.h"
             ...
             #include "pg_config.h"
             #include "pg_config_manual.h"    /* must be after pg_config.h */
             #include "pg_config_os.h"        /* must be before any system header files */
                 // checks _WIN32_WINNT:
                 #if defined(_WIN32_WINNT) && _WIN32_WINNT < MIN_WINNT

So if pg_config_os.h is really included before any system headers,
checking _WIN32_WINNT makes sense only when that define passed with
-D_WIN32_WINNT, no?

[1] https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=fairywren&dt=2024-09-19%2023%3A10%3A10&stg=build

Best regards,
Alexander



Re: msys inet_pton strangeness

From
Andrew Dunstan
Date:
On 2024-09-30 Mo 7:00 AM, Alexander Lakhin wrote:
> Hello Andrew and Thomas,
>
> 29.09.2024 18:47, Andrew Dunstan пишет:
>>
>> I'm inclined to think we might need to reverse the order of the last 
>> two. TBH I don't really understand how this has worked up to now.
>>
>
> I've looked at the last successful run [1] and discovered that
> fe-secure-common.c didn't compile cleanly too:
> ccache gcc ... 
> /home/pgrunner/bf/root/REL_15_STABLE/pgsql.build/../pgsql/src/interfaces/libpq/fe-secure-common.c
> C:/tools/nmsys64/home/pgrunner/bf/root/REL_15_STABLE/pgsql/src/interfaces/libpq/fe-secure-common.c: 
> In function 'pq_verify_peer_name_matches_certificate_ip':
> C:/tools/nmsys64/home/pgrunner/bf/root/REL_15_STABLE/pgsql/src/interfaces/libpq/fe-secure-common.c:219:21: 
> warning: implicit declaration of function 'inet_pton'; did you mean 
> 'inet_aton'? [-Wimplicit-function-declaration]
>   219 |                 if (inet_pton(AF_INET6, host, &addr) == 1)
>       |                     ^~~~~~~~~
>       |                     inet_aton
>
> So it worked just because that missing declaration generated just a
> warning, not an error.



Ah, so this is because gcc 14.1.0 treats this as an error but gcc 12.2.0 
treats it as a warning. Now it makes sense.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com




Re: msys inet_pton strangeness

From
Andrew Dunstan
Date:
On 2024-09-29 Su 6:28 PM, Thomas Munro wrote:
> Just an idea...
>
> --- a/src/include/port/win32.h
> +++ b/src/include/port/win32.h
> @@ -16,7 +16,7 @@
>    * get support for GetLocaleInfoEx() with locales. For everything else
>    * the minimum version is Windows XP (0x0501).
>    */
> -#if defined(_MSC_VER) && _MSC_VER >= 1900
> +#if !defined(_MSC_VER) || _MSC_VER >= 1900
>   #define MIN_WINNT 0x0600
>   #else
>   #define MIN_WINNT 0x0501


This seems reasonable as just about the most minimal change we can make 
work.


cheers


andrew



--
Andrew Dunstan
EDB: https://www.enterprisedb.com




Re: msys inet_pton strangeness

From
Tom Lane
Date:
Andrew Dunstan <andrew@dunslane.net> writes:
> Ah, so this is because gcc 14.1.0 treats this as an error but gcc 12.2.0 
> treats it as a warning. Now it makes sense.

Not entirely ... if fairywren had been generating that warning all
along, I would have noticed it long ago, because I periodically
scrape the BF database for compiler warnings.  There has to have
been some recent change in the system include files.

            regards, tom lane



Re: msys inet_pton strangeness

From
Andrew Dunstan
Date:
On 2024-09-30 Mo 10:08 AM, Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
>> Ah, so this is because gcc 14.1.0 treats this as an error but gcc 12.2.0
>> treats it as a warning. Now it makes sense.
> Not entirely ... if fairywren had been generating that warning all
> along, I would have noticed it long ago, because I periodically
> scrape the BF database for compiler warnings.  There has to have
> been some recent change in the system include files.


here's what I see on vendikar:


pgbfprod=> select min(snapshot) from build_status_log where log_stage in 
('build.log', 'make.log') and branch = 'REL_15_STABLE' and sysname = 
'fairywren' and snapshot > now() - interval '1500 days' and log_text ~ 
'inet_pton';
          min
---------------------
  2022-06-30 18:04:08
(1 row)


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com




Re: msys inet_pton strangeness

From
Tom Lane
Date:
Andrew Dunstan <andrew@dunslane.net> writes:
> On 2024-09-30 Mo 10:08 AM, Tom Lane wrote:
>> Not entirely ... if fairywren had been generating that warning all
>> along, I would have noticed it long ago, because I periodically
>> scrape the BF database for compiler warnings.  There has to have
>> been some recent change in the system include files.

> here's what I see on vendikar:

Oh, wait, I forgot this is only about the v15 branch.  I seldom
search for warnings except on HEAD.  Still, I'd have expected to
notice it while v15 was development tip.  Maybe we changed something
since then?

Anyway, it's pretty moot, I see no reason not to push forward
with the proposed fix.

            regards, tom lane



Re: msys inet_pton strangeness

From
Andrew Dunstan
Date:


On 2024-09-30 Mo 11:11 AM, Tom Lane wrote:
Andrew Dunstan <andrew@dunslane.net> writes:
On 2024-09-30 Mo 10:08 AM, Tom Lane wrote:
Not entirely ... if fairywren had been generating that warning all
along, I would have noticed it long ago, because I periodically
scrape the BF database for compiler warnings.  There has to have
been some recent change in the system include files.
here's what I see on vendikar:
Oh, wait, I forgot this is only about the v15 branch.  I seldom
search for warnings except on HEAD.  Still, I'd have expected to
notice it while v15 was development tip.  Maybe we changed something
since then?

Anyway, it's pretty moot, I see no reason not to push forward
with the proposed fix.
			


Thanks, done.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com