Thread: 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 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
Attachment
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
Attachment
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]. 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] https://www.postgresql.org/message-id/30536376-cb57-d233-12d4-a5d70d0349ce%402ndquadrant.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
Attachment
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
Attachment
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
Attachment
* 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