Thread: additional GCC warning flags
This patch adds code to "configure" to check if GCC supports the following warning flags: -Wdeclaration-after-statement (GCC 3.4+), -Wold-style-definition (GCC 3.4+), and -Wendif-labels (GCC 3.3+). Any of these options that are supported by $CC are added to $CFLAGS. The patch also removes -Wmissing-declarations from the default CFLAGS, on the grounds that it is (I believe) redundant when -Wmissing-prototypes is also specified. (I didn't mention -Wendif-labels in my mail to -hackers, but it should be pretty innocuous: "Warn whenever an #else or an #endif are followed by text.") This patch updates configure.in and configure (I re-ran autoconf 2.53). It doesn't introduce any additional warning messages locally (Linux, GCC 3.4), but might do so on some platforms (the code in src/port/ is likely to trigger some warnings, I think). Because of that, I think it is probably best to save this for 8.1 -Neil
Attachment
FYI, I also use:
-Wpointer-arith -Wcast-align
locally. I think -Wcast-align throws errors on some platforms and can't
be use generally.
---------------------------------------------------------------------------
Neil Conway wrote:
> This patch adds code to "configure" to check if GCC supports the
> following warning flags: -Wdeclaration-after-statement (GCC 3.4+),
> -Wold-style-definition (GCC 3.4+), and -Wendif-labels (GCC 3.3+). Any of
> these options that are supported by $CC are added to $CFLAGS. The patch
> also removes -Wmissing-declarations from the default CFLAGS, on the
> grounds that it is (I believe) redundant when -Wmissing-prototypes is
> also specified.
>
> (I didn't mention -Wendif-labels in my mail to -hackers, but it should
> be pretty innocuous: "Warn whenever an #else or an #endif are followed
> by text.")
>
> This patch updates configure.in and configure (I re-ran autoconf 2.53).
> It doesn't introduce any additional warning messages locally (Linux, GCC
> 3.4), but might do so on some platforms (the code in src/port/ is likely
> to trigger some warnings, I think). Because of that, I think it is
> probably best to save this for 8.1
>
> -Neil
>
[ Attachment, skipping... ]
>
> ---------------------------(end of broadcast)---------------------------
> TIP 1: subscribe and unsubscribe commands go to majordomo@postgresql.org
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073
Neil Conway <neilc@samurai.com> writes:
> This patch updates configure.in and configure (I re-ran autoconf 2.53).
> It doesn't introduce any additional warning messages locally (Linux, GCC
> 3.4), but might do so on some platforms (the code in src/port/ is likely
> to trigger some warnings, I think). Because of that, I think it is
> probably best to save this for 8.1
As long as it can only introduce warnings and not errors, I think it is
fine to apply now. It might help us flush out some portability issues
--- we are just coming up on the stage of officially calling for port
reports for 8.0, so it seems like now is a good time to put it in.
One thing I'd suggest is that the "if test "$GCC" = yes; then"
conditional go outside the PGAC macros, ie
if test "$GCC" = yes; then
PGAC_PROG_CC_WARN_DECL_AFTER_STMT
PGAC_PROG_CC_WARN_OLD_STYLE_DEFN
PGAC_PROG_CC_WARN_ENDIF_LABELS
fi
as otherwise people with non-gcc compilers will think configure is
pretty stupid even to be making the tests.
Also, how about removing the conditional CFLAGS hacking from
Makefile.global.in entirely, and change the above to
if test "$GCC" = yes; then
CFLAGS="$CFLAGS -Wall -Wmissing-prototypes"
PGAC_PROG_CC_WARN_DECL_AFTER_STMT
PGAC_PROG_CC_WARN_OLD_STYLE_DEFN
PGAC_PROG_CC_WARN_ENDIF_LABELS
fi
which localizes the logic a tad more, and keeps the CFLAGS entries in a
rational-looking order.
regards, tom lane
Agreed, I see no harm in adding it now. --------------------------------------------------------------------------- Tom Lane wrote: > Neil Conway <neilc@samurai.com> writes: > > This patch updates configure.in and configure (I re-ran autoconf 2.53). > > It doesn't introduce any additional warning messages locally (Linux, GCC > > 3.4), but might do so on some platforms (the code in src/port/ is likely > > to trigger some warnings, I think). Because of that, I think it is > > probably best to save this for 8.1 > > As long as it can only introduce warnings and not errors, I think it is > fine to apply now. It might help us flush out some portability issues > --- we are just coming up on the stage of officially calling for port > reports for 8.0, so it seems like now is a good time to put it in. > > One thing I'd suggest is that the "if test "$GCC" = yes; then" > conditional go outside the PGAC macros, ie > > if test "$GCC" = yes; then > PGAC_PROG_CC_WARN_DECL_AFTER_STMT > PGAC_PROG_CC_WARN_OLD_STYLE_DEFN > PGAC_PROG_CC_WARN_ENDIF_LABELS > fi > > as otherwise people with non-gcc compilers will think configure is > pretty stupid even to be making the tests. > > Also, how about removing the conditional CFLAGS hacking from > Makefile.global.in entirely, and change the above to > > if test "$GCC" = yes; then > CFLAGS="$CFLAGS -Wall -Wmissing-prototypes" > PGAC_PROG_CC_WARN_DECL_AFTER_STMT > PGAC_PROG_CC_WARN_OLD_STYLE_DEFN > PGAC_PROG_CC_WARN_ENDIF_LABELS > fi > > which localizes the logic a tad more, and keeps the CFLAGS entries in a > rational-looking order. > > regards, tom lane > > ---------------------------(end of broadcast)--------------------------- > TIP 5: Have you checked our extensive FAQ? > > http://www.postgresql.org/docs/faqs/FAQ.html > -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073
Neil Conway wrote: > This patch adds code to "configure" to check if GCC supports the > following warning flags: -Wdeclaration-after-statement (GCC 3.4+), > -Wold-style-definition (GCC 3.4+), and -Wendif-labels (GCC 3.3+). Any > of these options that are supported by $CC are added to $CFLAGS. Can't you write one macro that takes the option to be tested as argument? For example like PGAC_PROG_CC_WARNING([-Wmissing-declarations]). -- Peter Eisentraut http://developer.postgresql.org/~petere/
On Tue, 2004-10-19 at 03:19, Tom Lane wrote: > As long as it can only introduce warnings and not errors, I think it is > fine to apply now. Okay -- I'll submit a revised patch that incorporates the suggestions from Peter and yourself later today. -Neil
On Mon, 2004-10-18 at 21:43, Bruce Momjian wrote: > FYI, I also use: > > -Wpointer-arith -Wcast-align > > locally. I think -Wcast-align throws errors on some platforms and can't > be use generally. -Wpointer-arith might be worth enabling. I'll add it to the GCC CFLAGS in the next patch I send in. As you said, -Wcast-align can't be enabled in general at the moment because it flags a ton of warnings on certain platforms (at least on IA64, probably other 64-bit platforms as well). I remember there were some vague rumblings that fixing these issues, but I haven't heard anything about that lately. -Neil
Neil Conway <neilc@samurai.com> writes:
> -Wpointer-arith might be worth enabling. I'll add it to the GCC CFLAGS
> in the next patch I send in.
If PG builds cleanly or nearly so with it on, by all means.
> As you said, -Wcast-align can't be enabled in general at the moment
> because it flags a ton of warnings on certain platforms (at least on
> IA64, probably other 64-bit platforms as well).
Yes, HPPA as well. I have long harbored a wish to clean this up but
it never gets to the top of the to-do list. I am not sure that it would
really repay the effort to make the code clean against this --- I can
only recall one or two bugs in the last several years that this might
have caught.
When I looked at the current gcc Info docs, I noticed that there seem to
be several new warning types that might be worth turning on. Did you
consider others beyond the three you're proposing now?
regards, tom lane
On Tue, 2004-10-19 at 12:21, Tom Lane wrote: > When I looked at the current gcc Info docs, I noticed that there seem to > be several new warning types that might be worth turning on. Did you > consider others beyond the three you're proposing now? I took a look through the list, but I probably missed a few useful options. -Wcast-qual emits too many warnings, as does -Wsign-compare. -Wstrict-prototypes causes a few distinct warnings but they are emitted repeatedly. At least one does not seem easily solvable: expression_tree_walker() and friends declare the callback function as bool (*walker) (), but it seems a pain to make that type declaration more precise. -Wnested-externs causes a single warning (postmaster.c:580) that seems fixable, so that might be worth enabling. -Waggregate-return emits a lot of warnings (since it flags both the call site and the definition of a function that returns an aggregate type), but there are relatively few offending functions: GetRedoRecPtr, XLogInsert(), log_heap_clean(), log_heap_update(), log_heap_move(), BufferGetFileNode() and SetOutput() (in pg_dump). If I've missed any you think might be useful, let me know. -Neil
On Tue, 2004-10-19 at 11:59, Neil Conway wrote: > -Wpointer-arith might be worth enabling. I'll add it to the GCC CFLAGS > in the next patch I send in. Attached is a revised patch. Changes: - add -Wpointer-arith to the default CFLAGS when using GCC - add an AC macro AC_PROG_CC_CFLAGS_OPT that checks if $CC supports the specified command-line option and adds it to the CFLAGS if it does - replace the hard-coded test for -fno-strict-aliasing with AC_PROG_CC_CFLAGS, and use AC_PROG_CC_CFLAGS to check for -Wendif-labels, -Wdeclaration-after-statement, and -Wold-style-definition Barring any objections, I will apply this to CVS tomorrow. BTW, since we're on the topic of compiler options, is there a reason we don't use -g3 with GCC when --enable-debug is specified? It seems worth using to me. -Neil
Attachment
Neil Conway <neilc@samurai.com> writes:
> Attached is a revised patch. Changes:
Looks reasonable to me. Just one comment: should the
-fno-strict-aliasing probe be inside the "if test "$GCC" = yes" part?
It effectively was in the original.
> BTW, since we're on the topic of compiler options, is there a reason we
> don't use -g3 with GCC when --enable-debug is specified? It seems worth
> using to me.
How much does it bloat the executable?
regards, tom lane
Neil Conway <neilc@samurai.com> writes:
> -Wstrict-prototypes causes a few distinct warnings but they are emitted
> repeatedly. At least one does not seem easily solvable:
> expression_tree_walker() and friends declare the callback function as
> bool (*walker) (), but it seems a pain to make that type declaration
> more precise.
Yeah; I deliberately left that callback weakly typed, because it seemed
that the alternative was to expect every caller to cast, which would
effectively disable any error checking you might hope to get anyway :-(
> -Wnested-externs causes a single warning (postmaster.c:580) that seems
> fixable, so that might be worth enabling.
Agreed. That extern is pretty bogus in itself...
> If I've missed any you think might be useful, let me know.
-Wbad-function-cast might possibly be interesting, although I'm afraid
it would be likely to barf on some of our Datum<=>pointer conversions.
regards, tom lane
Neil Conway wrote: > Attached is a revised patch. Changes: Another word from the wise: Never write "recent" in code designed for longevity. :) > BTW, since we're on the topic of compiler options, is there a reason > we don't use -g3 with GCC when --enable-debug is specified? It seems > worth using to me. I'm sure we could discuss dozens of compiler options. Don't even start on the -march ones. I think in the interest of compatibility with the rest of the world we should stick with the basic levels of optimization, debugging, and warning that make most people reasonably happy and let the users worry about the rest in their own time. I'm already not so happy about the new warning options, because they make the compile lines too long. How's that for an argument? ;-) -- Peter Eisentraut http://developer.postgresql.org/~petere/
On Tue, Oct 19, 2004 at 07:40:56PM +0200, Peter Eisentraut wrote:
> I'm already not so happy about the new warning options, because they
> make the compile lines too long. How's that for an argument? ;-)
There's a better solution: use an approach similar to that used in the
Linux kernel, which echoes only the object name that's currently being
compiled. So there's output showing progress, but not unnecessarily
long lines. Of course, it can be easily reverted by using a configure
argument or make variable.
This is from Linux's main Makefile (I may be missing a piece):
# Beautify output
# ---------------------------------------------------------------------------
#
# Normally, we echo the whole command before executing it. By making
# that echo $($(quiet)$(cmd)), we now have the possibility to set
# $(quiet) to choose other forms of output instead, e.g.
#
# quiet_cmd_cc_o_c = Compiling $(RELDIR)/$@
# cmd_cc_o_c = $(CC) $(c_flags) -c -o $@ $<
#
# If $(quiet) is empty, the whole command will be printed.
# If it is set to "quiet_", only the short version will be printed.
# If it is set to "silent_", nothing wil be printed at all, since
# the variable $(silent_cmd_cc_o_c) doesn't exist.
#
# A simple variant is to prefix commands with $(Q) - that's usefull
# for commands that shall be hidden in non-verbose mode.
#
# $(Q)ln $@ :<
#
# If KBUILD_VERBOSE equals 0 then the above command will be hidden.
# If KBUILD_VERBOSE equals 1 then the above command is displayed.
ifeq ($(KBUILD_VERBOSE),1)
quiet =
Q =
else
quiet=quiet_
Q = @
endif
A sample target would be
%.o: %.c scripts FORCE
$(Q)$(MAKE) $(build)=$(@D) $@
--
Alvaro Herrera (<alvherre[a]dcc.uchile.cl>)
"Endurecerse, pero jamás perder la ternura" (E. Guevara)
Alvaro Herrera <alvherre@dcc.uchile.cl> writes:
> There's a better solution: use an approach similar to that used in the
> Linux kernel, which echoes only the object name that's currently being
> compiled.
If I'm reading this correctly, it requires replacing every default build
rule with our own hacked-up version. That's definitely not a recipe for
maintainability, portability, or playing nice with local
customizations... even if I thought the goal was a good idea, which I do
not particularly. If I'm looking at make output it's generally because
I need to fix a problem; not knowing exactly what's being executed
is not going to improve my life.
regards, tom lane
Tom Lane wrote:
> Neil Conway <neilc@samurai.com> writes:
>
>>-Wpointer-arith might be worth enabling. I'll add it to the GCC CFLAGS
>>in the next patch I send in.
>
>
> If PG builds cleanly or nearly so with it on, by all means.
>
>
>>As you said, -Wcast-align can't be enabled in general at the moment
>>because it flags a ton of warnings on certain platforms (at least on
>>IA64, probably other 64-bit platforms as well).
>
>
> Yes, HPPA as well. I have long harbored a wish to clean this up but
> it never gets to the top of the to-do list. I am not sure that it would
> really repay the effort to make the code clean against this --- I can
> only recall one or two bugs in the last several years that this might
> have caught.
>
> When I looked at the current gcc Info docs, I noticed that there seem to
> be several new warning types that might be worth turning on. Did you
> consider others beyond the three you're proposing now?
Time ago I compiled 8.0 using gcc 4.0 with a bunch of warning, did you see
my post ? I report it here:
--------------------------------------------------------------------------------------
Hi all,
I succesfull compiled postgres 8.0beta2 with a recent gcc 4.0 snapshot:
gcc (GCC) 4.0.0 20040911 (experimental)
Copyright (C) 2004 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
# select version();
version
-----------------------------------------------------------------------------------------------------
PostgreSQL 8.0.0beta2 on i686-pc-linux-gnu, compiled by GCC gcc (GCC) 4.0.0 20040911 (experimental)
(1 row)
All the tests passed, however I had a bunch of warning during the
compilation, the more common was: "left-hand operand of comma expression has no effect"
These are some examples:
=============================================================================
gist.c: In function `gistlayerinsert':
gist.c:463: warning: left-hand operand of comma expression has no effect
gist.c: In function `gistreadbuffer':
gist.c:644: warning: left-hand operand of comma expression has no effect
gist.c: In function `gistchoose':
gist.c:1479: warning: left-hand operand of comma expression has no effect
=============================================================================
pg_conversion.c: In function `pg_convert_using':
pg_conversion.c:334: warning: pointer targets in passing arg 1 of `strlen' differ in signedness
=============================================================================
nbtinsert.c: In function `_bt_insertonpg':
nbtinsert.c:380: warning: 'itup_blkno' may be used uninitialized in this function
nbtinsert.c:379: warning: 'itup_off' may be used uninitialized in this function
=============================================================================
pg_proc.c: In function `match_prosrc_to_query':
pg_proc.c:915: warning: pointer targets in passing arg 1 of `pg_mbstrlen_with_len' differ in signedness
pg_proc.c:929: warning: pointer targets in passing arg 1 of `pg_mbstrlen_with_len' differ in signedness
pg_proc.c: In function `match_prosrc_to_literal':
pg_proc.c:982: warning: pointer targets in passing arg 1 of `pg_mblen' differ in signedness
=============================================================================
orindxpath.c: In function `best_or_subclause_indexes':
orindxpath.c:258: warning: 'best_indexinfo' may be used uninitialized in this function
orindxpath.c:260: warning: 'best_indexquals' may be used uninitialized in this function
orindxpath.c:259: warning: 'best_indexclauses' may be used uninitialized in this function
orindxpath.c:261: warning: 'best_startup_cost' may be used uninitialized in this function
orindxpath.c:262: warning: 'best_total_cost' may be used uninitialized in this function
=============================================================================
createplan.c: In function `create_plan':
createplan.c:1244: warning: 'opclass' may be used uninitialized in this function
=============================================================================
Regards
Gaetano Mendola
On Wed, 2004-10-20 at 02:07, Tom Lane wrote: > Looks reasonable to me. Just one comment: should the > -fno-strict-aliasing probe be inside the "if test "$GCC" = yes" part? > It effectively was in the original. Yeah, makes sense. Patch applied with this fix and Peter's suggested improvement to a comment. > > BTW, since we're on the topic of compiler options, is there a reason we > > don't use -g3 with GCC when --enable-debug is specified? It seems worth > > using to me. > > How much does it bloat the executable? Quite a bit, as it turns out: with CFLAGS="-O2 -g, "postgres" is 7077111 bytes. With CFLAGS="-O2 -g3", "postgres" is 51227279 bytes. So there is quite an increase -- I guess we had better stick with -g then. -Neil
Neil Conway wrote: > On Wed, 2004-10-20 at 02:07, Tom Lane wrote: > > Looks reasonable to me. Just one comment: should the > > -fno-strict-aliasing probe be inside the "if test "$GCC" = yes" part? > > It effectively was in the original. > > Yeah, makes sense. Patch applied with this fix and Peter's suggested > improvement to a comment. > > > > BTW, since we're on the topic of compiler options, is there a reason we > > > don't use -g3 with GCC when --enable-debug is specified? It seems worth > > > using to me. > > > > How much does it bloat the executable? > > Quite a bit, as it turns out: with CFLAGS="-O2 -g, "postgres" is 7077111 > bytes. With CFLAGS="-O2 -g3", "postgres" is 51227279 bytes. So there is > quite an increase -- I guess we had better stick with -g then. Yes, stay with -g. As I remember the more verbose -g information isn't usable by most debug tools anyway. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073