[Qemu-devel] [RFC] Enable Werrror by default

[Qemu-devel] [RFC] Enable Werrror by default

[Paul Brook]

[-- Attachment #1: Type: text/plain, Size: 320 bytes --]

I'd like to enable Werror by default. I've been using --enable-werror locally 
for a while now, and it's been extremely useful in picking up dumb errors 
(like the recent stellaris_enet.c breakage).

Any objections?

You can of course configure with --disable-werror if you really want the force 
things to build.

Paul

[-- Attachment #2: 0001-Enable-Werror-by-default.patch --]
[-- Type: text/x-patch, Size: 771 bytes --]

From 5ccaebfce38fc161a776725c6c945b2173459397 Mon Sep 17 00:00:00 2001
From: Paul Brook <paul@codesourcery.com>
Date: Thu, 11 Jun 2009 13:31:46 +0100
Subject: [PATCH] Enable Werror by default

Signed-off-by: Paul Brook <paul@codesourcery.com>
---
 configure |    6 +-----
 1 files changed, 1 insertions(+), 5 deletions(-)

diff --git a/configure b/configure
index 89e7f53..9e03ee3 100755
--- a/configure
+++ b/configure
@@ -356,11 +356,7 @@ else
 fi
 [ -f "$workdir/vl.c" ] || source_path_used="yes"
 
-werror="no"
-# generate compile errors on warnings for development builds
-#if grep cvs $source_path/VERSION > /dev/null 2>&1 ; then
-#werror="yes";
-#fi
+werror="yes"
 
 for opt do
   optarg=`expr "x$opt" : 'x[^=]*=\(.*\)'`
-- 
1.6.3.1

Re: [Qemu-devel] [RFC] Enable Werrror by default

[Tristan Gingold]

On Jun 11, 2009, at 2:35 PM, Paul Brook wrote:

> I'd like to enable Werror by default. I've been using --enable- 
> werror locally
> for a while now, and it's been extremely useful in picking up dumb  
> errors
> (like the recent stellaris_enet.c breakage).
>
> Any objections?

IIRC, qemu is not warnings clean on Darwin.
But adding -Werror may increase the pressure :-)

Re: [Qemu-devel] [RFC] Enable Werrror by default

[Anthony Liguori]

Paul Brook wrote:
> I'd like to enable Werror by default. I've been using --enable-werror locally 
> for a while now, and it's been extremely useful in picking up dumb errors 
> (like the recent stellaris_enet.c breakage).
>
> Any objections?
>   

It's going to break Windows and more recent versions of GCC.

Regards,

Anthony Liguori

> You can of course configure with --disable-werror if you really want the force 
> things to build.
>
> Paul
>   

Re: [Qemu-devel] [RFC] Enable Werrror by default

[Christoph Egger]

On Thursday 11 June 2009 14:46:18 Tristan Gingold wrote:
> On Jun 11, 2009, at 2:35 PM, Paul Brook wrote:
> > I'd like to enable Werror by default. I've been using --enable-
> > werror locally
> > for a while now, and it's been extremely useful in picking up dumb
> > errors
> > (like the recent stellaris_enet.c breakage).
> >
> > Any objections?
>
> IIRC, qemu is not warnings clean on Darwin.
> But adding -Werror may increase the pressure :-)

pthreads are not -Wshadow or -Wnested-extern warning clean by design
on many OS's.

Christoph

-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Karl-Hammerschmidt-Str. 34, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Thomas M. McCoy, Giuliano Meroni
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

Re: [Qemu-devel] [RFC] Enable Werrror by default

[Mark McLoughlin]

On Thu, 2009-06-11 at 13:35 +0100, Paul Brook wrote:

> I'd like to enable Werror by default. I've been using --enable-werror locally 
> for a while now, and it's been extremely useful in picking up dumb errors 
> (like the recent stellaris_enet.c breakage).
> 
> Any objections?
> 
> You can of course configure with --disable-werror if you really want the force 
> things to build.

