[RFA] Turn on -Werror by default

[RFA] Turn on -Werror by default

[Mark Kettenis]

A while ago, we basically agreed that this would be a good thing to do
after the release.  I tried to steal the code form BFD, but that
didn't really fit in very well in our configure.ac.  Anyway, this adds
--enable-werror, defaulting to "yes", so to turn it off you'll
probably want to use --disable-werror.  This is similar to what BFD
does, so you can easily disable -Werror for the whole tree.  To
disable -Werror for GDB only, you can use
--enable-gdb-build-warnings=,-Wno-error.

ok?

Mark


Index: configure.ac
===================================================================
RCS file: /cvs/src/src/gdb/configure.ac,v
retrieving revision 1.25
diff -u -p -r1.25 configure.ac
--- configure.ac	17 Dec 2005 22:33:59 -0000	1.25
+++ configure.ac	8 Jan 2006 17:54:43 -0000
@@ -1109,6 +1109,24 @@ AC_ARG_WITH(sysroot,
 AC_SUBST(TARGET_SYSTEM_ROOT)
 AC_SUBST(TARGET_SYSTEM_ROOT_DEFINE)
 
+AC_ARG_ENABLE(werror,
+  [  --enable-werror    treat compile warnings as errors],
+  [case "${enableval}" in
+     yes | y) ERROR_ON_WARNING="yes" ;;
+     no | n)  ERROR_ON_WARNING="no" ;;
+     *) AC_MSG_ERROR(bad value ${enableval} for --enable-werror) ;;
+   esac])
+
+# Enable -Werror by default when using gcc
+if test "${GCC}" = yes -a -z "${ERROR_ON_WARNING}" ; then
+    ERROR_ON_WARNING=yes
+fi
+
+WERROR_CFLAGS=""
+if test "${ERROR_ON_WARNING}" = yes ; then
+    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
@@ -1164,7 +1182,6 @@ if test x"$silent" != x"yes" && test x"$
   echo "Setting GDB specific compiler warning flags = $build_warnings" 6>&1
 fi])dnl
 WARN_CFLAGS=""
-WERROR_CFLAGS=""
 if test "x${build_warnings}" != x -a "x$GCC" = xyes
 then
     AC_MSG_CHECKING(compiler warning flags)

Re: [RFA] Turn on -Werror by default

[Daniel Jacobowitz]

On Sun, Jan 08, 2006 at 06:59:24PM +0100, Mark Kettenis wrote:
> A while ago, we basically agreed that this would be a good thing to do
> after the release.  I tried to steal the code form BFD, but that
> didn't really fit in very well in our configure.ac.  Anyway, this adds
> --enable-werror, defaulting to "yes", so to turn it off you'll
> probably want to use --disable-werror.  This is similar to what BFD
> does, so you can easily disable -Werror for the whole tree.  To
> disable -Werror for GDB only, you can use
> --enable-gdb-build-warnings=,-Wno-error.
> 
> ok?

Unless I've missed quite a lot, all Linux builds and targets are still
noisy; can we add it but default to disabled for now?  Maybe that'll
give someone (me?) incentive to finish the cleanup.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] Turn on -Werror by default

[Mark Kettenis]

> Date: Sun, 8 Jan 2006 17:25:47 -0500
> From: Daniel Jacobowitz <drow@false.org>
> 
> On Sun, Jan 08, 2006 at 06:59:24PM +0100, Mark Kettenis wrote:
> > A while ago, we basically agreed that this would be a good thing to do
> > after the release.  I tried to steal the code form BFD, but that
> > didn't really fit in very well in our configure.ac.  Anyway, this adds
> > --enable-werror, defaulting to "yes", so to turn it off you'll
> > probably want to use --disable-werror.  This is similar to what BFD
> > does, so you can easily disable -Werror for the whole tree.  To
> > disable -Werror for GDB only, you can use
> > --enable-gdb-build-warnings=,-Wno-error.
> > 
> > ok?
> 
> Unless I've missed quite a lot, all Linux builds and targets are still
> noisy; can we add it but default to disabled for now?  Maybe that'll
> give someone (me?) incentive to finish the cleanup.

Sorry, but I think we've waited long enough.  GCC 4 has been out for 9
months now, so people have had plenty of time to fix issues with it.
In fact, I think people have largely ignored its problems because we
don't enable -Werror by default.  I bet enabling it, will make people
actually fix the problems.

If you're talking about Linux builds that fall over with -Werror even
with older GCC versions, then there is no real excuse.  GDB
maintainers should have been compiling with -Werror for at least the
past four years.

