RFC: Use -Wall -Wextra

RFC: Use -Wall -Wextra

[Daniel Jacobowitz]

I'd like to hear opinions on this patch.  It changes the default set of GDB
build warnings from:

build_warnings="-Wimplicit -Wreturn-type -Wcomment -Wtrigraphs \
-Wformat -Wparentheses -Wpointer-arith -Wformat-nonliteral \
-Wunused-label -Wunused-function -Wno-pointer-sign"

to:

build_warnings="-Wall -Wextra -Wpointer-arith -Wformat-nonliteral \
-Wno-pointer-sign -Wno-unused-parameter \
-Wno-unused -Wno-sign-compare -Wno-switch -Wno-missing-field-initializers"

The main reason for avoiding -Wall, as far as I can remember and according
to the comments, was to avoid -Wunused-parameter.  But it's easy to do that
explicitly and hold on to -Wall (which doesn't even enable that warning
unless -Wextra is added too, at least nowadays).  I went even further,
and added -Wextra.

My original motivation for this patch was a bug I'll be fixing later
today on ia64, which turned out to be a comparison of size_t < 0 - always
false.  GCC can warn about that, but we don't ask it to.

I picked the above set of warnings after some experimentation.  GDB
doesn't build with them without the patch I'll post after this one.
Some of that patch was placating GCC, but more of it was fixing actual
bugs that haven't bitten yet, so I think this is a good set!

I'd really like to turn on -Wunused too, but it has been off for so long
that we have a substantial number of unused local variables - it will take
some work to clean up.  I'm sure this patch will break some other targets,
but we have a while before the next release to clean that up, and it's
very easy to do so.  I'll volunteer to do everything else that shows up
building cross debuggers if we agree on this change.

By the way, a nice side benefit is that -Wall enables -Wuninitialized iff
optimization is enabled so you'll be able to say CFLAGS="-g -O0" without
having to disable -Werror for the "-Wuninitialized requires -O1" warning.

-- 
Daniel Jacobowitz
CodeSourcery

2006-12-28  Daniel Jacobowitz  <dan@codesourcery.com>

	* configure.ac (build_warnings): Use -Wall -Wextra.
	* configure: Regenerated.

2006-12-28  Daniel Jacobowitz  <dan@codesourcery.com>

	* doc/gdbint.texinfo (Compiler Warnings): Update for -Wall -Wextra
	usage.

Index: configure.ac
===================================================================
RCS file: /cvs/src/src/gdb/configure.ac,v
retrieving revision 1.36
diff -u -p -r1.36 configure.ac
--- configure.ac	22 Nov 2006 17:34:15 -0000	1.36
+++ configure.ac	28 Dec 2006 19:39:05 -0000
@@ -1111,32 +1111,17 @@ if test "${ERROR_ON_WARNING}" = yes ; th
     WERROR_CFLAGS="-Werror"
 fi
 
-# NOTE: Don't add -Wall or -Wunused, they both include
-# -Wunused-parameter which reports bogus warnings.
-# NOTE: If you add to this list, remember to update
+# We avoid -Wunused-parameter, because GDB has a lot of
+# callback interfaces with unused arguments.
+# The entries after -Wno-unused-parameter are disabled warnings
+# which may or may not be enabled in the future, which can not
+# currently be used to build GDB.
+# NOTE: If you change this list, remember to update
 # gdb/doc/gdbint.texinfo.
-build_warnings="-Wimplicit -Wreturn-type -Wcomment -Wtrigraphs \
--Wformat -Wparentheses -Wpointer-arith -Wformat-nonliteral \
--Wunused-label -Wunused-function -Wno-pointer-sign"
-
-# GCC supports -Wuninitialized only with -O or -On, n != 0.
-if test x${CFLAGS+set} = xset; then
-  case "${CFLAGS}" in
-    *"-O0"* ) ;;
-    *"-O"* )
-      build_warnings="${build_warnings} -Wuninitialized"
-    ;;
-  esac
-else
-  build_warnings="${build_warnings} -Wuninitialized"
-fi
+build_warnings="-Wall -Wextra -Wpointer-arith -Wformat-nonliteral \
+-Wno-pointer-sign -Wno-unused-parameter \
+-Wno-unused -Wno-sign-compare -Wno-switch -Wno-missing-field-initializers"
 
-# Up for debate: -Wswitch -Wcomment -trigraphs -Wtrigraphs
-# -Wunused-function -Wunused-variable -Wunused-value
-# -Wchar-subscripts -Wtraditional -Wshadow -Wcast-qual
-# -Wcast-align -Wwrite-strings -Wconversion -Wstrict-prototypes
-# -Wmissing-prototypes -Wmissing-declarations -Wredundant-decls
-# -Woverloaded-virtual -Winline -Werror"
 AC_ARG_ENABLE(build-warnings,
 [  --enable-build-warnings Enable build-time compiler warnings if gcc is used],
 [case "${enableval}" in
Index: doc/gdbint.texinfo
===================================================================
RCS file: /cvs/src/src/gdb/doc/gdbint.texinfo,v
retrieving revision 1.247
diff -u -p -r1.247 gdbint.texinfo
--- doc/gdbint.texinfo	10 Nov 2006 19:20:37 -0000	1.247
+++ doc/gdbint.texinfo	28 Dec 2006 19:39:07 -0000
@@ -5471,64 +5471,30 @@ errors.}
 @subsection Compiler Warnings
 @cindex compiler warnings
 
-With few exceptions, developers should include the configuration option
-@samp{--enable-gdb-build-warnings=,-Werror} when building @value{GDBN}.
-The exceptions are listed in the file @file{gdb/MAINTAINERS}.
+With few exceptions, developers should avoid the configuration option
+@samp{--disable-werror} when building @value{GDBN}.  The exceptions
+are listed in the file @file{gdb/MAINTAINERS}.  The default, when
+building with @sc{gcc}, is @samp{--enable-werror}.
 
 This option causes @value{GDBN} (when built using GCC) to be compiled
 with a carefully selected list of compiler warning flags.  Any warnings
-from those flags being treated as errors.
+from those flags are treated as errors.
 
 The current list of warning flags includes:
 
 @table @samp
-@item -Wimplicit
-Since @value{GDBN} coding standard requires all functions to be declared
-using a prototype, the flag has the side effect of ensuring that
-prototyped functions are always visible with out resorting to
-@samp{-Wstrict-prototypes}.
-
-@item -Wreturn-type
-Such code often appears to work except on instruction set architectures
-that use register windows.
+@item -Wall -Wextra
+All typical @sc{gcc} warnings.
 
-@item -Wcomment
-
-@item -Wtrigraphs
+@item -Wpointer-arith
 
-@item -Wformat
-@itemx -Wformat-nonliteral
+@item -Wformat-nonliteral
+Non-literal format strings, with a few exceptions, are bugs - they
+might contain unintented user-supplied format specifiers.
 Since @value{GDBN} uses the @code{format printf} attribute on all
-@code{printf} like functions these check not just @code{printf} calls
+@code{printf} like functions this checks not just @code{printf} calls
 but also calls to functions such as @code{fprintf_unfiltered}.
 
-@item -Wparentheses
-This warning includes uses of the assignment operator within an
-@code{if} statement.
-
-@item -Wpointer-arith
-
-@item -Wuninitialized
-
-@item -Wunused-label
-This warning has the additional benefit of detecting the absence of the
-@code{case} reserved word in a switch statement:
-@smallexample
-enum @{ FD_SCHEDULED, NOTHING_SCHEDULED @} sched;
-@dots{}
-switch (sched)
-  @{
-  case FD_SCHEDULED:
-    @dots{};
-    break;
-  NOTHING_SCHEDULED:
-    @dots{};
-    break;
-  @}
-@end smallexample
-
-@item -Wunused-function
-
 @item -Wno-pointer-sign
 In version 4.0, GCC began warning about pointer argument passing or
 assignment even when the source and destination differed only in
@@ -5537,19 +5503,21 @@ carefully between @code{char} and @code{
 the @value{GDBN} developers decided correcting these warnings wasn't
 worth the time it would take.
 
-@end table
-
-@emph{Pragmatics: Due to the way that @value{GDBN} is implemented most
-functions have unused parameters.  Consequently the warning
-@samp{-Wunused-parameter} is precluded from the list.  The macro
+@item -Wno-unused-parameter
+Due to the way that @value{GDBN} is implemented many functions have
+unused parameters.  Consequently this warning is avoided.  The macro
 @code{ATTRIBUTE_UNUSED} is not used as it leads to false negatives ---
 it is not an error to have @code{ATTRIBUTE_UNUSED} on a parameter that
-is being used.  The options @samp{-Wall} and @samp{-Wunused} are also
-precluded because they both include @samp{-Wunused-parameter}.}
+is being used.
+
+@item -Wno-unused
+@itemx -Wno-sign-compare
+@itemx -Wno-switch
+@itemx -Wno-missing-field-initializers
+These are warnings which might be useful for @value{GDBN}, but are
+currently too noisy to enable with @samp{-Werror}.
 
-@emph{Pragmatics: @value{GDBN} has not simply accepted the warnings
-enabled by @samp{-Wall -Werror -W...}.  Instead it is selecting warnings
-when and where their benefits can be demonstrated.}
+@end table
 
 @subsection Formatting
 

Re: RFC: Use -Wall -Wextra

[Eli Zaretskii]

> Date: Thu, 28 Dec 2006 14:55:33 -0500
> From: Daniel Jacobowitz <drow@false.org>
> 
> I'd like to hear opinions on this patch.  It changes the default set of GDB
> build warnings from:
> 
> build_warnings="-Wimplicit -Wreturn-type -Wcomment -Wtrigraphs \
> -Wformat -Wparentheses -Wpointer-arith -Wformat-nonliteral \
> -Wunused-label -Wunused-function -Wno-pointer-sign"
> 
> to:
> 
> build_warnings="-Wall -Wextra -Wpointer-arith -Wformat-nonliteral \
> -Wno-pointer-sign -Wno-unused-parameter \
> -Wno-unused -Wno-sign-compare -Wno-switch -Wno-missing-field-initializers"

I would agree only if we never try to use -Werror, because with such
aggressive warnings GDB will never build if we add -Werror.

My other fear is that, with GCC becoming more and more picky about
perfectly valid C code, these options will cause the compilation to
become very noisy, but I guess we will hear complaints if that
happens.

> I'd really like to turn on -Wunused too, but it has been off for so long
> that we have a substantial number of unused local variables - it will take
> some work to clean up.

I'd advise against -Wunused: the problems it finds are harmless,
whereas fixing them is not trivial at all, and quite ugly in some
cases.

> Index: doc/gdbint.texinfo
> ===================================================================
> RCS file: /cvs/src/src/gdb/doc/gdbint.texinfo,v
> retrieving revision 1.247
> diff -u -p -r1.247 gdbint.texinfo
> --- doc/gdbint.texinfo	10 Nov 2006 19:20:37 -0000	1.247
> +++ doc/gdbint.texinfo	28 Dec 2006 19:39:07 -0000

This is fine with me, thanks.

Re: RFC: Use -Wall -Wextra

[Daniel Jacobowitz]

On Fri, Dec 29, 2006 at 02:13:16PM +0200, Eli Zaretskii wrote:
> > Date: Thu, 28 Dec 2006 14:55:33 -0500
> > From: Daniel Jacobowitz <drow@false.org>
> > 
> > I'd like to hear opinions on this patch.  It changes the default set of GDB
> > build warnings from:
> > 
> > build_warnings="-Wimplicit -Wreturn-type -Wcomment -Wtrigraphs \
> > -Wformat -Wparentheses -Wpointer-arith -Wformat-nonliteral \
> > -Wunused-label -Wunused-function -Wno-pointer-sign"
> > 
> > to:
> > 
> > build_warnings="-Wall -Wextra -Wpointer-arith -Wformat-nonliteral \
> > -Wno-pointer-sign -Wno-unused-parameter \
> > -Wno-unused -Wno-sign-compare -Wno-switch -Wno-missing-field-initializers"
> 
> I would agree only if we never try to use -Werror, because with such
> aggressive warnings GDB will never build if we add -Werror.
> 
> My other fear is that, with GCC becoming more and more picky about
> perfectly valid C code, these options will cause the compilation to
> become very noisy, but I guess we will hear complaints if that
> happens.

I don't understand.  I built GDB with these warnings and -Werror, so
that can't be literally true.  I definitely don't want to back away
from -Werror: I think it's done us a lot of good.  Binutils and GCC
already build by default using -Wall -Werror, GCC already uses -Wextra
-Werror, and we already use -Wpointer-arith and -Wformat-nonliteral
with -Werror; only -Wextra is really new for us.

I enabled -Wextra mostly because I wanted -Walways-true; some of the
others are useful too.  For instance, the warning about empty bodies in
if statements is in -Wextra and found a real bug in GDB.  Some of the
-Wextra warnings are a bit dubious, but most of the dubious ones can be
disabled... which is what I've done above.

Can you give me some concrete examples of warnings you're concerned about?

(Remember, we ship releases without -Werror.)

> > I'd really like to turn on -Wunused too, but it has been off for so long
> > that we have a substantial number of unused local variables - it will take
> > some work to clean up.
> 
> I'd advise against -Wunused: the problems it finds are harmless,
> whereas fixing them is not trivial at all, and quite ugly in some
> cases.

I'm afraid I don't understand this either.  The problems -Wunused finds
are usually very easy to fix.  We already use -Wunused-label and
-Wunused-function. (oops!  My patch disabled those because I added
-Wunused at the last minute!  I need to fix that.) That leaves unused
parameters, unused values, and unused local variables. I don't really
agree with the rationale in gdbint about why we don't want the unused
parameter warnings, and binutils made the opposite choice, but in any
case I left it disabled.  And we avoid macros and conditional
compilation, so unused local variables and values are quite easy to
fix.

Anyway, if you want a weaker set of warnings, I'll go for that too.
I can still configure with more aggressive ones here and fix the
problems without them getting in anyone else's way.  -Wunused in
particular - that's basically garbage collection.  I think the average
GDB source file has two unused local variables, and we have a lot
of source files.

-- 
Daniel Jacobowitz
CodeSourcery

Re: RFC: Use -Wall -Wextra

[Eli Zaretskii]

> Date: Fri, 29 Dec 2006 08:58:15 -0500
> From: Daniel Jacobowitz <drow@false.org>
> Cc: gdb-patches@sourceware.org
> > 
> > I would agree only if we never try to use -Werror, because with such
> > aggressive warnings GDB will never build if we add -Werror.
> > 
> > My other fear is that, with GCC becoming more and more picky about
> > perfectly valid C code, these options will cause the compilation to
> > become very noisy, but I guess we will hear complaints if that
> > happens.
> 
> I don't understand.  I built GDB with these warnings and -Werror, so
> that can't be literally true.

Well, it is on this machine:

  Linux fencepost 2.6.16.29-xen #1 SMP Wed Dec 6 07:32:36 EST 2006 x86_64 GNU/Linux

See below for the error message.

> I definitely don't want to back away from -Werror: I think it's done
> us a lot of good.

I'm not against -Werror, I just don't trust GCC to not flag some
innocent code as questionable under -Wall -Wextra.  These flags are
for those who intentionally want a noisy compiler; adding -Werror to
them is asking for too much trouble.

> I enabled -Wextra mostly because I wanted -Walways-true

IMO -Walways-true is one of the greatest nuisances with the latest GCC
versions.  Some code, especially sophisticated macros that need to
work portably with different data sizes, simply cannot be made to work
around this warning, especially if you want to handle 32-bit and
64-bit machines.  There already are such problems in GNU Make and in
Emacs, and developers are unwilling to fix them because it's too much
trouble, if at all possible.

> Can you give me some concrete examples of warnings you're concerned about?

Those issued by -Walways-true and the ones that wine about mismatch
between pointers to signed and unsigned char are the two that come to
mind.

> (Remember, we ship releases without -Werror.)

Thank God for small favors!

> > I'd advise against -Wunused: the problems it finds are harmless,
> > whereas fixing them is not trivial at all, and quite ugly in some
> > cases.
> 
> I'm afraid I don't understand this either.  The problems -Wunused finds
> are usually very easy to fix.

You mean with "i = i;"?

Here's the problem that prevented the latest snapshot from building
after I patched it with your patches:

    gcc -c -g -O2    -I. -I. -I./config -DLOCALEDIR="\"/usr/local/share/locale\"" -DHAVE_CONFIG_H -I./../include/opcode -I./../readline/.. -I../bfd -I./../bfd -I./../include   -DMI_OUT=1 -DTUI=1  -Wall -Wextra -Wpointer-arith -Wformat-nonliteral -Wno-pointer-sign -Wno-unused-parameter -Wno-unused -Wno-sign-compare -Wno-switch -Wno-missing-field-initializers -Werror infrun.c
    cc1: warnings being treated as errors
    infrun.c: In function 'handle_inferior_event':
    infrun.c:1458: warning: statement with no effect
    make[2]: *** [infrun.o] Error 1

Re: RFC: Use -Wall -Wextra

[Daniel Jacobowitz]

On Fri, Dec 29, 2006 at 10:44:25PM +0200, Eli Zaretskii wrote:
> > I definitely don't want to back away from -Werror: I think it's done
> > us a lot of good.
> 
> I'm not against -Werror, I just don't trust GCC to not flag some
> innocent code as questionable under -Wall -Wextra.  These flags are
> for those who intentionally want a noisy compiler; adding -Werror to
> them is asking for too much trouble.

How about -Wall without -Wextra then?  This is the compromise adopted
by binutils and GCC.

> > I enabled -Wextra mostly because I wanted -Walways-true
> 
> IMO -Walways-true is one of the greatest nuisances with the latest GCC
> versions.  Some code, especially sophisticated macros that need to
> work portably with different data sizes, simply cannot be made to work
> around this warning, especially if you want to handle 32-bit and
> 64-bit machines.  There already are such problems in GNU Make and in
> Emacs, and developers are unwilling to fix them because it's too much
> trouble, if at all possible.

I think neither of us is actually talking about -Walways-true.  It
doesn't seem to control what I thought it did.  Anyway, I was looking
for the "comparison of unsigned >= 0 is always true" warning
specifically.  The only thing controlling that is -Wextra.  I suspect
that's the one you are concerned about, too.

I'd like to enable that warning but I won't cry if we agree not to. It
did find one bug in GDB already, and would have found another if it had
been enabled (the ia64-tdep bug I fixed yesterday).  But I can build
with it locally on systems where I know it isn't a big problem.

> Those issued by -Walways-true and the ones that wine about mismatch
> between pointers to signed and unsigned char are the two that come to
> mind.

I actually think the latter is useful, but not useful enough to
continue fixing up GDB for it - my patch left that one disabled,
deliberately.  Let's not go back there again, it was a real nuisance.

> > > I'd advise against -Wunused: the problems it finds are harmless,
> > > whereas fixing them is not trivial at all, and quite ugly in some
> > > cases.
> > 
> > I'm afraid I don't understand this either.  The problems -Wunused finds
> > are usually very easy to fix.
> 
> You mean with "i = i;"?

No, either by deleting the unused local variable if it's truly unused,
or by adding ATTRIBUTE_UNUSED if it is conditionally unused (or
avoiding conditional compilation).

>     gcc -c -g -O2    -I. -I. -I./config -DLOCALEDIR="\"/usr/local/share/locale\"" -DHAVE_CONFIG_H -I./../include/opcode -I./../readline/.. -I../bfd -I./../bfd -I./../include   -DMI_OUT=1 -DTUI=1  -Wall -Wextra -Wpointer-arith -Wformat-nonliteral -Wno-pointer-sign -Wno-unused-parameter -Wno-unused -Wno-sign-compare -Wno-switch -Wno-missing-field-initializers -Werror infrun.c
>     cc1: warnings being treated as errors
>     infrun.c: In function 'handle_inferior_event':
>     infrun.c:1458: warning: statement with no effect
>     make[2]: *** [infrun.o] Error 1

Thanks.  That comes from the default definition of a macro which no
longer has any non-default definitions; we may as well garbage collect
it.  I don't know why I didn't get the warning; I can provoke it for
a small testcase.

I'll remove the macro, since that's an unrelated cleanup.

-- 
Daniel Jacobowitz
CodeSourcery

Re: RFC: Use -Wall -Wextra

[Eli Zaretskii]

> Date: Fri, 29 Dec 2006 16:02:06 -0500
> From: Daniel Jacobowitz <drow@false.org>
> Cc: gdb-patches@sourceware.org
> 
> How about -Wall without -Wextra then?

Fine with me.

> I think neither of us is actually talking about -Walways-true.

Maybe.  I was talking about this one (happens with yesterday's
snapshot):

    gcc -c -DHAVE_CONFIG_H    -I. -I.././readline -DRL_LIBRARY_VERSION='"5.1"' -g -O2 bind.c
    bind.c: In function 'rl_function_of_keyseq':
    bind.c:682: warning: comparison is always true due to limited range of data type
    rm -f display.o
    gcc -c -DHAVE_CONFIG_H    -I. -I.././readline -DRL_LIBRARY_VERSION='"5.1"' -g -O2 display.c
    display.c: In function 'rl_character_len':
    display.c:1844: warning: comparison is always true due to limited range of data type

It looks like this happens even without -Wall.  What a screwup!

> >     gcc -c -g -O2    -I. -I. -I./config -DLOCALEDIR="\"/usr/local/share/locale\"" -DHAVE_CONFIG_H -I./../include/opcode -I./../readline/.. -I../bfd -I./../bfd -I./../include   -DMI_OUT=1 -DTUI=1  -Wall -Wextra -Wpointer-arith -Wformat-nonliteral -Wno-pointer-sign -Wno-unused-parameter -Wno-unused -Wno-sign-compare -Wno-switch -Wno-missing-field-initializers -Werror infrun.c
> >     cc1: warnings being treated as errors
> >     infrun.c: In function 'handle_inferior_event':
> >     infrun.c:1458: warning: statement with no effect
> >     make[2]: *** [infrun.o] Error 1
> 
> Thanks.  That comes from the default definition of a macro which no
> longer has any non-default definitions; we may as well garbage collect
> it.  I don't know why I didn't get the warning; I can provoke it for
> a small testcase.
> 
> I'll remove the macro, since that's an unrelated cleanup.

I'll wait for the patch, thanks.

Re: RFC: Use -Wall -Wextra

[Daniel Jacobowitz]

On Sat, Dec 30, 2006 at 05:32:26PM +0200, Eli Zaretskii wrote:
> > I think neither of us is actually talking about -Walways-true.
> 
> Maybe.  I was talking about this one (happens with yesterday's
> snapshot):
> 
>     gcc -c -DHAVE_CONFIG_H    -I. -I.././readline -DRL_LIBRARY_VERSION='"5.1"' -g -O2 bind.c
>     bind.c: In function 'rl_function_of_keyseq':
>     bind.c:682: warning: comparison is always true due to limited range of data type
>     rm -f display.o
>     gcc -c -DHAVE_CONFIG_H    -I. -I.././readline -DRL_LIBRARY_VERSION='"5.1"' -g -O2 display.c
>     display.c: In function 'rl_character_len':
>     display.c:1844: warning: comparison is always true due to limited range of data type
> 
> It looks like this happens even without -Wall.  What a screwup!

OK, that's a whole different one - and yes, it's unconditional.  I
meant this one:

warning: comparison of unsigned expression >= 0 is always true

That's controlled by -Wextra directly.

Anyway, let's go with -Wall; that's still an improvement over the
present state and fixes the annoying -Wuninitialized warnings.  I'll
commit my saved up warning fixes, and then post a patch to do this.

> > Thanks.  That comes from the default definition of a macro which no
> > longer has any non-default definitions; we may as well garbage collect
> > it.  I don't know why I didn't get the warning; I can provoke it for
> > a small testcase.
> > 
> > I'll remove the macro, since that's an unrelated cleanup.
> 
> I'll wait for the patch, thanks.

Done now, in a separate message.

-- 
Daniel Jacobowitz
CodeSourcery

Re: RFC: Use -Wall -Wextra

[Eli Zaretskii]

> Date: Sat, 30 Dec 2006 11:00:34 -0500
> From: Daniel Jacobowitz <drow@false.org>
> Cc: gdb-patches@sourceware.org
> 
> > > Thanks.  That comes from the default definition of a macro which no
> > > longer has any non-default definitions; we may as well garbage collect
> > > it.  I don't know why I didn't get the warning; I can provoke it for
> > > a small testcase.
> > > 
> > > I'll remove the macro, since that's an unrelated cleanup.
> > 
> > I'll wait for the patch, thanks.
> 
> Done now, in a separate message.

Thanks.  Building now...

Re: RFC: Use -Wall -Wextra

[Daniel Jacobowitz]

On Sat, Dec 30, 2006 at 11:00:34AM -0500, Daniel Jacobowitz wrote:
> Anyway, let's go with -Wall; that's still an improvement over the
> present state and fixes the annoying -Wuninitialized warnings.  I'll
> commit my saved up warning fixes, and then post a patch to do this.

Here's what I have just tested on x86_64-pc-linux-gnu.  This is a much
more conservative patch than my previous one.  It may still break other
targets, especially native ones, with -Werror; I'll be happy to help
fix any breakage if the fixes aren't apparent.

OK to commit?

-- 
Daniel Jacobowitz
CodeSourcery

2007-01-03  Daniel Jacobowitz  <dan@codesourcery.com>

	* configure.ac (build_warnings): Use -Wall and
	-Wdeclaration-after-statement.
	* configure: Regenerated.

2007-01-03  Daniel Jacobowitz  <dan@codesourcery.com>

	* doc/gdbint.texinfo (Compiler Warnings): Update for -Wall use.

Index: configure.ac
===================================================================
RCS file: /cvs/src/src/gdb/configure.ac,v
retrieving revision 1.37
diff -u -p -r1.37 configure.ac
--- configure.ac	31 Dec 2006 20:20:13 -0000	1.37
+++ configure.ac	3 Jan 2007 19:09:53 -0000
@@ -1138,32 +1138,15 @@ if test "${ERROR_ON_WARNING}" = yes ; th
     WERROR_CFLAGS="-Werror"
 fi
 
-# NOTE: Don't add -Wall or -Wunused, they both include
-# -Wunused-parameter which reports bogus warnings.
-# NOTE: If you add to this list, remember to update
+# The entries after -Wno-pointer-sign are disabled warnings which may
+# be enabled in the future, which can not currently be used to build
+# GDB.
+# NOTE: If you change this list, remember to update
 # gdb/doc/gdbint.texinfo.
-build_warnings="-Wimplicit -Wreturn-type -Wcomment -Wtrigraphs \
--Wformat -Wparentheses -Wpointer-arith -Wformat-nonliteral \
--Wunused-label -Wunused-function -Wno-pointer-sign"
-
-# GCC supports -Wuninitialized only with -O or -On, n != 0.
-if test x${CFLAGS+set} = xset; then
-  case "${CFLAGS}" in
-    *"-O0"* ) ;;
-    *"-O"* )
-      build_warnings="${build_warnings} -Wuninitialized"
-    ;;
-  esac
-else
-  build_warnings="${build_warnings} -Wuninitialized"
-fi
+build_warnings="-Wall -Wdeclaration-after-statement -Wpointer-arith \
+-Wformat-nonliteral -Wno-pointer-sign \
+-Wno-unused -Wno-switch"
 
