msys inet_pton strangeness

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?

regards, tom lane

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

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

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

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

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

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

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

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.

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]https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=fairywren&amp;dt=2024-09-19%2023%3A10%3A10&amp;stg=build 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&amp;dt=2024-09-19%2023%3A10%3A10&amp;stg=build

Best regards,
Alexander

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

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

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

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

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

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