As a compromise, I'm willing to hold off checking in this patch (or
check it in with -enable-werror defaulting to "no"), if we set a fixed
date for enabling it not too far in the future.  The 1st of February
should give people enough time to sort out the last GCC 4 issues.

Mark

Re: [RFA] Turn on -Werror by default

[Daniel Jacobowitz]

On Sun, Jan 08, 2006 at 11:53:21PM +0100, Mark Kettenis wrote:
> Sorry, but I think we've waited long enough.  GCC 4 has been out for 9
> months now, so people have had plenty of time to fix issues with it.
> In fact, I think people have largely ignored its problems because we
> don't enable -Werror by default.  I bet enabling it, will make people
> actually fix the problems.

What version of GCC are you using?  The first of 189 GCC 4 warnings in
my builds comes from source.c (and is a little non-obvious to fix, I'm
working on it).  Exactly one warning is in Linux-specific code.

I definitely object if you're going to check in a patch that makes GDB
build nowhere by default.

> If you're talking about Linux builds that fall over with -Werror even
> with older GCC versions, then there is no real excuse.  GDB
> maintainers should have been compiling with -Werror for at least the
> past four years.

FYI, most of the -Werror problems in Linux builds with older compilers
are specifically fallout of fixes for the stupid GCC 4 warnings.  It
_used_ to build with -Werror until mass interface changes started
breaking it.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] Turn on -Werror by default

[Mark Kettenis]

> Date: Sun, 15 Jan 2006 12:01:03 -0500
> From: Daniel Jacobowitz <drow@false.org>
> 
> On Sun, Jan 08, 2006 at 11:53:21PM +0100, Mark Kettenis wrote:
> > Sorry, but I think we've waited long enough.  GCC 4 has been out for 9
> > months now, so people have had plenty of time to fix issues with it.
> > In fact, I think people have largely ignored its problems because we
> > don't enable -Werror by default.  I bet enabling it, will make people
> > actually fix the problems.
> 
> What version of GCC are you using?  The first of 189 GCC 4 warnings in
> my builds comes from source.c (and is a little non-obvious to fix, I'm
> working on it).  Exactly one warning is in Linux-specific code.

OpenBSD comes with GCC 3.3.5 or GCC 2.95.3, so that's what I use to
test all the OpenBSD targets.  FreeBSD and NetBSD still ship with GCC
3.4.x, even in -CURRENT, wo that's what I use for testing those
targets.  Solaris 10 has GCC 3.4.2.  Even most Linux systems out there
will still use GCC 3.  At work the first machine with SuSE 10 cam in a
week ago (and we get new machines every couple of months or so).  All
older machines still have GCC 3.

From time to time I play with GCC 4, and try to fix a few problems,
but I get the feeling I'm the only one.

The source.c warning has to do with line numbers isn't it?  I spent
some time on tracking that down too.  Simply changing signed integers
into unsigned integers makes the compiler happy, but results in GDB
being broken.  This fact alone should make clear that there are cases
where these new warnings are actually useful.

> I definitely object if you're going to check in a patch that makes GDB
> build nowhere by default.

I'm certainly not proposing to do that; even with -Werror enabled, GDB
builds fine on most systems.  And when we enable -Werror, we should
seriously consider turning it off again before release.

> > If you're talking about Linux builds that fall over with -Werror even
> > with older GCC versions, then there is no real excuse.  GDB
> > maintainers should have been compiling with -Werror for at least the
> > past four years.
> 
> FYI, most of the -Werror problems in Linux builds with older compilers
> are specifically fallout of fixes for the stupid GCC 4 warnings.  It
> _used_ to build with -Werror until mass interface changes started
> breaking it.

There certainly has been some fallout, and a significant amount off my
commits since April have been devoted to fixing those.  It usually is
just a few minutes to fix them.  Unfortunately I don't own any arm
hardware, so I don't regularly compile things for arm.  

However, over the past few months I've fixed more than a few problems
that were introduced by people contributing new code.  They clearly
are not using -Werror, and therefore losing a valuable tool to catch
bugs.

Mark

Re: [RFA] Turn on -Werror by default

[Daniel Jacobowitz]

On Sun, Jan 15, 2006 at 06:55:28PM +0100, Mark Kettenis wrote:
> From time to time I play with GCC 4, and try to fix a few problems,
> but I get the feeling I'm the only one.

