Обсуждение: Setting -Werror in CFLAGS

Поиск
Список
Период
Сортировка

Setting -Werror in CFLAGS

От
Peter Geoghegan
Дата:
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


Re: Setting -Werror in CFLAGS

От
Robert Haas
Дата:
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


Re: Setting -Werror in CFLAGS

От
Tom Lane
Дата:
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


Re: Setting -Werror in CFLAGS

От
Robert Haas
Дата:
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


Re: Setting -Werror in CFLAGS

От
Peter Geoghegan
Дата:
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


Re: Setting -Werror in CFLAGS

От
Heikki Linnakangas
Дата:
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


Re: Setting -Werror in CFLAGS

От
Andrew Dunstan
Дата:

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







Re: Setting -Werror in CFLAGS

От
Peter Eisentraut
Дата:
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.




Re: Setting -Werror in CFLAGS

От
Peter Eisentraut
Дата:
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.



Re: Setting -Werror in CFLAGS

От
Peter Eisentraut
Дата:
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.



Re: Setting -Werror in CFLAGS

От
Peter Geoghegan
Дата:
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


Re: Setting -Werror in CFLAGS

От
Bruce Momjian
Дата:
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. +