Based on experiences with other projects:

  1) Release tarballs should not ship with -Werror on by default - e.g. 
     new gcc comes along with new warnings and the tarball build fails 
     with no benefit to anyone

  2) Anyone submitting patches should build with -Werror and make sure 
     they don't introduce new warnings

  3) People with newer gcc are likely to be tripped up by warnings 
     introduced by others with older gcc[1]

  4) It's debatable whether builds from git should default to -Werror - 
     on the plus side it helps ensure (2) happens, on the minus side if 
     a warning does sneak in, it makes life a pain for everyone until a 
     fix gets applied

IMHO, we should enable it by default for git builds.

Cheers,
Mark.

[1] e.g. with gcc-4.4.0, but not with gcc-4.3.2:

  hw/virtio-blk.c:302: warning: ‘blkcfg.size_max’ is used uninitialized in this function

Re: [Qemu-devel] [RFC] Enable Werrror by default

[Christoph Egger]

On Thursday 11 June 2009 15:14:46 Mark McLoughlin wrote:
> On Thu, 2009-06-11 at 13:35 +0100, Paul Brook wrote:
> > I'd like to enable Werror by default. I've been using --enable-werror
> > locally for a while now, and it's been extremely useful in picking up
> > dumb errors (like the recent stellaris_enet.c breakage).
> >
> > Any objections?
> >
> > You can of course configure with --disable-werror if you really want the
> > force things to build.
>
> Based on experiences with other projects:
>
>   1) Release tarballs should not ship with -Werror on by default - e.g.
>      new gcc comes along with new warnings and the tarball build fails
>      with no benefit to anyone
>
>   2) Anyone submitting patches should build with -Werror and make sure
>      they don't introduce new warnings

Newer gcc enable warnings by default older versions already have as well.

>   3) People with newer gcc are likely to be tripped up by warnings
>      introduced by others with older gcc[1]
>
>   4) It's debatable whether builds from git should default to -Werror -
>      on the plus side it helps ensure (2) happens, on the minus side if
>      a warning does sneak in, it makes life a pain for everyone until a
>      fix gets applied
>
> IMHO, we should enable it by default for git builds.
>
> Cheers,
> Mark.
>
> [1] e.g. with gcc-4.4.0, but not with gcc-4.3.2:
>
>   hw/virtio-blk.c:302: warning: ‘blkcfg.size_max’ is used uninitialized in
> this function

You should see the same warning with gcc 4.3.2  -Wuninitialized , too.

Christoph

-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Karl-Hammerschmidt-Str. 34, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Thomas M. McCoy, Giuliano Meroni
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

Re: [Qemu-devel] [RFC] Enable Werrror by default

[Paul Brook]

>   hw/virtio-blk.c:302: warning: ‘blkcfg.size_max’ is used uninitialized in
> this function

By my reading that's an actual bug, and uninitialized data is leaking out to 
the guest.

Paul

Re: [Qemu-devel] [RFC] Enable Werrror by default

[Andreas Färber]

Am 11.06.2009 um 14:46 schrieb Tristan Gingold:

>
> On Jun 11, 2009, at 2:35 PM, Paul Brook wrote:
>
>> I'd like to enable Werror by default. I've been using --enable- 
>> werror locally
>> for a while now, and it's been extremely useful in picking up dumb  
>> errors
>> (like the recent stellaris_enet.c breakage).
>>
>> Any objections?
>
> IIRC, qemu is not warnings clean on Darwin.
> But adding -Werror may increase the pressure :-)

Those producing the warnings != those seeing the build breakage.

We've been reporting an incredibly large number of warnings on Solaris  
10 in --std=gnu99 mode (which had become necessary, unfortunately),  
fixing which would seem to require a major refactoring of the  
softfloat support iirc.

And yes, there is at least one warning on Darwin, outside Cocoa. I'll  
take a look at that later.

Other packages restrict such flags to an --enable-maintainer-mode  
configure flag btw.

Andreas

Re: [Qemu-devel] [RFC] Enable Werrror by default

[Richard W.M. Jones]

On Thu, Jun 11, 2009 at 02:14:46PM +0100, Mark McLoughlin wrote:
>   2) Anyone submitting patches should build with -Werror and make sure 
>      they don't introduce new warnings

And in libvirt this is codified in the HACKING file, along with
several other tests that patch submitters are required to run:

http://git.et.redhat.com/?p=libvirt.git;a=blob;f=HACKING;hb=HEAD
(tip #5)

Rich.

-- 
Richard Jones, Emerging Technologies, Red Hat  http://et.redhat.com/~rjones
Read my programming blog: http://rwmj.wordpress.com
Fedora now supports 75 OCaml packages (the OPEN alternative to F#)
http://cocan.org/getting_started_with_ocaml_on_red_hat_and_fedora

Re: [Qemu-devel] [RFC] Enable Werrror by default

[David Turner]

[-- Attachment #1: Type: text/plain, Size: 593 bytes --]

On Thu, Jun 11, 2009 at 2:35 PM, Paul Brook <paul@codesourcery.com> wrote:

> I'd like to enable Werror by default. I've been using --enable-werror
> locally
> for a while now, and it's been extremely useful in picking up dumb errors
> (like the recent stellaris_enet.c breakage).
>
> Any objections?
>

I would recommend only making this the default for linux-x86 and
linux-x86_64 hosts,

My experience is that it is extremely unlikely that codes that compiles
warning-free on Linux
will do the same on Windows or Darwin. Even changing compiler versions can
result in
very surprising things.

[-- Attachment #2: Type: text/html, Size: 911 bytes --]

Re: [Qemu-devel] [RFC] Enable Werrror by default

[Stuart Brady]

On Thu, Jun 11, 2009 at 05:18:27PM +0200, David Turner wrote:
> Even changing compiler versions can result in very surprising things.

For this reason, amongst others, I prefer not to use -Werror by default.

I gather that changing code optimisation settings can also result in
surprising things... and then there's FORTIFY_SOURCE...

Using more specific options such as -Werror=format on versions of GCC
that support it might be more reasonable... but care needs to be taken.
-Werror[=-]implicit-function-declaration is probably a sensible one, but
note that GCC 4.4 seems to have a different name for that option than
earlier releases.

Otherwise, you can almost certainly guarantee there there will be 
distributions that forget to disable -Werror, forcing you to alter their
packages to get them to build. :-(

Note also that enabling -Werror will *discourage* the use of additional
warning options that would be helpful, despite being slightly noisy.
-- 
Stuart Brady

Re: [Qemu-devel] [RFC] Enable Werrror by default

[Anthony Liguori]

[-- Attachment #1: Type: text/plain, Size: 1326 bytes --]

Mark McLoughlin wrote:
> On Thu, 2009-06-11 at 13:35 +0100, Paul Brook wrote:
>
>   
>> I'd like to enable Werror by default. I've been using --enable-werror locally 
>> for a while now, and it's been extremely useful in picking up dumb errors 
>> (like the recent stellaris_enet.c breakage).
>>
>> Any objections?
>>
>> You can of course configure with --disable-werror if you really want the force 
>> things to build.
>>     
>
> Based on experiences with other projects:
>
>   1) Release tarballs should not ship with -Werror on by default - e.g. 
>      new gcc comes along with new warnings and the tarball build fails 
>      with no benefit to anyone
>
>   2) Anyone submitting patches should build with -Werror and make sure 
>      they don't introduce new warnings
>
>   3) People with newer gcc are likely to be tripped up by warnings 
>      introduced by others with older gcc[1]
>
>   4) It's debatable whether builds from git should default to -Werror - 
>      on the plus side it helps ensure (2) happens, on the minus side if 
>      a warning does sneak in, it makes life a pain for everyone until a 
>      fix gets applied
>
> IMHO, we should enable it by default for git builds.
>   
How about the following?  It currently only enables werror for Linux 
hosts and git builds.

Regards,

Anthony Liguori

[-- Attachment #2: enable-werror.patch --]
[-- Type: text/x-patch, Size: 1237 bytes --]

commit ba59b4615be2b9b989b425bad2499f47a6928d5c
Author: Anthony Liguori <aliguori@us.ibm.com>
Date:   Thu Jun 11 13:28:25 2009 -0500

    Enable -Werror by default for git builds on Linux hosts
    
    Additional hosts can be added to the white list as they are confirmed to build
    with --enable-werror.
    
    Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

diff --git a/configure b/configure
index 89e7f53..48c8949 100755
--- a/configure
+++ b/configure
@@ -356,11 +356,7 @@ else
 fi
 [ -f "$workdir/vl.c" ] || source_path_used="yes"
 
-werror="no"
-# generate compile errors on warnings for development builds
-#if grep cvs $source_path/VERSION > /dev/null 2>&1 ; then
-#werror="yes";
-#fi
+werror=""
 
 for opt do
   optarg=`expr "x$opt" : 'x[^=]*=\(.*\)'`
@@ -657,6 +653,18 @@ if test ! -x "$(which cgcc 2>/dev/null)"; then
     sparse="no"
 fi
 
+# Consult white-list to determine whether to enable werror
+# by default.  Only enable by default for git builds
+if test -z "$werror" ; then
+    z_version=`cut -f3 -d. $source_path/VERSION`
+    if test "$z_version" = "50" -a \
+       "$linux" = "yes" ; then
+	werror="yes"
+    else
+	werror="no"
+    fi
+fi
+
 #
 # Solaris specific configure tool chain decisions
 #

Re: [Qemu-devel] [RFC] Enable Werrror by default

[Mark McLoughlin]

On Thu, 2009-06-11 at 13:30 -0500, Anthony Liguori wrote:

> How about the following?  It currently only enables werror for Linux 
> hosts and git builds.
> 
... 
> +# Consult white-list to determine whether to enable werror
> +# by default.  Only enable by default for git builds
> +if test -z "$werror" ; then
> +    z_version=`cut -f3 -d. $source_path/VERSION`
> +    if test "$z_version" = "50" -a \

Looks good, except this will enable it in kvm-XX releases, and not in
future stable branches.

How about just 'test -d $source_path/.git/' ?

Cheers,
Mark.

Re: [Qemu-devel] [RFC] Enable Werrror by default

[Anthony Liguori]

Mark McLoughlin wrote:
> On Thu, 2009-06-11 at 13:30 -0500, Anthony Liguori wrote:
>
>   
>> How about the following?  It currently only enables werror for Linux 
>> hosts and git builds.
>>
>>     
> ... 
>   
>> +# Consult white-list to determine whether to enable werror
>> +# by default.  Only enable by default for git builds
>> +if test -z "$werror" ; then
>> +    z_version=`cut -f3 -d. $source_path/VERSION`
>> +    if test "$z_version" = "50" -a \
>>     
>
> Looks good, except this will enable it in kvm-XX releases, and not in
> future stable branches.
>   

My main requirement is that I don't have to change anything when making 
a tarball release.

kvm releases carry a KVM_VERSION file that could be used to disable werror.

It's a good point about the stable branch though.  I don't know the best 
way to deal with that.  Disabling werror more than we could is certainly 
better than enabling it more than we should though.

> How about just 'test -d $source_path/.git/' ?
>   

Makes me a little nervous to add git-knowledge to configure.

Regards,

Anthony Liguori

> Cheers,
> Mark.
>
>   

Re: [Qemu-devel] [RFC] Enable Werrror by default

[Paul Brook]

> How about the following?  It currently only enables werror for Linux
> hosts and git builds.
>+    z_version=`cut -f3 -d. $source_path/VERSION`
>+    if test "$z_version" = "50" -a \

Looks ok to me.
FWIW I think testing for the presence of .git would be a bad idea.

Paul