You may be the only one who prioritizes fixing these warnings, which I
still maintain are mostly useless.  I think your sample is seriously
biased.  A few points:
  - You're the only active GDB developer I can think of offhand that
    considers the warnings a serious problem.  I do consider them a
    problem - one which should be fixed, before we make policy
    statements about them.
  - You're primarily focused on the BSDs, and the BSD system compilers
    can currently build GDB without warning.
  - Debian/unstable and Fedora Core, both popular development
    platforms, can not.
  - Many people doing development on GDB HEAD are likely to have GCC
    HEAD lying around, which also can't build GDB with -Werror.

This patch is a policy statement that the GDB developers agree with you
on the importance of warning-free code, which will inconvenience you
not at all, and me a great deal.  Can you see where I'm coming from?

I maintain that the correct way to turn on -Werror is to first fix the
warnings.  As the developer who thinks -Werror is an important step
forward, the burden is on you to make GDB warning-free on a reasonable
set of platforms - I think we both agree on that already.  I'm
disputing your reasonable set of platforms, however.

GCC 4 is unavoidably the future.  Ignoring it here won't make it go
away, it will just force someone else to clean up after you.

> I'm certainly not proposing to do that; even with -Werror enabled, GDB
> builds fine on most systems.  And when we enable -Werror, we should
> seriously consider turning it off again before release.

I disagree with "most" in this paragraph.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] Turn on -Werror by default

[Mark Kettenis]

> Date: Sun, 15 Jan 2006 13:20:55 -0500
> From: Daniel Jacobowitz <drow@false.org>
> 
> On Sun, Jan 15, 2006 at 06:55:28PM +0100, Mark Kettenis wrote:
> > From time to time I play with GCC 4, and try to fix a few problems,
> > but I get the feeling I'm the only one.
> 
> You may be the only one who prioritizes fixing these warnings, which I
> still maintain are mostly useless.  I think your sample is seriously
> biased.  A few points:
>   - You're the only active GDB developer I can think of offhand that
>     considers the warnings a serious problem.  I do consider them a
>     problem - one which should be fixed, before we make policy
>     statements about them.

Sorry, but you misunderstood me here.  I do not consider these *new*
GCC 4 warnings a problem.  What I do consider to be a problem is that
it makes people ignore other, useful warnings.

>   - You're primarily focused on the BSDs, and the BSD system compilers
>     can currently build GDB without warning.

Indeed.

>   - Debian/unstable and Fedora Core, both popular development
>     platforms, can not.

It truly amazes me that they are popular develepment platforms for the
average application developer (as opposed to Debian and Fedora Core
developers).  That shows that these people don't understand what
backwards compatibility means, and fully explains why so many
applications don't even compile on my Linux desktop system at work.

>   - Many people doing development on GDB HEAD are likely to have GCC
>     HEAD lying around, which also can't build GDB with -Werror.

True.  And in fact it is important for us to make sure GDB can be used
to debug code compiled with at least the latest official release of
GCC.  Tracking GCC HEAD, and using it to compile GDB, may very well be
the only way to achieve that goal.

> This patch is a policy statement that the GDB developers agree with you
> on the importance of warning-free code, which will inconvenience you
> not at all, and me a great deal.  Can you see where I'm coming from?

Yes.  But warning-free code used to be the policy back in the GCC 3
days.  I'm not aware that we dropped that policy; MAINTAINERS still
lists -Werror in the "Target ISA" section.

> I maintain that the correct way to turn on -Werror is to first fix the
> warnings.  As the developer who thinks -Werror is an important step
> forward, the burden is on you to make GDB warning-free on a reasonable
> set of platforms - I think we both agree on that already.  I'm
> disputing your reasonable set of platforms, however.

Since we also seem to agree that the "warn for pointer argument
passing or assignment with different signedness" warning from GCC 4 is
mostly pedantry, how about the attached patch, which adds
-Wno-pointer-sign" to the mix.

I can add that bit first, and then when we've fixed the few remaining
warnings, we can enable -Werror.

Mark


Index: configure.ac
===================================================================
RCS file: /cvs/src/src/gdb/configure.ac,v
retrieving revision 1.25
diff -u -p -r1.25 configure.ac
--- configure.ac	17 Dec 2005 22:33:59 -0000	1.25
+++ configure.ac	15 Jan 2006 20:00:55 -0000
@@ -1109,6 +1109,24 @@ AC_ARG_WITH(sysroot,
 AC_SUBST(TARGET_SYSTEM_ROOT)
 AC_SUBST(TARGET_SYSTEM_ROOT_DEFINE)
 
+AC_ARG_ENABLE(werror,
+  [  --enable-werror    treat compile warnings as errors],
+  [case "${enableval}" in
+     yes | y) ERROR_ON_WARNING="yes" ;;
+     no | n)  ERROR_ON_WARNING="no" ;;
+     *) AC_MSG_ERROR(bad value ${enableval} for --enable-werror) ;;
+   esac])
+
+# Enable -Werror by default when using gcc
+if test "${GCC}" = yes -a -z "${ERROR_ON_WARNING}" ; then
+    ERROR_ON_WARNING=yes
+fi
+
+WERROR_CFLAGS=""
+if test "${ERROR_ON_WARNING}" = yes ; then
+    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
@@ -1116,6 +1134,9 @@ AC_SUBST(TARGET_SYSTEM_ROOT_DEFINE)
 build_warnings="-Wimplicit -Wreturn-type -Wcomment -Wtrigraphs \
 -Wformat -Wparentheses -Wpointer-arith -Wformat-nonliteral \
 -Wunused-label -Wunused-function"
