Thread: Setting -Werror in CFLAGS
During the talk "How To Get Your PostgreSQL Patch Accepted" during PgCon last year, I raised the idea of making a -Werror build option easily available. I think it was Robert that pointed out that the problem with that was that there is a warning due to an upstream Flex bug that we can't do anything about. He was right about that; I subsequently tried fairly hard to get the Flex guys to fix it a few months back, but it seems that Flex isn't that well maintained, so even though I convinced someone to write a patch to fix it, it ended up going nowhere. Looking back through the commit log, I can see that this is an old problem. i thought that we could get most of the benefit of a -Werror option by excluding the relevant class of error in CLAGS; gcc allows for this (pity it doesn't have MSVC's much more granular ability to discriminate against diagnostic messages). This should work: ./configure --prefix=/home/peter/pgsql CFLAGS="-Werror -Wno-error=unused-but-set-variable" However, it does not (with or without the -Wno-error): checking whether getpwuid_r takes a fifth argument... no checking whether strerror_r returns int... no checking test program... ok checking whether long int is 64 bits... no checking whether long long int is 64 bits... no configure: error: Cannot find a working 64-bit integer type. Obviously it breaks this one configure test. However, I got it to work in 2 minutes by hacking up config.log. It would be nice if someone could fix the test (and possibly later ones) so that it doesn't produce a warning/error when invoking the compiler, and it finds a 64-bit int type as expected. That way, it could sort of become folk wisdom that you should build Postgres with those flags during development, and maybe even, on some limited basis, on the build farm, and we'd all be sightly better off. Giving that many people build with CFLAGS="-O0 -g" on a day-to-day basis, which doesn't show some warnings, the folk wisdom might end up being: "Just before you submit your patch, see how an -O2 -Werror build goes". There is an entry in the parser Makefile to support this sort of use, which makes it look like my -Wno-error=unused-but-set-variable use would be redundant if it worked: # Latest flex causes warnings in this file. ifeq ($(GCC),yes) gram.o: CFLAGS += -Wno-error endif (although we could discriminate better within the parse here by using my flag). As if to prove my point, I found this warning which I didn't previously notice, just from 5 minutes of playing around: hashovfl.c: In function ‘_hash_freeovflpage’: hashovfl.c:394:10: error: variable ‘bucket’ set but not used [-Werror=unused-but-set-variable] cc1: all warnings being treated as errors (plus others) Yes, I know that these only appeared in GCC 4.6+ and as such are a relatively recent phenomenon, but there has been some effort to eliminate them, and if I could get a non-hacked -Werror build I'd feel happy enough about excluding them as already outlined. -- Peter Geoghegan http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training and Services
On Tue, Jan 3, 2012 at 7:39 PM, Peter Geoghegan <peter@2ndquadrant.com> wrote: > Yes, I know that these only appeared in GCC 4.6+ and as such are a > relatively recent phenomenon, but there has been some effort to > eliminate them, and if I could get a non-hacked -Werror build I'd feel > happy enough about excluding them as already outlined. I just do this: echo COPT=-Werror > src/Makefile.custom ...which seems to work reasonably well. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Jan 3, 2012 at 7:39 PM, Peter Geoghegan <peter@2ndquadrant.com> wrote:
>> Yes, I know that these only appeared in GCC 4.6+ and as such are a
>> relatively recent phenomenon, but there has been some effort to
>> eliminate them, and if I could get a non-hacked -Werror build I'd feel
>> happy enough about excluding them as already outlined.
> I just do this:
> echo COPT=-Werror > src/Makefile.custom
> ...which seems to work reasonably well.
I see no point in -Werror whatsoever. If you aren't examining the make
output for warnings, you're not following proper development practice
IMO. gcc is not the only tool we use in the build process, so if you
are relying on -Werror to call attention to everything you should be
worrying about, you lost already.
I'm also less than thrilled with the idea that whatever the gcc boys
decide to make a warning tomorrow will automatically become a MUST FIX
NOW for us. If you don't see why this is a problem, try building any
PG release more than a few months old on latest and greatest gcc.
(Of note here, latest-and-greatest is changing again this week, at least
for Fedora, and I fully expect 4.7 to start whinging about things we
never heard of before.) Moving-target warnings are one thing,
moving-target hard errors are another thing entirely.
Personally I tend to do something like
make -j4 >make.out 2>make.errcat make.err
regards, tom lane
On Tue, Jan 3, 2012 at 9:23 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Tue, Jan 3, 2012 at 7:39 PM, Peter Geoghegan <peter@2ndquadrant.com> wrote: >>> Yes, I know that these only appeared in GCC 4.6+ and as such are a >>> relatively recent phenomenon, but there has been some effort to >>> eliminate them, and if I could get a non-hacked -Werror build I'd feel >>> happy enough about excluding them as already outlined. > >> I just do this: >> echo COPT=-Werror > src/Makefile.custom >> ...which seems to work reasonably well. > > I see no point in -Werror whatsoever. If you aren't examining the make > output for warnings, you're not following proper development practice > IMO. I find -Werror to be a convenient way to examine the output for warnings. Otherwise they scroll off the screen. Yeah, I could save the output to a file and grep it afterwards, but that seems less convenient. I'm clearly not the only one doing it this way, since src/backend/parser/gram.o manually sticks in -Wno-error... > gcc is not the only tool we use in the build process, so if you > are relying on -Werror to call attention to everything you should be > worrying about, you lost already. Hmm, I guess. I've never had a problem with anything else that I can remember, though. > I'm also less than thrilled with the idea that whatever the gcc boys > decide to make a warning tomorrow will automatically become a MUST FIX > NOW for us. I'm not thrilled about that either. Especially since they seem to be adding more and more warnings that are harder and harder to work around for issues that are less and less important. Unimportant warnings that are easily avoidable are not so bad, but... -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 4 January 2012 18:44, Robert Haas <robertmhaas@gmail.com> wrote: > On Tue, Jan 3, 2012 at 9:23 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> I'm also less than thrilled with the idea that whatever the gcc boys >> decide to make a warning tomorrow will automatically become a MUST FIX >> NOW for us. > > I'm not thrilled about that either. Especially since they seem to be > adding more and more warnings that are harder and harder to work > around for issues that are less and less important. Unimportant > warnings that are easily avoidable are not so bad, but... I'd have a certain amount of sympathy for that view. It took building with Clang to notice that we incorrectly used one enum rvalue to assign to a variable of another enum type, which I thought was a little bit surprising; I'd have expected GCC to catch that one, since it is pretty likely to be valid. -- Peter Geoghegan http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training and Services
On 04.01.2012 20:44, Robert Haas wrote: > On Tue, Jan 3, 2012 at 9:23 PM, Tom Lane<tgl@sss.pgh.pa.us> wrote: >> Robert Haas<robertmhaas@gmail.com> writes: >>> On Tue, Jan 3, 2012 at 7:39 PM, Peter Geoghegan<peter@2ndquadrant.com> wrote: >>>> Yes, I know that these only appeared in GCC 4.6+ and as such are a >>>> relatively recent phenomenon, but there has been some effort to >>>> eliminate them, and if I could get a non-hacked -Werror build I'd feel >>>> happy enough about excluding them as already outlined. >> >>> I just do this: >>> echo COPT=-Werror> src/Makefile.custom >>> ...which seems to work reasonably well. >> >> I see no point in -Werror whatsoever. If you aren't examining the make >> output for warnings, you're not following proper development practice >> IMO. > > I find -Werror to be a convenient way to examine the output for > warnings. Otherwise they scroll off the screen. Yeah, I could save > the output to a file and grep it afterwards, but that seems less > convenient. I'm clearly not the only one doing it this way, since > src/backend/parser/gram.o manually sticks in -Wno-error... I use "make -s". -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
On 01/04/2012 02:35 PM, Heikki Linnakangas wrote: > On 04.01.2012 20:44, Robert Haas wrote: >> On Tue, Jan 3, 2012 at 9:23 PM, Tom Lane<tgl@sss.pgh.pa.us> wrote: >>> Robert Haas<robertmhaas@gmail.com> writes: >>>> On Tue, Jan 3, 2012 at 7:39 PM, Peter >>>> Geoghegan<peter@2ndquadrant.com> wrote: >>>>> Yes, I know that these only appeared in GCC 4.6+ and as such are a >>>>> relatively recent phenomenon, but there has been some effort to >>>>> eliminate them, and if I could get a non-hacked -Werror build I'd >>>>> feel >>>>> happy enough about excluding them as already outlined. >>> >>>> I just do this: >>>> echo COPT=-Werror> src/Makefile.custom >>>> ...which seems to work reasonably well. >>> >>> I see no point in -Werror whatsoever. If you aren't examining the make >>> output for warnings, you're not following proper development practice >>> IMO. >> >> I find -Werror to be a convenient way to examine the output for >> warnings. Otherwise they scroll off the screen. Yeah, I could save >> the output to a file and grep it afterwards, but that seems less >> convenient. I'm clearly not the only one doing it this way, since >> src/backend/parser/gram.o manually sticks in -Wno-error... > > I use "make -s". Yeah, that's a good thing to do. We are by far the most vigilant project I am aware of about fixing warnings. That's a Good Thing (tm,). Build most FOSS software and you see huge numbers of warnings fly by. It can get quite distressing. We turn the errors off for gram.o precisely because we can't control it, since the included source file scan.c is generated by flex. cheers andrew
On ons, 2012-01-04 at 00:39 +0000, Peter Geoghegan wrote: > This should work: > > ./configure --prefix=/home/peter/pgsql CFLAGS="-Werror > -Wno-error=unused-but-set-variable" > > However, it does not (with or without the -Wno-error): > > checking whether getpwuid_r takes a fifth argument... no > checking whether strerror_r returns int... no > checking test program... ok > checking whether long int is 64 bits... no > checking whether long long int is 64 bits... no > configure: error: Cannot find a working 64-bit integer type. > > Obviously it breaks this one configure test. However, I got it to work > in 2 minutes by hacking up config.log. It would be nice if someone > could fix the test (and possibly later ones) so that it doesn't > produce a warning/error when invoking the compiler, and it finds a > 64-bit int type as expected. I've tried this in the past, it's pretty difficult to make this work throughout. I think and easier approach would be to filter out -Werror out of CFLAGS early in configure and put it back at the end. > That way, it could sort of become folk wisdom that you should build > Postgres with those flags during > development, and maybe even, on some limited basis, on the build farm, > and we'd all be sightly better off. I think in practice many developers do this anyway, so you do get that level of testing already. > There is an entry in the parser Makefile to support this sort of use, > which makes it look like my -Wno-error=unused-but-set-variable use > would be redundant if it worked: > > # Latest flex causes warnings in this file. > ifeq ($(GCC),yes) > gram.o: CFLAGS += -Wno-error > endif > > (although we could discriminate better within the parse here by using my flag). The -Wno-error=something flag doesn't work in older versions of GCC. But feel free to research which ones. > As if to prove my point, I found this warning which I didn't > previously notice, just from 5 minutes of playing around: > > hashovfl.c: In function ‘_hash_freeovflpage’: > hashovfl.c:394:10: error: variable ‘bucket’ set but not used > [-Werror=unused-but-set-variable] > cc1: all warnings being treated as errors > > (plus others) See thread "[HACKERS] lots of unused variable warnings in assert-free builds" for the reason.
On tis, 2012-01-03 at 21:23 -0500, Tom Lane wrote: > I see no point in -Werror whatsoever. If you aren't examining the make > output for warnings, you're not following proper development practice > IMO. gcc is not the only tool we use in the build process, so if you > are relying on -Werror to call attention to everything you should be > worrying about, you lost already. Well, cite your source. ;-) Proper software development practices, as I understand them, is that there is one command to build the thing, and that either fails or it doesn't. If we rely on people to read the build log, then we've lost already. The only particular case where this doesn't work that I can think of is that you need to examine the flex output for warnings. But that is very isolated. If you don't change the scanner source files (and perhaps one or two other things, or if you change the build system itself), then relying on the exit status of make should be enough. If not, we need to fix that. > I'm also less than thrilled with the idea that whatever the gcc boys > decide to make a warning tomorrow will automatically become a MUST FIX > NOW for us. If you don't see why this is a problem, try building any > PG release more than a few months old on latest and greatest gcc. That's why -Werror should never be the default for a postgresql build, but that doesn't mean that we can't make it a useful optional tool. > (Of note here, latest-and-greatest is changing again this week, at least > for Fedora, and I fully expect 4.7 to start whinging about things we > never heard of before.) As a side note: I tested gcc 4.7 a few weeks ago and it doesn't introduce any new warnings. Of course, it's not final yet, but it's pretty close.
On ons, 2012-01-04 at 13:44 -0500, Robert Haas wrote: > I'm not thrilled about that either. Especially since they seem to be > adding more and more warnings that are harder and harder to work > around for issues that are less and less important. Unimportant > warnings that are easily avoidable are not so bad, but... > I think the reason they add all these new warnings is that a lot of software is of poor quality. A lot of software probably doesn't check any return values, so they need to see those warnings. If the last 3 out of 100 are hard to fix, that might be a small price to pay.
On 18 January 2012 19:40, Peter Eisentraut <peter_e@gmx.net> wrote: > On ons, 2012-01-04 at 13:44 -0500, Robert Haas wrote: >> I'm not thrilled about that either. Especially since they seem to be >> adding more and more warnings that are harder and harder to work >> around for issues that are less and less important. Unimportant >> warnings that are easily avoidable are not so bad, but... >> > I think the reason they add all these new warnings is that a lot of > software is of poor quality. A lot of software probably doesn't check > any return values, so they need to see those warnings. If the last 3 > out of 100 are hard to fix, that might be a small price to pay. I recently ran a static analysis tool, Clang Static Analyzer, on Postgres. It was an amusing distraction for half an hour, but it wasn't particularly useful. It was immediately obvious 1) Why the tool flagged certain code as possibly questionable and 2) Why it wasn't actually questionable at all. It would probably be really useful with a poor quality codebase though. As I've already said, the fact that a Clang warning successfully flagged a bug in Postgres a while back does go to show that there is some benefit to be had from all of these sorts of diagnostics. I have a rather low opinion of GCC's diagnostics though. -- Peter Geoghegan http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training and Services
On Wed, Jan 04, 2012 at 01:44:07PM -0500, Robert Haas wrote: > On Tue, Jan 3, 2012 at 9:23 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Robert Haas <robertmhaas@gmail.com> writes: > >> On Tue, Jan 3, 2012 at 7:39 PM, Peter Geoghegan <peter@2ndquadrant.com> wrote: > >>> Yes, I know that these only appeared in GCC 4.6+ and as such are a > >>> relatively recent phenomenon, but there has been some effort to > >>> eliminate them, and if I could get a non-hacked -Werror build I'd feel > >>> happy enough about excluding them as already outlined. > > > >> I just do this: > >> echo COPT=-Werror > src/Makefile.custom > >> ...which seems to work reasonably well. > > > > I see no point in -Werror whatsoever. If you aren't examining the make > > output for warnings, you're not following proper development practice > > IMO. > > I find -Werror to be a convenient way to examine the output for > warnings. Otherwise they scroll off the screen. Yeah, I could save > the output to a file and grep it afterwards, but that seems less > convenient. Our src/tools/pgtest does this: http://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/tools/pgtest;hb=HEAD -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +