[Qemu-devel] [PATCH] move -Wno-error=uninitialized to configure

[Qemu-devel] [PATCH] move -Wno-error=uninitialized to configure

[Michael S. Tsirkin]

Move -Wno-error=uninitialized out of rules.mak and into configure. Only
use it if supported by compiler.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 configure |   15 ++++++++++++++-
 rules.mak |    2 +-
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/configure b/configure
index eb9d73a..69c7471 100755
--- a/configure
+++ b/configure
@@ -526,8 +526,21 @@ if test -z "$werror" ; then
     fi
 fi
 
+# Disable uninitialized variable warning or convert it to non-error, if possible
+cat > $TMPC <<EOF
+int main(void) {}
+EOF
+if $cc -Werror -Wno-error=uninitialized -c -o $TMPO $TMPC > /dev/null 2> /dev/null ; then
+    # Enable werror by default for recent compilers
+    werror_cflags="-Werror -Wno-error=uninitialized"
+elif $cc -Werror -Wno-uninitialized -c -o $TMPO $TMPC > /dev/null 2> /dev/null ; then
+    werror_cflags="-Werror -Wno-uninitialized"
+else
+    werror_cflags="-Werror"
+fi
+
 if test "$werror" = "yes" ; then
-    CFLAGS="$CFLAGS -Werror"
+    CFLAGS="$CFLAGS $werror_cflags"
 fi
 
 if test "$solaris" = "no" ; then
diff --git a/rules.mak b/rules.mak
index defee1d..8d6d96e 100644
--- a/rules.mak
+++ b/rules.mak
@@ -1,6 +1,6 @@
 
 %.o: %.c
-	$(call quiet-command,$(CC) $(CPPFLAGS) $(CFLAGS) -Werror -Wno-error=uninitialized -c -o $@ $<,"  CC    $(TARGET_DIR)$@")
+	$(call quiet-command,$(CC) $(CPPFLAGS) $(CFLAGS) -c -o $@ $<,"  CC    $(TARGET_DIR)$@")
 
 %.o: %.S
 	$(call quiet-command,$(CC) $(CPPFLAGS) -c -o $@ $<,"  AS    $(TARGET_DIR)$@")
-- 
1.6.2.2

Re: [Qemu-devel] [PATCH] move -Wno-error=uninitialized to configure

[Paul Brook]

On Tuesday 30 June 2009, Michael S. Tsirkin wrote:
> Move -Wno-error=uninitialized out of rules.mak and into configure. Only
> use it if supported by compiler.

Why? Shouldn't we be fixing the uninitialized variables?

Paul

Re: [Qemu-devel] [PATCH] move -Wno-error=uninitialized to configure

[Michael S. Tsirkin]

On Tue, Jun 30, 2009 at 01:07:34PM +0100, Paul Brook wrote:
> On Tuesday 30 June 2009, Michael S. Tsirkin wrote:
> > Move -Wno-error=uninitialized out of rules.mak and into configure. Only
> > use it if supported by compiler.
> 
> Why? Shouldn't we be fixing the uninitialized variables?
> 
> Paul

gcc's algorithm for detecting uninitialized variables is
famously unreliable. Just scattering '= 0' around the code
masks the warning but hides the fact that application logic
might still be broken.

-- 
MST

Re: [Qemu-devel] [PATCH] move -Wno-error=uninitialized to configure

[Michael S. Tsirkin]

On Tue, Jun 30, 2009 at 03:12:40PM +0300, Michael S. Tsirkin wrote:
> On Tue, Jun 30, 2009 at 01:07:34PM +0100, Paul Brook wrote:
> > On Tuesday 30 June 2009, Michael S. Tsirkin wrote:
> > > Move -Wno-error=uninitialized out of rules.mak and into configure. Only
> > > use it if supported by compiler.
> > 
> > Why? Shouldn't we be fixing the uninitialized variables?
> > 
> > Paul
> 
> gcc's algorithm for detecting uninitialized variables is
> famously unreliable. Just scattering '= 0' around the code
> masks the warning but hides the fact that application logic
> might still be broken.

To clarify: what's broken is that gcc has false positives in the logic.
And if you work around this by adding initialization where it's not
really required, you confuse other tools that would otherwise have a
chance to detect an error.

-- 
MST

Re: [Qemu-devel] [PATCH] move -Wno-error=uninitialized to configure

[Paul Brook]

On Tuesday 30 June 2009, Michael S. Tsirkin wrote:
> On Tue, Jun 30, 2009 at 03:12:40PM +0300, Michael S. Tsirkin wrote:
> > On Tue, Jun 30, 2009 at 01:07:34PM +0100, Paul Brook wrote:
> > > On Tuesday 30 June 2009, Michael S. Tsirkin wrote:
> > > > Move -Wno-error=uninitialized out of rules.mak and into configure.
> > > > Only use it if supported by compiler.
> > >
> > > Why? Shouldn't we be fixing the uninitialized variables?
> > >
> > > Paul
> >
> > gcc's algorithm for detecting uninitialized variables is
> > famously unreliable. Just scattering '= 0' around the code
> > masks the warning but hides the fact that application logic
> > might still be broken.
>
> To clarify: what's broken is that gcc has false positives in the logic.
> And if you work around this by adding initialization where it's not
> really required, you confuse other tools that would otherwise have a
> chance to detect an error.

In that case should we be using -Wno-uninitialized?

The whole point of -Werror is that it forces us to keep the code clean. Having 
to mentally filter out false positives just doesn't work. Once you have 
routinely expected warnings then people stop checking for new ones.

Paul

Re: [Qemu-devel] [PATCH] move -Wno-error=uninitialized to configure

[Michael S. Tsirkin]

On Tue, Jun 30, 2009 at 01:44:31PM +0100, Paul Brook wrote:
> On Tuesday 30 June 2009, Michael S. Tsirkin wrote:
> > On Tue, Jun 30, 2009 at 03:12:40PM +0300, Michael S. Tsirkin wrote:
> > > On Tue, Jun 30, 2009 at 01:07:34PM +0100, Paul Brook wrote:
> > > > On Tuesday 30 June 2009, Michael S. Tsirkin wrote:
> > > > > Move -Wno-error=uninitialized out of rules.mak and into configure.
> > > > > Only use it if supported by compiler.
> > > >
> > > > Why? Shouldn't we be fixing the uninitialized variables?
> > > >
> > > > Paul
> > >
> > > gcc's algorithm for detecting uninitialized variables is
> > > famously unreliable. Just scattering '= 0' around the code
> > > masks the warning but hides the fact that application logic
> > > might still be broken.
> >
> > To clarify: what's broken is that gcc has false positives in the logic.
> > And if you work around this by adding initialization where it's not
> > really required, you confuse other tools that would otherwise have a
> > chance to detect an error.
> 
> In that case should we be using -Wno-uninitialized?
> 
> The whole point of -Werror is that it forces us to keep the code clean.
> Having to mentally filter out false positives just doesn't work. Once you have 
> routinely expected warnings then people stop checking for new ones.
> 
> Paul

That's what -Wno-error=uninitialized does: makes the filtering
automatically so you don't need to filter it mentally.


-- 
MST

Re: [Qemu-devel] [PATCH] move -Wno-error=uninitialized to configure

[Stuart Brady]

On Tue, Jun 30, 2009 at 03:58:49PM +0300, Michael S. Tsirkin wrote:
> On Tue, Jun 30, 2009 at 01:44:31PM +0100, Paul Brook wrote:
> > The whole point of -Werror is that it forces us to keep the code clean.
> > Having to mentally filter out false positives just doesn't work. Once you have 
> > routinely expected warnings then people stop checking for new ones.
> 
> That's what -Wno-error=uninitialized does: makes the filtering
> automatically so you don't need to filter it mentally.

Right, but not all versions of GCC that support '-Werror' support
'-Wno-error=...'.  As I tried to hint at previously, the only sensible
way to deal with this is to use '-Werror=...' to be specific about the
types of checking that you want, so that you can avoid checks that
produce false positives, and only enable the checking on versions of GCC
that support it.  Otherwise, future versions of GCC could produce all
sorts of false positives that you have no control over.  '-Werror=...'
doesn't fix that entirely, but at least it's a start.

I've already seen several 'fixes' to QEMU to suppress false positives.
For some strange reason people always pick the magic value '0' when
'shutting GCC up'.  *sigh*  At the very least, *please please please*
consider something like Linux's uninitialized_var() macro for this.

(BTW, recommended reading: http://lwn.net/Articles/331593/)

Cheers,
-- 
Stuart Brady

Re: [Qemu-devel] [PATCH] move -Wno-error=uninitialized to configure

[Avi Kivity]

On 06/30/2009 03:07 PM, Paul Brook wrote:
> On Tuesday 30 June 2009, Michael S. Tsirkin wrote:
>    
>> Move -Wno-error=uninitialized out of rules.mak and into configure. Only
>> use it if supported by compiler.
>>      
>
> Why? Shouldn't we be fixing the uninitialized variables?
>    

Fixing older compilers to support -Wno-error is out of the scope of this 
project.

-- 
error compiling committee.c: too many arguments to function