+# NOTE: GCC 4 emits some warnings that are not very useful.  We need
+# to clean up our source tree, but until we do disabled them.
+build_warnings="${build_warnings} -Wno-pointer-sign"
 
 # GCC supports -Wuninitialized only with -O or -On, n != 0.
 if test x${CFLAGS+set} = xset; then
@@ -1164,7 +1185,6 @@ if test x"$silent" != x"yes" && test x"$
   echo "Setting GDB specific compiler warning flags = $build_warnings" 6>&1
 fi])dnl
 WARN_CFLAGS=""
-WERROR_CFLAGS=""
 if test "x${build_warnings}" != x -a "x$GCC" = xyes
 then
     AC_MSG_CHECKING(compiler warning flags)

Re: [RFA] Turn on -Werror by default

[Daniel Jacobowitz]

On Sun, Jan 15, 2006 at 09:04:01PM +0100, Mark Kettenis wrote:
> > This patch is a policy statement that the GDB developers agree with you
> > on the importance of warning-free code, which will inconvenience you
> > not at all, and me a great deal.  Can you see where I'm coming from?
> 
> Yes.  But warning-free code used to be the policy back in the GCC 3
> days.  I'm not aware that we dropped that policy; MAINTAINERS still
> lists -Werror in the "Target ISA" section.

When we started committing incomplete patches to address bits of GCC 4
warnings without regards for GCC 3 builds, we dropped this policy in
practice.

> > I maintain that the correct way to turn on -Werror is to first fix the
> > warnings.  As the developer who thinks -Werror is an important step
> > forward, the burden is on you to make GDB warning-free on a reasonable
> > set of platforms - I think we both agree on that already.  I'm
> > disputing your reasonable set of platforms, however.
> 
> Since we also seem to agree that the "warn for pointer argument
> passing or assignment with different signedness" warning from GCC 4 is
> mostly pedantry, how about the attached patch, which adds
> -Wno-pointer-sign" to the mix.
> 
> I can add that bit first, and then when we've fixed the few remaining
> warnings, we can enable -Werror.

I think we do need to fix the pointer sign warnings eventually, as a
code cleanliness issue, so perhaps this is a good time to do it. 
You've needled me enough this week that I'm actually rolling now :-)
Give me a few days.

BTW, if we were to go with your patch, wouldn't we would need to
conditionalize the warning on an appropriate version of GCC?  Or is
there pre-existing magic for this in $build_warnings?

I'd gotten it into my head that there was no way to turn these off.
Nice to be wrong about that.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] Turn on -Werror by default

[Mark Kettenis]

> Date: Sun, 15 Jan 2006 15:26:01 -0500
> From: Daniel Jacobowitz <drow@false.org>
> 
> > Since we also seem to agree that the "warn for pointer argument
> > passing or assignment with different signedness" warning from GCC 4 is
> > mostly pedantry, how about the attached patch, which adds
> > -Wno-pointer-sign" to the mix.
> > 
> > I can add that bit first, and then when we've fixed the few remaining
> > warnings, we can enable -Werror.
> 
> I think we do need to fix the pointer sign warnings eventually, as a
> code cleanliness issue, so perhaps this is a good time to do it. 
> You've needled me enough this week that I'm actually rolling now :-)
> Give me a few days.

Heh ;-)

> BTW, if we were to go with your patch, wouldn't we would need to
> conditionalize the warning on an appropriate version of GCC?  Or is
> there pre-existing magic for this in $build_warnings?

Yes, there is.

> I'd gotten it into my head that there was no way to turn these off.
> Nice to be wrong about that.

It may very well be that -Wno-pointer-sign, wasn't there in GCC HEAD
when Andrew initially started "fixing" things.

Anyway, I'll keep this patch in my tree, and will bring it up again if
it turns out we need it.  I'll start nagging again in two weeks or so ;-).

Mark