A few warnings on Windows

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

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?

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

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

LGTM, 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

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&gt;, 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&gt;, 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

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&gt;, 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&gt;, 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&gt;, 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