A few warnings on Windows
Hi,
Not sure if it's a goal to be warning free on Windows, but I was
testing some patches on that fine operating system and spotted a
couple of warnings that seemed worth asking about:
src/backend/replication/basebackup.c(1470): warning C4146: unary minus
operator applied to unsigned type, result still unsigned
Yeah, we have: if (fseek(fp, -(cnt - BLCKSZ * i), SEEK_CUR) == -1)
... where cnt is size_t. Perhaps we should use (or cast to) off_t?
src/bin/pgbench/pgbench.c(971): warning C4307: '*' : integral constant
overflow [C:\buildfarm\buildenv\HEAD\pgsql.build\pgbench.vcxproj]
We have: uint64 result = seed ^ (sizeof(int64) * MM2_MUL);
... where MM2_MUL is a UINT64CONST. I checked the upstream source of
this code and it's using a runtime multiplicand while here it's a
constant so the compiler sees the overflow. I suppose we could make
the warning go away by just defining a constant (which I make out to
be 0x35253c9ade8f4ca8).
C:\Program Files (x86)\Microsoft Visual Studio
12.0\VC\include\stdbool.h(11): warning C4005: 'false' : macro
redefinition [C:\buildfarm\buildenv\HEAD\pgsql.build\jsonb_plperl.vcxproj]
c:\buildfarm\buildenv\head\pgsql.build\src\include\c.h(283)
: see previous definition of 'false'
C:\Program Files (x86)\Microsoft Visual Studio
12.0\VC\include\stdbool.h(12): warning C4005: 'true' : macro
redefinition [C:\buildfarm\buildenv\HEAD\pgsql.build\jsonb_plperl.vcxproj]
c:\buildfarm\buildenv\head\pgsql.build\src\include\c.h(279)
: see previous definition of 'true'
Somehow when building jsonb_plperl and plperl we get our true/false
macros tangled up with the ones from stdbool.h on this platform. Not
sure if this was known/expected.
There are also a bunch of other macro redefinition warnings but I
suspect those are expected (open, unlink etc).
--
Thomas Munro
http://www.enterprisedb.com
On Tue, May 01, 2018 at 05:40:18PM +1200, Thomas Munro wrote:
src/backend/replication/basebackup.c(1470): warning C4146: unary minus
operator applied to unsigned type, result still unsignedYeah, we have: if (fseek(fp, -(cnt - BLCKSZ * i), SEEK_CUR) == -1)
... where cnt is size_t. Perhaps we should use (or cast to) off_t?
Sounds sensible.
src/bin/pgbench/pgbench.c(971): warning C4307: '*' : integral constant
overflow [C:\buildfarm\buildenv\HEAD\pgsql.build\pgbench.vcxproj]We have: uint64 result = seed ^ (sizeof(int64) * MM2_MUL);
... where MM2_MUL is a UINT64CONST. I checked the upstream source of
this code and it's using a runtime multiplicand while here it's a
constant so the compiler sees the overflow. I suppose we could make
the warning go away by just defining a constant (which I make out to
be 0x35253c9ade8f4ca8).
Or just enforce it with some casts?
C:\Program Files (x86)\Microsoft Visual Studio
12.0\VC\include\stdbool.h(11): warning C4005: 'false' : macro
redefinition [C:\buildfarm\buildenv\HEAD\pgsql.build\jsonb_plperl.vcxproj]
c:\buildfarm\buildenv\head\pgsql.build\src\include\c.h(283)
: see previous definition of 'false'
C:\Program Files (x86)\Microsoft Visual Studio
12.0\VC\include\stdbool.h(12): warning C4005: 'true' : macro
redefinition [C:\buildfarm\buildenv\HEAD\pgsql.build\jsonb_plperl.vcxproj]
c:\buildfarm\buildenv\head\pgsql.build\src\include\c.h(279)
: see previous definition of 'true'
Those are caused by the interactions of stdbool.h from MSVC and the
definitions from c.h. I am pretty sure that there is an unnecessary
double-inclusion happening in the perl scripts of src/tools/msvc, but I
did not take the time to dig into it, and it happens that they are
actually harmless if you look at what msvc ships as definition of true
and false.
--
Michael
Michael Paquier <michael@paquier.xyz> writes:
On Tue, May 01, 2018 at 05:40:18PM +1200, Thomas Munro wrote:
src/backend/replication/basebackup.c(1470): warning C4146: unary minus
operator applied to unsigned type, result still unsigned
... where cnt is size_t. Perhaps we should use (or cast to) off_t?
Sounds sensible.
+1.
We have: uint64 result = seed ^ (sizeof(int64) * MM2_MUL);
... where MM2_MUL is a UINT64CONST. I checked the upstream source of
this code and it's using a runtime multiplicand while here it's a
constant so the compiler sees the overflow. I suppose we could make
the warning go away by just defining a constant (which I make out to
be 0x35253c9ade8f4ca8).
Or just enforce it with some casts?
I'm not sure we could silence the warning with casts. Thomas' proposal
of a hand-computed constant seems reasonable.
C:\Program Files (x86)\Microsoft Visual Studio
12.0\VC\include\stdbool.h(11): warning C4005: 'false' : macro
redefinition [C:\buildfarm\buildenv\HEAD\pgsql.build\jsonb_plperl.vcxproj]
c:\buildfarm\buildenv\head\pgsql.build\src\include\c.h(283)
: see previous definition of 'false'
C:\Program Files (x86)\Microsoft Visual Studio
12.0\VC\include\stdbool.h(12): warning C4005: 'true' : macro
redefinition [C:\buildfarm\buildenv\HEAD\pgsql.build\jsonb_plperl.vcxproj]
c:\buildfarm\buildenv\head\pgsql.build\src\include\c.h(279)
: see previous definition of 'true'
Those are caused by the interactions of stdbool.h from MSVC and the
definitions from c.h.
Yeah. In the wake of Peter's changes to use <stdbool.h> on other
platforms, should we be enabling HAVE_STDBOOL_H for Windows?
On more or less the same topic, I just scraped all the compiler warnings
for HEAD from the buildfarm database, and there seem to be a few other
things worth cleaning up. One that I'm looking at is that recent gcc
has a -Wimplicit-fallthrough warning for switch branches not separated
by a "break" or similar. It can be silenced with a comment similar to
/* FALLTHROUGH */, but we have not been entirely consistent about
providing such comments. I'm inclined to run around and fix those
omissions. Perhaps at some point we should have configure turn that
warning on if available?
regards, tom lane
On Wed, May 2, 2018 at 8:48 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Yeah. In the wake of Peter's changes to use <stdbool.h> on other
platforms, should we be enabling HAVE_STDBOOL_H for Windows?
It seems that header arrived in VC 2013. I will find the conditional
macrology for that.
https://blogs.msdn.microsoft.com/vcblog/2013/07/19/c99-library-support-in-visual-studio-2013/
--
Thomas Munro
http://www.enterprisedb.com
On Wed, May 2, 2018 at 9:29 AM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:
On Wed, May 2, 2018 at 8:48 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Yeah. In the wake of Peter's changes to use <stdbool.h> on other
platforms, should we be enabling HAVE_STDBOOL_H for Windows?It seems that header arrived in VC 2013. I will find the conditional
macrology for that.https://blogs.msdn.microsoft.com/vcblog/2013/07/19/c99-library-support-in-visual-studio-2013/
Here's a patch that builds warning-free for me. Result:
https://ci.appveyor.com/project/macdice/postgres/build/1.0.139
Unfortunately my scripting for that doesn't actually build the plperl
stuff yet (need to cannibalise more buildfarm scripts...) so I can't
confirm that it'll fix the true/false redefinition warnings visible on
whelk (VC 2013) and dory (2015) but not hamerkop (2005), thrips
(2010), bowerbird (2012). It seems likely.
--
Thomas Munro
http://www.enterprisedb.com
Attachments:
0001-Fix-some-assorted-compiler-warnings-on-Windows.patchapplication/octet-stream; name=0001-Fix-some-assorted-compiler-warnings-on-Windows.patchDownload
From e78233003918294ec42094404efbc0f6909af34a Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.munro@enterprisedb.com>
Date: Wed, 2 May 2018 09:42:14 +1200
Subject: [PATCH] Fix some assorted compiler warnings on Windows.
Don't overflow the result type of constant expressions. Don't negate unsigned
types. Define HAVE_STDBOOL_H for Visual C++ 2013 and later.
Thomas Munro
Reviewed-By: Michael Paquier and Tom Lane
Discussion: https://postgr.es/m/CAEepm%3D3%3DTDYEXUEcHpEx%2BTwc31wo7PA0oBAiNt6sWmq93MW02A%40mail.gmail.com
---
src/backend/replication/basebackup.c | 2 +-
src/bin/pgbench/pgbench.c | 3 ++-
src/include/pg_config.h.win32 | 4 +++-
3 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/src/backend/replication/basebackup.c b/src/backend/replication/basebackup.c
index bc9f585b85b..5688cbe2e9a 100644
--- a/src/backend/replication/basebackup.c
+++ b/src/backend/replication/basebackup.c
@@ -1361,7 +1361,7 @@ sendFile(const char *readfilename, const char *tarfilename, struct stat *statbuf
char buf[TAR_SEND_SIZE];
uint16 checksum;
int checksum_failures = 0;
- size_t cnt;
+ off_t cnt;
int i;
pgoff_t len = 0;
char *page;
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 78b8f1706ca..930fe70e532 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -67,6 +67,7 @@
#define FNV_PRIME UINT64CONST(0x100000001b3)
#define FNV_OFFSET_BASIS UINT64CONST(0xcbf29ce484222325)
#define MM2_MUL UINT64CONST(0xc6a4a7935bd1e995)
+#define MM2_MUL_TIMES_8 UINT64CONST(0x35253c9ade8f4ca8)
#define MM2_ROT 47
/*
@@ -968,7 +969,7 @@ getHashFnv1a(int64 val, uint64 seed)
static int64
getHashMurmur2(int64 val, uint64 seed)
{
- uint64 result = seed ^ (sizeof(int64) * MM2_MUL);
+ uint64 result = seed ^ MM2_MUL_TIMES_8; /* sizeof(int64) */
uint64 k = (uint64) val;
k *= MM2_MUL;
diff --git a/src/include/pg_config.h.win32 b/src/include/pg_config.h.win32
index bc437b08f0c..e776b34f223 100644
--- a/src/include/pg_config.h.win32
+++ b/src/include/pg_config.h.win32
@@ -357,7 +357,9 @@
#define HAVE_SSL_GET_CURRENT_COMPRESSION 1
/* Define to 1 if stdbool.h conforms to C99. */
-/* #undef HAVE_STDBOOL_H */
+#if (_MSC_VER >= 1800)
+#define HAVE_STDBOOL_H 1
+#endif
/* Define to 1 if you have the <stdint.h> header file. */
/* #undef HAVE_STDINT_H */
--
2.17.0
Thomas Munro <thomas.munro@enterprisedb.com> writes:
Here's a patch that builds warning-free for me. Result:
https://ci.appveyor.com/project/macdice/postgres/build/1.0.139
LGTM, pushed.
Unfortunately my scripting for that doesn't actually build the plperl
stuff yet (need to cannibalise more buildfarm scripts...) so I can't
confirm that it'll fix the true/false redefinition warnings visible on
whelk (VC 2013) and dory (2015) but not hamerkop (2005), thrips
(2010), bowerbird (2012). It seems likely.
We'll soon find out.
regards, tom lane
On Wed, May 2, 2018 at 11:39 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Thomas Munro <thomas.munro@enterprisedb.com> writes:
Here's a patch that builds warning-free for me. Result:
https://ci.appveyor.com/project/macdice/postgres/build/1.0.139LGTM, pushed.
Thanks. The first two warnings I mentioned are fixed.
Unfortunately my scripting for that doesn't actually build the plperl
stuff yet (need to cannibalise more buildfarm scripts...) so I can't
confirm that it'll fix the true/false redefinition warnings visible on
whelk (VC 2013) and dory (2015) but not hamerkop (2005), thrips
(2010), bowerbird (2012). It seems likely.We'll soon find out.
Nope -- and I think that's because we only actually use stdbool.h
instead of our own macros if we think sizeof(bool) is exactly 1. But
we don't because pg_config.h.win32 says:
#define SIZEOF_BOOL 0
Perhaps that's what Peter E meant when he said "Windows could use some
manual adjustments in pg_config.h.win32 if anyone
cares"[1]/messages/by-id/30536376-cb57-d233-12d4-a5d70d0349ce@2ndquadrant.com. Should we just change this to 1? I'm going to go and test
that now. From googling sizeof(bool) am aware that ancient VC (before
5.0 more than 20 years ago) had a header that defined bool as int, but
that seems irrelevant now, right?
[1]: /messages/by-id/30536376-cb57-d233-12d4-a5d70d0349ce@2ndquadrant.com
--
Thomas Munro
http://www.enterprisedb.com
On Wed, May 2, 2018 at 12:51 PM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:
On Wed, May 2, 2018 at 11:39 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
We'll soon find out.
Nope -- and I think that's because we only actually use stdbool.h
instead of our own macros if we think sizeof(bool) is exactly 1. But
we don't because pg_config.h.win32 says:#define SIZEOF_BOOL 0
Perhaps that's what Peter E meant when he said "Windows could use some
manual adjustments in pg_config.h.win32 if anyone
cares"[1]. Should we just change this to 1? I'm going to go and test
that now. From googling sizeof(bool) am aware that ancient VC (before
5.0 more than 20 years ago) had a header that defined bool as int, but
that seems irrelevant now, right?
That compiles and runs the main checks (except tablespace which I
suppress) cleanly for me and I assume it really is using
stdbool.h this time. Hopefully plperl will be happier this way.
Since my earlier test, a new entirely independent warning arrived with
commit 41c912ca:
c:\projects\postgres\src\bin\pgbench\pgbench.c(2327): warning C4715:
'evalStandardFunc' : not all control paths return a value
[C:\projects\postgres\pgbench.vcxproj]
Patch for that attached, too.
--
Thomas Munro
http://www.enterprisedb.com
Attachments:
0001-Change-SIZEOF_BOOL-to-1-for-Windows.patchapplication/octet-stream; name=0001-Change-SIZEOF_BOOL-to-1-for-Windows.patchDownload
From 381300d881bd718f1fd92cf0e82907a050cff4f2 Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.munro@enterprisedb.com>
Date: Wed, 2 May 2018 12:54:46 +1200
Subject: [PATCH 1/2] Change SIZEOF_BOOL to 1 for Windows.
Previously it was defined as 0, which inhibited the use of stdbool.h even when
it is available.
Thomas Munro
Discussion: https://postgr.es/m/CAEepm%3D3%3DTDYEXUEcHpEx%2BTwc31wo7PA0oBAiNt6sWmq93MW02A%40mail.gmail.com
---
src/include/pg_config.h.win32 | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/include/pg_config.h.win32 b/src/include/pg_config.h.win32
index e776b34f223..9149c7ad376 100644
--- a/src/include/pg_config.h.win32
+++ b/src/include/pg_config.h.win32
@@ -615,7 +615,7 @@
#define PG_VERSION_STR "Uninitialized version string (win32)"
/* The size of `bool', as computed by sizeof. */
-#define SIZEOF_BOOL 0
+#define SIZEOF_BOOL 1
/* The size of `long', as computed by sizeof. */
#define SIZEOF_LONG 4
--
2.17.0
0002-Fix-compiler-warning-on-Windows.patchapplication/octet-stream; name=0002-Fix-compiler-warning-on-Windows.patchDownload
From 88b227bd536893a14d853087dd63579e4b13adc0 Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.munro@enterprisedb.com>
Date: Wed, 2 May 2018 13:14:51 +1200
Subject: [PATCH 2/2] Fix compiler warning on Windows.
Commit 41c912ca caused MSVC to complain that not all control paths return
a value.
Thomas Munro
Discussion: https://postgr.es/m/CAEepm%3D3%3DTDYEXUEcHpEx%2BTwc31wo7PA0oBAiNt6sWmq93MW02A%40mail.gmail.com
---
src/bin/pgbench/pgbench.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index c36556c497f..6f9b26e3d6a 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -1980,7 +1980,8 @@ evalStandardFunc(TState *thread, CState *st,
}
}
- break; /* NOTREACHED */
+ Assert(0);
+ return false; /* NOTREACHED */
}
/* integer bitwise operators */
--
2.17.0
On 5/1/18 16:48, Tom Lane wrote:
On more or less the same topic, I just scraped all the compiler warnings
for HEAD from the buildfarm database, and there seem to be a few other
things worth cleaning up. One that I'm looking at is that recent gcc
has a -Wimplicit-fallthrough warning for switch branches not separated
by a "break" or similar. It can be silenced with a comment similar to
/* FALLTHROUGH */, but we have not been entirely consistent about
providing such comments. I'm inclined to run around and fix those
omissions. Perhaps at some point we should have configure turn that
warning on if available?
I think it's useful, but I have found it a bit fickle at times.
One issue is <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79750>, which
requires this additional patch
diff --git a/src/interfaces/libpq/fe-secure.c
b/src/interfaces/libpq/fe-secure.c
index cfb77f6d85..a39d50ddcf 100644
--- a/src/interfaces/libpq/fe-secure.c
+++ b/src/interfaces/libpq/fe-secure.c
@@ -363,6 +363,7 @@ pqsecure_raw_write(PGconn *conn, const void *ptr,
size_t len)
/* FALL THRU */ #ifdef ECONNRESET
+ /* FALL THRU */
case ECONNRESET:
#endif
printfPQExpBuffer(&conn->errorMessage,
to build warning-free on some systems.
Another issue that has prevented me in the past from taking this too
seriously is requiring breaks after elog(ERROR) calls. I see you bit
the bullet and added those breaks, which is fair enough. But if gcc
ever fixes this bug
(https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79959), then as new code
gets added without it, users of older compilers will start getting
warnings. So we might have to craft that configure test to detect that
issue when that time comes.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2018-05-01 22:55:47 -0400, Peter Eisentraut wrote:
On 5/1/18 16:48, Tom Lane wrote:
On more or less the same topic, I just scraped all the compiler warnings
for HEAD from the buildfarm database, and there seem to be a few other
things worth cleaning up. One that I'm looking at is that recent gcc
has a -Wimplicit-fallthrough warning for switch branches not separated
by a "break" or similar. It can be silenced with a comment similar to
/* FALLTHROUGH */, but we have not been entirely consistent about
providing such comments. I'm inclined to run around and fix those
omissions. Perhaps at some point we should have configure turn that
warning on if available?I think it's useful, but I have found it a bit fickle at times.
One issue is <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79750>, which
requires this additional patch
I've just committed a variant of this (moving, not duplicating the comment).
Another issue that has prevented me in the past from taking this too
seriously is requiring breaks after elog(ERROR) calls. I see you bit
the bullet and added those breaks, which is fair enough. But if gcc
ever fixes this bug
(https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79959), then as new code
gets added without it, users of older compilers will start getting
warnings. So we might have to craft that configure test to detect that
issue when that time comes.
As long as we don't make those warnings errors, do we really care that
much? Also, seems easy enough to fix if necessary.
Greetings,
Andres Freund
Andres Freund <andres@anarazel.de> writes:
On 2018-05-01 22:55:47 -0400, Peter Eisentraut wrote:
On 5/1/18 16:48, Tom Lane wrote:
... Perhaps at some point we should have configure turn that
warning on if available?
I think it's useful, but I have found it a bit fickle at times.
Yeah, I noticed several behaviors that seemed like bugs or near-bugs
when I was preparing my patch. gcc 8.0.1 seems a bit inconsistent as
to what happens when there's an additional comment next to the
/* FALLTHROUGH */, for example, and it produced some warnings that didn't
seem to be happening with the gcc 7 compilers in the buildfarm. Still,
this is an ancient bug hazard in the C language, and so I think we should
make use of the warning even if we sometimes have to do slightly
surprising things to suppress it on some gcc versions.
Another issue that has prevented me in the past from taking this too
seriously is requiring breaks after elog(ERROR) calls. I see you bit
the bullet and added those breaks, which is fair enough. But if gcc
ever fixes this bug
(https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79959), then as new code
gets added without it, users of older compilers will start getting
warnings. So we might have to craft that configure test to detect that
issue when that time comes.
As long as we don't make those warnings errors, do we really care that
much? Also, seems easy enough to fix if necessary.
I think that "put a break or return after a switch case, even if the
case's code is noreturn" is effectively project style. Sure, that habit
dates from before we had compilers that could understand that elog(ERROR)
is noreturn, but nonetheless the *vast* majority of affected places
already have a "break". I only had to add a dozen or so --- and in quite
a few of those places, there were other branches of the very same switch
that already had the redundant break. So it's just inconsistent and
therefore poor style not to write it. I don't mind at all if our tools
enforce it, even if it's arguably a bug that they do.
regards, tom lane
Thomas Munro <thomas.munro@enterprisedb.com> writes:
On Wed, May 2, 2018 at 12:51 PM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:Perhaps that's what Peter E meant when he said "Windows could use some
manual adjustments in pg_config.h.win32 if anyone
cares"[1]. Should we just change this to 1? I'm going to go and test
that now. From googling sizeof(bool) am aware that ancient VC (before
5.0 more than 20 years ago) had a header that defined bool as int, but
that seems irrelevant now, right?
That compiles and runs the main checks (except tablespace which I
suppress) cleanly for me and I assume it really is using
stdbool.h this time. Hopefully plperl will be happier this way.
Pushed. I was slightly tempted to add a static assertion that
SIZEOF_BOOL == sizeof(bool), but there's not any obvious home
for such a thing, and it's probably just useless worrying anyway.
Since my earlier test, a new entirely independent warning arrived with
commit 41c912ca:
Ooops.
Patch for that attached, too.
Pushed that too, thanks.
regards, tom lane
On Wed, May 2, 2018 at 4:28 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Thomas Munro <thomas.munro@enterprisedb.com> writes:
That compiles and runs the main checks (except tablespace which I
suppress) cleanly for me and I assume it really is using
stdbool.h this time. Hopefully plperl will be happier this way.Pushed. I was slightly tempted to add a static assertion that
SIZEOF_BOOL == sizeof(bool), but there's not any obvious home
for such a thing, and it's probably just useless worrying anyway.
Thanks. Looking good on dory and woodlouse.
The only remaining warnings on those machines now come from the fact
that our port_win32.h and Perl's XSUB.h both want to define macros to
define macros for libc functions like mkdir etc. plperl.h already
seems to deal with other similar kinds of ugliness. Perhaps if
PG_NEED_PERL_XSUB_H is defined we should undefine port_win32.h's
libc-stealing macros before including it? Either way XSUB.h's
definitions win. That should get us to 0 warnings. See attached (not
tested).
--
Thomas Munro
http://www.enterprisedb.com
Attachments:
0001-Try-to-fix-compiler-warnings-in-plperl-on-Windows.patchapplication/octet-stream; name=0001-Try-to-fix-compiler-warnings-in-plperl-on-Windows.patchDownload
From d346ca621f1e9d2610b255b221bd6df3e07e799e Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.munro@enterprisedb.com>
Date: Wed, 2 May 2018 17:23:30 +1200
Subject: [PATCH] Try to fix compiler warnings in plperl on Windows.
Perl's XSUB.h header defines macros to replace libc functions. Our header
port_win32.h does something similar earlier, so XSUB.h causes compiler
warnings about macro redefinition. Undefine our macros before including
XSUB.h
Thomas Munro
Discussion: https://postgr.es/m/CAEepm%3D3%3DTDYEXUEcHpEx%2BTwc31wo7PA0oBAiNt6sWmq93MW02A%40mail.gmail.com
---
src/pl/plperl/plperl.h | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
diff --git a/src/pl/plperl/plperl.h b/src/pl/plperl/plperl.h
index c72c6ea59fc..91826ad1ba6 100644
--- a/src/pl/plperl/plperl.h
+++ b/src/pl/plperl/plperl.h
@@ -70,6 +70,27 @@
* before ppport.h, so use a #define flag to control inclusion here.
*/
#ifdef PG_NEED_PERL_XSUB_H
+/* port_win32.h defines macros that XSUB.h redefines, so avoid warnings. */
+#ifdef WIN32
+#undef accept
+#undef bind
+#undef connect
+#undef fopen
+#undef kill
+#undef listen
+#undef lstat
+#undef mkdir
+#undef open
+#undef putenv
+#undef recv
+#undef rename
+#undef select
+#undef send
+#undef socket
+#undef stat
+#undef unlink
+#undef vfprintf
+#endif
#include "XSUB.h"
#endif
--
2.17.0
Thomas Munro <thomas.munro@enterprisedb.com> writes:
The only remaining warnings on those machines now come from the fact
that our port_win32.h and Perl's XSUB.h both want to define macros to
define macros for libc functions like mkdir etc. plperl.h already
seems to deal with other similar kinds of ugliness. Perhaps if
PG_NEED_PERL_XSUB_H is defined we should undefine port_win32.h's
libc-stealing macros before including it? Either way XSUB.h's
definitions win. That should get us to 0 warnings. See attached (not
tested).
Seems reasonable to me. Pushed, we'll see what the buildfarm thinks.
regards, tom lane
On Thu, May 3, 2018 at 8:01 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Seems reasonable to me. Pushed, we'll see what the buildfarm thinks.
Build succeeded.
0 Warning(s)
0 Error(s)
Huzzah!
One more problem. whelk builds against Python 3.6 and says:
c:\users\pgbf\appdata\local\programs\python\python36-32\include\pyconfig.h(174):
warning C4142: benign redefinition of type
(src/pl/plpython/plpy_elog.c)
[C:\buildfarm\buildenv\HEAD\pgsql.build\plpython3.vcxproj]
Does anyone know what line 174 of pyconfig.h happens to say?
thrips builds against Python 3.5 and doesn't show any warnings.
--
Thomas Munro
http://www.enterprisedb.com
Thomas Munro <thomas.munro@enterprisedb.com> writes:
One more problem. whelk builds against Python 3.6 and says:
c:\users\pgbf\appdata\local\programs\python\python36-32\include\pyconfig.h(174):
warning C4142: benign redefinition of type
(src/pl/plpython/plpy_elog.c)
[C:\buildfarm\buildenv\HEAD\pgsql.build\plpython3.vcxproj]
Does anyone know what line 174 of pyconfig.h happens to say?
We'll have to ask the machine's owner I guess (cc'd here).
In the python 3.6 installation I have at hand, there's nothing
except #define's in pyconfig.h ... but the Windows version must
do things differently.
regards, tom lane
On Thu, May 03, 2018 at 09:03:15AM +1200, Thomas Munro wrote:
On Thu, May 3, 2018 at 8:01 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Build succeeded.
0 Warning(s)
0 Error(s)Huzzah!
Great work. This has been some time...
--
Michael
* Tom Lane wrote:
Thomas Munro <thomas.munro@enterprisedb.com> writes:
One more problem. whelk builds against Python 3.6 and says:
c:\users\pgbf\appdata\local\programs\python\python36-32\include\pyconfig.h(174):
warning C4142: benign redefinition of type
(src/pl/plpython/plpy_elog.c)
[C:\buildfarm\buildenv\HEAD\pgsql.build\plpython3.vcxproj]Does anyone know what line 174 of pyconfig.h happens to say?
typedef _W64 int ssize_t;
, in a "not for 64-bit" block.
<https://github.com/python/cpython/blob/v3.6.3/PC/pyconfig.h>, 3.6.3 is
the installed version on whelk.
--
Christian
Christian Ullrich <chris@chrullrich.net> writes:
Thomas Munro <thomas.munro@enterprisedb.com> writes:
Does anyone know what line 174 of pyconfig.h happens to say?
typedef _W64 int ssize_t;
, in a "not for 64-bit" block.
<https://github.com/python/cpython/blob/v3.6.3/PC/pyconfig.h>, 3.6.3 is
the installed version on whelk.
Thanks. Not a lot we're going to be able to do about silencing that
one, I'm afraid. Too bad they haven't wrapped that stanza in
"#ifndef HAVE_SSIZE_T".
regards, tom lane
On 5/3/18 10:18, Tom Lane wrote:
Christian Ullrich <chris@chrullrich.net> writes:
Thomas Munro <thomas.munro@enterprisedb.com> writes:
Does anyone know what line 174 of pyconfig.h happens to say?
typedef _W64 int ssize_t;
, in a "not for 64-bit" block.
<https://github.com/python/cpython/blob/v3.6.3/PC/pyconfig.h>, 3.6.3 is
the installed version on whelk.Thanks. Not a lot we're going to be able to do about silencing that
one, I'm afraid. Too bad they haven't wrapped that stanza in
"#ifndef HAVE_SSIZE_T".
There is still time to send a patch for Python 3.7.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, May 4, 2018 at 2:46 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
On 5/3/18 10:18, Tom Lane wrote:
Christian Ullrich <chris@chrullrich.net> writes:
Thomas Munro <thomas.munro@enterprisedb.com> writes:
Does anyone know what line 174 of pyconfig.h happens to say?
typedef _W64 int ssize_t;
, in a "not for 64-bit" block.
<https://github.com/python/cpython/blob/v3.6.3/PC/pyconfig.h>, 3.6.3 is
the installed version on whelk.Thanks. Not a lot we're going to be able to do about silencing that
one, I'm afraid. Too bad they haven't wrapped that stanza in
"#ifndef HAVE_SSIZE_T".There is still time to send a patch for Python 3.7.
Maybe we could poke this? https://bugs.python.org/issue11717
Apparently ssize_t is not defined on Windows (it's from POSIX, not C)
and various projects step on each other's toes defining it. On 64 bit
systems we both use __int64. On 32 bit systems, we chose long and
they chose int. If we changed our definition to use int too, I assume
that would fix the warnings here but might risk creating the opposite
problem somewhere else...
--
Thomas Munro
http://www.enterprisedb.com