additional GCC warning flags

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

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