-# Up for debate: -Wswitch -Wcomment -trigraphs -Wtrigraphs
-# -Wunused-function -Wunused-variable -Wunused-value
-# -Wchar-subscripts -Wtraditional -Wshadow -Wcast-qual
-# -Wcast-align -Wwrite-strings -Wconversion -Wstrict-prototypes
-# -Wmissing-prototypes -Wmissing-declarations -Wredundant-decls
-# -Woverloaded-virtual -Winline -Werror"
 AC_ARG_ENABLE(build-warnings,
 [  --enable-build-warnings Enable build-time compiler warnings if gcc is used],
 [case "${enableval}" in
@@ -1208,7 +1191,7 @@ then
 	    CFLAGS="$saved_CFLAGS"
 	esac
     done
-    AC_MSG_RESULT(${WARN_CFLAGS}${WERROR_CFLAGS})
+    AC_MSG_RESULT(${WARN_CFLAGS} ${WERROR_CFLAGS})
 fi
 AC_SUBST(WARN_CFLAGS)
 AC_SUBST(WERROR_CFLAGS)
Index: doc/gdbint.texinfo
===================================================================
RCS file: /cvs/src/src/gdb/doc/gdbint.texinfo,v
retrieving revision 1.248
diff -u -p -r1.248 gdbint.texinfo
--- doc/gdbint.texinfo	1 Jan 2007 14:04:18 -0000	1.248
+++ doc/gdbint.texinfo	3 Jan 2007 19:09:55 -0000
@@ -5471,63 +5471,34 @@ errors.}
 @subsection Compiler Warnings
 @cindex compiler warnings
 
-With few exceptions, developers should include the configuration option
-@samp{--enable-gdb-build-warnings=,-Werror} when building @value{GDBN}.
-The exceptions are listed in the file @file{gdb/MAINTAINERS}.
+With few exceptions, developers should avoid the configuration option
+@samp{--disable-werror} when building @value{GDBN}.  The exceptions
+are listed in the file @file{gdb/MAINTAINERS}.  The default, when
+building with @sc{gcc}, is @samp{--enable-werror}.
 
 This option causes @value{GDBN} (when built using GCC) to be compiled
 with a carefully selected list of compiler warning flags.  Any warnings
-from those flags being treated as errors.
+from those flags are treated as errors.
 
 The current list of warning flags includes:
 
 @table @samp
-@item -Wimplicit
-Since @value{GDBN} coding standard requires all functions to be declared
-using a prototype, the flag has the side effect of ensuring that
-prototyped functions are always visible with out resorting to
-@samp{-Wstrict-prototypes}.
-
-@item -Wreturn-type
-Such code often appears to work except on instruction set architectures
-that use register windows.
+@item -Wall
+Recommended @sc{gcc} warnings.
 
-@item -Wcomment
+@item -Wdeclaration-after-statement
 
-@item -Wtrigraphs
-
-@item -Wformat
-@itemx -Wformat-nonliteral
-Since @value{GDBN} uses the @code{format printf} attribute on all
-@code{printf} like functions these check not just @code{printf} calls
-but also calls to functions such as @code{fprintf_unfiltered}.
-
-@item -Wparentheses
-This warning includes uses of the assignment operator within an
-@code{if} statement.
+@sc{gcc} 3.x (and later) and @sc{c99} allow declarations mixed with
+code, but @sc{gcc} 2.x and @sc{c89} do not.
 
 @item -Wpointer-arith
 
-@item -Wuninitialized
-
-@item -Wunused-label
-This warning has the additional benefit of detecting the absence of the
-@code{case} reserved word in a switch statement:
-@smallexample
-enum @{ FD_SCHEDULED, NOTHING_SCHEDULED @} sched;
-@dots{}
-switch (sched)
-  @{
-  case FD_SCHEDULED:
-    @dots{};
-    break;
-  NOTHING_SCHEDULED:
-    @dots{};
-    break;
-  @}
-@end smallexample
-
-@item -Wunused-function
+@item -Wformat-nonliteral
+Non-literal format strings, with a few exceptions, are bugs - they
+might contain unintented user-supplied format specifiers.
+Since @value{GDBN} uses the @code{format printf} attribute on all
+@code{printf} like functions this checks not just @code{printf} calls
+but also calls to functions such as @code{fprintf_unfiltered}.
 
 @item -Wno-pointer-sign
 In version 4.0, GCC began warning about pointer argument passing or
@@ -5537,19 +5508,19 @@ carefully between @code{char} and @code{
 the @value{GDBN} developers decided correcting these warnings wasn't
 worth the time it would take.
 
-@end table
-
-@emph{Pragmatics: Due to the way that @value{GDBN} is implemented most
-functions have unused parameters.  Consequently the warning
-@samp{-Wunused-parameter} is precluded from the list.  The macro
+@item -Wno-unused-parameter
+Due to the way that @value{GDBN} is implemented many functions have
+unused parameters.  Consequently this warning is avoided.  The macro
 @code{ATTRIBUTE_UNUSED} is not used as it leads to false negatives ---
 it is not an error to have @code{ATTRIBUTE_UNUSED} on a parameter that
-is being used.  The options @samp{-Wall} and @samp{-Wunused} are also
-precluded because they both include @samp{-Wunused-parameter}.}
+is being used.
+
+@item -Wno-unused
+@itemx -Wno-switch
+These are warnings which might be useful for @value{GDBN}, but are
+currently too noisy to enable with @samp{-Werror}.
 
-@emph{Pragmatics: @value{GDBN} has not simply accepted the warnings
-enabled by @samp{-Wall -Werror -W...}.  Instead it is selecting warnings
-when and where their benefits can be demonstrated.}
+@end table
 
 @subsection Formatting
 

Re: RFC: Use -Wall -Wextra

[Mark Kettenis]

> Date: Wed, 3 Jan 2007 14:12:25 -0500
> From: Daniel Jacobowitz <drow@false.org>
> 
> On Sat, Dec 30, 2006 at 11:00:34AM -0500, Daniel Jacobowitz wrote:
> > Anyway, let's go with -Wall; that's still an improvement over the
> > present state and fixes the annoying -Wuninitialized warnings.  I'll
> > commit my saved up warning fixes, and then post a patch to do this.
> 
> Here's what I have just tested on x86_64-pc-linux-gnu.  This is a much
> more conservative patch than my previous one.  It may still break other
> targets, especially native ones, with -Werror; I'll be happy to help
> fix any breakage if the fixes aren't apparent.
> 
> OK to commit?

Fine with me.

Re: RFC: Use -Wall -Wextra

[Eli Zaretskii]

> Date: Wed, 3 Jan 2007 14:12:25 -0500
> From: Daniel Jacobowitz <drow@false.org>
> 
> Here's what I have just tested on x86_64-pc-linux-gnu.  This is a much
> more conservative patch than my previous one.  It may still break other
> targets, especially native ones, with -Werror; I'll be happy to help
> fix any breakage if the fixes aren't apparent.
> 
> OK to commit?

Yes, thanks.

Re: RFC: Use -Wall -Wextra

[Daniel Jacobowitz]

On Wed, Jan 03, 2007 at 02:12:25PM -0500, Daniel Jacobowitz wrote:
> 2007-01-03  Daniel Jacobowitz  <dan@codesourcery.com>
> 
> 	* configure.ac (build_warnings): Use -Wall and
> 	-Wdeclaration-after-statement.
> 	* configure: Regenerated.
> 
> 2007-01-03  Daniel Jacobowitz  <dan@codesourcery.com>
> 
> 	* doc/gdbint.texinfo (Compiler Warnings): Update for -Wall use.

Checked in now!

-- 
Daniel Jacobowitz
CodeSourcery