Re: [HACKERS] pg_recvlogical.c doesn't build with --disable-integer-datetimes
Mark Dilgerwrites: >> On Feb 17, 2017, at 3:37 PM, Peter Eisentraut >> wrote: >> If your compiler isn't warning about anything with that, then there is >> something wrong with it. > $ gcc --version > Configured with: --prefix=/Applications/Xcode.app/Contents/Developer/usr > --with-gxx-include-dir=/usr/include/c++/4.2.1 > Apple LLVM version 7.3.0 (clang-703.0.31) > Target: x86_64-apple-darwin15.6.0 > Thread model: posix > InstalledDir: > /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin Interesting. That option doesn't seem to do anything on my Mac's compiler either (Apple LLVM version 8.0.0 (clang-800.0.42.1)). However, it surely spews a metric buttload of warnings with RHEL6's gcc. Points up my concern upthread about different peoples' compilers interpreting these less-common switches differently. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_recvlogical.c doesn't build with --disable-integer-datetimes
> On Feb 17, 2017, at 3:37 PM, Peter Eisentraut >wrote: > > On 2/17/17 16:13, Mark Dilger wrote: >> + PGAC_PROG_CC_CFLAGS_OPT([-Wc++-compat]) > > If your compiler isn't warning about anything with that, then there is > something wrong with it. $ gcc --version Configured with: --prefix=/Applications/Xcode.app/Contents/Developer/usr --with-gxx-include-dir=/usr/include/c++/4.2.1 Apple LLVM version 7.3.0 (clang-703.0.31) Target: x86_64-apple-darwin15.6.0 Thread model: posix InstalledDir: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_recvlogical.c doesn't build with --disable-integer-datetimes
On 2/17/17 16:13, Mark Dilger wrote: > + PGAC_PROG_CC_CFLAGS_OPT([-Wc++-compat]) If your compiler isn't warning about anything with that, then there is something wrong with it. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_recvlogical.c doesn't build with --disable-integer-datetimes
I wrote: > Right now I'd rather focus on getting to where we can have -Werror on in > the buildfarm at all. longfin says we've got work to do on that, at least > in the back branches. It may be that we can't expect near-EOL branches to > always compile perfectly cleanly on newer compilers. I found that longfin's compiler is happy with 9.4 and up as-is, and it required only a few very small and safe back-patches to make 9.3 work too. However, it's looking like -Werror with 9.2 is a bridge too far. The thing that seems untenable is that to make 9.2 compile clean, we would have to back-patch commit 71450d7fd, along with a fair amount of follow-on work, so that the compiler would understand that ereport(ERROR) doesn't return. I don't think we want to go that far --- and maybe introduce portability issues for someone's ancient compiler --- for a branch that's near EOL anyway. I'll adjust longfin's configuration so that it only applies -Werror in 9.3 and up. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_recvlogical.c doesn't build with --disable-integer-datetimes
Mark Dilgerwrites: > if test "$GCC" = yes -a "$ICC" = no; then >CFLAGS="-Wall -Wmissing-prototypes -Wpointer-arith" > + PGAC_PROG_CC_CFLAGS_OPT([-Wempty-body]) > + PGAC_PROG_CC_CFLAGS_OPT([-Wignored-qualifiers]) > + PGAC_PROG_CC_CFLAGS_OPT([-Wimplicit-fallthrough]) > + PGAC_PROG_CC_CFLAGS_OPT([-Wtype-limits]) > + PGAC_PROG_CC_CFLAGS_OPT([-Wuninitialized]) > + PGAC_PROG_CC_CFLAGS_OPT([-Wshift-negative-value]) > + PGAC_PROG_CC_CFLAGS_OPT([-Wmissing-include-dirs]) > + PGAC_PROG_CC_CFLAGS_OPT([-Wshift-overflow]) > + PGAC_PROG_CC_CFLAGS_OPT([-Wswitch-default]) > + PGAC_PROG_CC_CFLAGS_OPT([-Wdangling-else]) > + PGAC_PROG_CC_CFLAGS_OPT([-Waggregate-return]) > + PGAC_PROG_CC_CFLAGS_OPT([-Wstrict-prototypes]) > + PGAC_PROG_CC_CFLAGS_OPT([-Wmissing-declarations]) > + PGAC_PROG_CC_CFLAGS_OPT([-Wredundant-decls]) > + PGAC_PROG_CC_CFLAGS_OPT([-Winline]) > + PGAC_PROG_CC_CFLAGS_OPT([-Woverlength-strings]) > + PGAC_PROG_CC_CFLAGS_OPT([-Wc++-compat]) > + PGAC_PROG_CC_CFLAGS_OPT([-Wold-style-definition]) ># These work in some but not all gcc versions >PGAC_PROG_CC_CFLAGS_OPT([-Wdeclaration-after-statement]) >PGAC_PROG_CC_CFLAGS_OPT([-Wendif-labels]) I'm not terribly for that. For the most part, if a gcc warning isn't included by '-Wall', there's a reason for it. We could talk about whether individual extra warnings are useful for the Postgres project, but I don't want to suddenly move the goalposts to where we are enforcing a bunch of pedantic warnings that different compiler versions might not even interpret the same. I'm particularly not for turning them up in the way you suggest here, where violations would result in buildfarm failures on machines running with -Werror. Right now I'd rather focus on getting to where we can have -Werror on in the buildfarm at all. longfin says we've got work to do on that, at least in the back branches. It may be that we can't expect near-EOL branches to always compile perfectly cleanly on newer compilers. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_recvlogical.c doesn't build with --disable-integer-datetimes
On February 17, 2017 1:13:10 PM PST, Mark Dilgerwrote: >How about we add (some of) these extra warnings, plus -Werror, >in a section that is only active for platforms/compilers where we >know there aren't spurious warnings? That would make detecting >unintentionally introduced warnings simpler, without the use of >COPT. Perhaps where the compiler is GCC or CLANG, and the >platform is x86_64 redhat, something like that? Strongly against that. I do *not* want Werrror enabled during development. I have a lot of warnings enabled, but while hacking I often have some of them triggering (e.g. an unused variable or static function). Preventing me from compiling in those scenarios has no benefits. Werror it's useful in automated scenarios (e.g. my pre push script compiles with it), a lot less in interactive scenarios where you can just use make -s. Andres -- Sent from my Android device with K-9 Mail. Please excuse my brevity. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_recvlogical.c doesn't build with --disable-integer-datetimes
> On Feb 17, 2017, at 12:21 PM, Tom Lanewrote: > > Thomas Munro writes: >> On Sat, Feb 18, 2017 at 9:04 AM, Tom Lane wrote: >>> [ pokes around... ] Ah, that's called COPT, and it's entirely >>> undocumented :-(. Probably ought to fix that. > >> One way to set that up is like this: > >> $ cat src/Makefile.custom >> COPT=-Wall -Werror $(CC_OPT) Makefile.custom is deprecated per comment in src/Makefile.global, so you'd at least have to remove or edit that comment. It was enough to scare me off using Makefile.custom as a solution for this when I was working on getting -Werror into all my builds. > Well, we don't document Makefile.custom either, and probably now is > not the time to start. I'm inclined to just explain that you can > set COPT in the environment of the "make" step to add flags that > you couldn't or didn't tell configure about. There is a test in configure.in for whether the compiler is GCC, followed by additional flags if so. On my platform (Mac laptop) I was able to add additional flags against HEAD without any of them actually triggering a warning (though my last merge from HEAD was a while ago): diff --git a/configure.in b/configure.in index b9831bc..d6e7e24 100644 --- a/configure.in +++ b/configure.in @@ -446,6 +446,24 @@ fi if test "$GCC" = yes -a "$ICC" = no; then CFLAGS="-Wall -Wmissing-prototypes -Wpointer-arith" + PGAC_PROG_CC_CFLAGS_OPT([-Wempty-body]) + PGAC_PROG_CC_CFLAGS_OPT([-Wignored-qualifiers]) + PGAC_PROG_CC_CFLAGS_OPT([-Wimplicit-fallthrough]) + PGAC_PROG_CC_CFLAGS_OPT([-Wtype-limits]) + PGAC_PROG_CC_CFLAGS_OPT([-Wuninitialized]) + PGAC_PROG_CC_CFLAGS_OPT([-Wshift-negative-value]) + PGAC_PROG_CC_CFLAGS_OPT([-Wmissing-include-dirs]) + PGAC_PROG_CC_CFLAGS_OPT([-Wshift-overflow]) + PGAC_PROG_CC_CFLAGS_OPT([-Wswitch-default]) + PGAC_PROG_CC_CFLAGS_OPT([-Wdangling-else]) + PGAC_PROG_CC_CFLAGS_OPT([-Waggregate-return]) + PGAC_PROG_CC_CFLAGS_OPT([-Wstrict-prototypes]) + PGAC_PROG_CC_CFLAGS_OPT([-Wmissing-declarations]) + PGAC_PROG_CC_CFLAGS_OPT([-Wredundant-decls]) + PGAC_PROG_CC_CFLAGS_OPT([-Winline]) + PGAC_PROG_CC_CFLAGS_OPT([-Woverlength-strings]) + PGAC_PROG_CC_CFLAGS_OPT([-Wc++-compat]) + PGAC_PROG_CC_CFLAGS_OPT([-Wold-style-definition]) # These work in some but not all gcc versions PGAC_PROG_CC_CFLAGS_OPT([-Wdeclaration-after-statement]) PGAC_PROG_CC_CFLAGS_OPT([-Wendif-labels]) diff --git a/src/Makefile.global.in b/src/Makefile.global.in index d39d6ca..96fbc60 100644 --- a/src/Makefile.global.in +++ b/src/Makefile.global.in @@ -240,7 +240,7 @@ endif # not PGXS CC = @CC@ GCC = @GCC@ SUN_STUDIO_CC = @SUN_STUDIO_CC@ -CFLAGS = @CFLAGS@ +CFLAGS = @CFLAGS@ -Werror CFLAGS_VECTOR = @CFLAGS_VECTOR@ CFLAGS_SSE42 = @CFLAGS_SSE42@ How about we add (some of) these extra warnings, plus -Werror, in a section that is only active for platforms/compilers where we know there aren't spurious warnings? That would make detecting unintentionally introduced warnings simpler, without the use of COPT. Perhaps where the compiler is GCC or CLANG, and the platform is x86_64 redhat, something like that? Mark Dilger -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_recvlogical.c doesn't build with --disable-integer-datetimes
Thomas Munrowrites: > On Sat, Feb 18, 2017 at 9:04 AM, Tom Lane wrote: >> [ pokes around... ] Ah, that's called COPT, and it's entirely >> undocumented :-(. Probably ought to fix that. > One way to set that up is like this: > $ cat src/Makefile.custom > COPT=-Wall -Werror $(CC_OPT) Well, we don't document Makefile.custom either, and probably now is not the time to start. I'm inclined to just explain that you can set COPT in the environment of the "make" step to add flags that you couldn't or didn't tell configure about. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_recvlogical.c doesn't build with --disable-integer-datetimes
On Sat, Feb 18, 2017 at 9:04 AM, Tom Lanewrote: > Andres Freund writes: >> On February 17, 2017 11:40:57 AM PST, Tom Lane wrote: >>> AFAICS, if you want to build with -Werror, you have to configure >>> without that and then inject it afterwards. I wonder how people who >>> use -Werror are handling that. Is it possible to do at all in a >>> buildfarm critter? > >> Yea, I complained about that before. I think we'd have to add an extra >> configure option for it, because we rely in setting it only for certain >> tests. Personally I use CEXTRA (IIRC) to achieve that result after >> configure ran. > > [ pokes around... ] Ah, that's called COPT, and it's entirely > undocumented :-(. Probably ought to fix that. One way to set that up is like this: $ cat src/Makefile.custom COPT=-Wall -Werror $(CC_OPT) -- Thomas Munro http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_recvlogical.c doesn't build with --disable-integer-datetimes
Andres Freundwrites: > On February 17, 2017 11:40:57 AM PST, Tom Lane wrote: >> AFAICS, if you want to build with -Werror, you have to configure >> without that and then inject it afterwards. I wonder how people who >> use -Werror are handling that. Is it possible to do at all in a >> buildfarm critter? > Yea, I complained about that before. I think we'd have to add an extra > configure option for it, because we rely in setting it only for certain > tests. Personally I use CEXTRA (IIRC) to achieve that result after > configure ran. [ pokes around... ] Ah, that's called COPT, and it's entirely undocumented :-(. Probably ought to fix that. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_recvlogical.c doesn't build with --disable-integer-datetimes
> On Feb 17, 2017, at 11:40 AM, Tom Lanewrote: > > I wrote: >> Yeah. I have longfin which is running Apple's clang, and is on a machine >> that doesn't have much to do otherwise. I propose to turn on -Werror in >> its configuration, and to configure a second critter on the same hardware >> that runs with -Werror as well as --disable-integer-datetimes. Somebody >> else should do similarly with a reasonably modern/stable gcc release. > > So I tried doing that by adding -Werror to longfin's CFLAGS environment, > and it crashed and burned in configure. The most obvious problem is > that we didn't bother to supply a prototype for does_int64_work() in > the probes for 64-bit ints. However, even after fixing that, configure > produces totally bollixed output because -Werror breaks many of its > built-in tests. We could fix the int64 tests but we have no good way of > fixing the built-in tests. > > AFAICS, if you want to build with -Werror, you have to configure without > that and then inject it afterwards. I wonder how people who use -Werror > are handling that. Is it possible to do at all in a buildfarm critter? I ran into the same problem when adding -Werror, and fixed it as follows: diff --git a/src/Makefile.global.in b/src/Makefile.global.in index d39d6ca..96fbc60 100644 --- a/src/Makefile.global.in +++ b/src/Makefile.global.in @@ -240,7 +240,7 @@ endif # not PGXS CC = @CC@ GCC = @GCC@ SUN_STUDIO_CC = @SUN_STUDIO_CC@ -CFLAGS = @CFLAGS@ +CFLAGS = @CFLAGS@ -Werror CFLAGS_VECTOR = @CFLAGS_VECTOR@ CFLAGS_SSE42 = @CFLAGS_SSE42@ That adds -Werror to the compilation of sources but not to the compilation of configure test programs. I'd be happy to hear if anybody has a cleaner way of doing this. Mark Dilger -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_recvlogical.c doesn't build with --disable-integer-datetimes
On February 17, 2017 11:40:57 AM PST, Tom Lanewrote: >I wrote: >> Yeah. I have longfin which is running Apple's clang, and is on a >machine >> that doesn't have much to do otherwise. I propose to turn on -Werror >in >> its configuration, and to configure a second critter on the same >hardware >> that runs with -Werror as well as --disable-integer-datetimes. >Somebody >> else should do similarly with a reasonably modern/stable gcc release. > >So I tried doing that by adding -Werror to longfin's CFLAGS >environment, >and it crashed and burned in configure. The most obvious problem is >that we didn't bother to supply a prototype for does_int64_work() in >the probes for 64-bit ints. However, even after fixing that, configure >produces totally bollixed output because -Werror breaks many of its >built-in tests. We could fix the int64 tests but we have no good way >of >fixing the built-in tests. > >AFAICS, if you want to build with -Werror, you have to configure >without >that and then inject it afterwards. I wonder how people who use >-Werror >are handling that. Is it possible to do at all in a buildfarm critter? Yea, I complained about that before. I think we'd have to add an extra configure option for it, because we rely in setting it only for certain tests. Personally I use CEXTRA (IIRC) to achieve that result after configure ran. Andres -- Sent from my Android device with K-9 Mail. Please excuse my brevity. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_recvlogical.c doesn't build with --disable-integer-datetimes
I wrote: > Yeah. I have longfin which is running Apple's clang, and is on a machine > that doesn't have much to do otherwise. I propose to turn on -Werror in > its configuration, and to configure a second critter on the same hardware > that runs with -Werror as well as --disable-integer-datetimes. Somebody > else should do similarly with a reasonably modern/stable gcc release. So I tried doing that by adding -Werror to longfin's CFLAGS environment, and it crashed and burned in configure. The most obvious problem is that we didn't bother to supply a prototype for does_int64_work() in the probes for 64-bit ints. However, even after fixing that, configure produces totally bollixed output because -Werror breaks many of its built-in tests. We could fix the int64 tests but we have no good way of fixing the built-in tests. AFAICS, if you want to build with -Werror, you have to configure without that and then inject it afterwards. I wonder how people who use -Werror are handling that. Is it possible to do at all in a buildfarm critter? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_recvlogical.c doesn't build with --disable-integer-datetimes
Alvaro Herrerawrites: > Tom Lane wrote: >> My own compilers don't generate errors either, only warnings. Maybe >> we could catch this sort of thing mechanically with a critter configured >> with -Werror as well as --disable-integer-datetimes, but I'm a tad >> hesitant to have a buildfarm machine configured with -Werror. We'd >> be raising the stakes quite a bit on keeping the code warning-free >> for a specific compiler that might prove to be picky in silly ways. >> >> But if it were a pretty mainstream compiler that hadn't shown much >> evidence of wanting to throw useless warnings, maybe that's fine. > I think we're at a point where one gcc plus one clang animals set up > with -Werror would be a good thing to have, and keep green. In > practice, this list fulfills the role of such a compiler, only with much > higher communication overhead. Yeah. I have longfin which is running Apple's clang, and is on a machine that doesn't have much to do otherwise. I propose to turn on -Werror in its configuration, and to configure a second critter on the same hardware that runs with -Werror as well as --disable-integer-datetimes. Somebody else should do similarly with a reasonably modern/stable gcc release. In the longer term, we might want to encourage more buildfarm owners to use -Werror, but only with mainstream compilers that are generally pretty sane about warnings. Some of the older critters generate hundreds/thousands of useless warnings; I'm not interested in trying to suppress all of those. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_recvlogical.c doesn't build with --disable-integer-datetimes
Tom Lane wrote: > My own compilers don't generate errors either, only warnings. Maybe > we could catch this sort of thing mechanically with a critter configured > with -Werror as well as --disable-integer-datetimes, but I'm a tad > hesitant to have a buildfarm machine configured with -Werror. We'd > be raising the stakes quite a bit on keeping the code warning-free > for a specific compiler that might prove to be picky in silly ways. > > But if it were a pretty mainstream compiler that hadn't shown much > evidence of wanting to throw useless warnings, maybe that's fine. > > Thoughts? Note that Thomas' compiler was using -Werror. I think we're at a point where one gcc plus one clang animals set up with -Werror would be a good thing to have, and keep green. In practice, this list fulfills the role of such a compiler, only with much higher communication overhead. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_recvlogical.c doesn't build with --disable-integer-datetimes
I wrote: > Thomas Munrowrites: >> Since commit 7c030783a5bd07cadffc2a1018bc33119a4c7505 it seems that $SUBJECT. > Hmm ... I thought we had at least one buildfarm member still testing > --disable-integer-datetimes. Evidently somebody needs to close that > gap. The plot thickens: we *do* have one such critter, mandrill, but for some reason it's not generating any complaint about this. Must be a very lax compiler. My own compilers don't generate errors either, only warnings. Maybe we could catch this sort of thing mechanically with a critter configured with -Werror as well as --disable-integer-datetimes, but I'm a tad hesitant to have a buildfarm machine configured with -Werror. We'd be raising the stakes quite a bit on keeping the code warning-free for a specific compiler that might prove to be picky in silly ways. But if it were a pretty mainstream compiler that hadn't shown much evidence of wanting to throw useless warnings, maybe that's fine. Thoughts? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_recvlogical.c doesn't build with --disable-integer-datetimes
Thomas Munrowrites: > Since commit 7c030783a5bd07cadffc2a1018bc33119a4c7505 it seems that $SUBJECT. Hmm ... I thought we had at least one buildfarm member still testing --disable-integer-datetimes. Evidently somebody needs to close that gap. > Maybe the attached is the right fix for that? I'll look at this later today, if no one beats me to it. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_recvlogical.c doesn't build with --disable-integer-datetimes
Thomas Munro wrote: > Hi, > > Since commit 7c030783a5bd07cadffc2a1018bc33119a4c7505 it seems that $SUBJECT. Added it to open items list. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers