From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1754311AbaGHLhb (ORCPT <rfc822;w@1wt.eu>);
	Tue, 8 Jul 2014 07:37:31 -0400
Received: from mail.skyhub.de ([78.46.96.112]:51487 "EHLO mail.skyhub.de"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1753430AbaGHLha (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
	Tue, 8 Jul 2014 07:37:30 -0400
Date: Tue, 8 Jul 2014 13:37:24 +0200
From: Borislav Petkov <bp@alien8.de>
To: Paul Bolle <pebolle@tiscali.nl>
Cc: Sam Ravnborg <sam@ravnborg.org>, lkml <linux-kernel@vger.kernel.org>,
        Michael Matz <matz@suse.de>, linux-kbuild@vger.kernel.org,
        x86-ml <x86@kernel.org>
Subject: Re: [PATCH] Kbuild: Move -Wmaybe-uninitialized to W=1
Message-ID: <20140708113724.GA27659@pd.tnic>
References: <20140616132045.GE8170@pd.tnic>
 <20140616211405.GA7914@ravnborg.org>
 <20140624213835.GD15068@pd.tnic>
 <20140707105339.GA4776@pd.tnic>
 <1404811520.26126.38.camel@x220>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <1404811520.26126.38.camel@x220>
User-Agent: Mutt/1.5.23 (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Tue, Jul 08, 2014 at 11:25:20AM +0200, Paul Bolle wrote:
> Is the fact that this generates false positives by itself enough to
> downgrade this check to W=1?
> 
> I do not have any hard numbers to back up my claims, but I'd like to
> point out that it is possible that we never see many of the warnings
> that GCC correctly issues because they might only occur during local
> development. Ie, the developer involved cleans up the patch before
> sending out.

You're assuming that a developer doesn't use W=<num> to test her/his
patches.

> My experience with the warnings that actually do make it into mainline

Which warnings? All warnings or only the maybe-uninitialized ones?

> is that quite a few are correct while the false positives tend to be
> generated by a pieces of code that are complicated (I think I've seen
> the warning labeled as a "code smell").
> 
> For example, in my local builds this warning popped up three times in
> the current development cycle:
>     0) fs/direct-io.c:1011:12: warning: ‘from’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>        fs/direct-io.c:1011:12: warning: ‘to’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>     1) drivers/platform/x86/eeepc-laptop.c:279:10: warning: ‘value’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>     2) drivers/usb/misc/usb3503.c:195:11: warning: ‘err’ may be used uninitialized in this function [-Wmaybe-uninitialized]
> 
> Warning 0) occurs in a 150 line function, that grows over 200 lines when
> the inline functions it calls are included. And I do think it's not easy
> to see hwat that code does.

Have you actually tried to understand what the code does and also to see
that gcc simply cannot see that the to and from will never be used in
the error case? It is not that hard actually.

> Anyhow, see
> https://lkml.org/lkml/2014/7/1/496 for my attempt to silence this
> warning by simplifying this function.

Terrible idea - misdesign the code just because gcc doesn't say that two
variables *might* not be initialized.

> See https://lkml.org/lkml/2014/7/1/150 for my patch that silences 1) by,
> again, simplifying the code.

Again, this is wrong on *every* level! This is perfectly sane code where
value is used *only* in the success case.

> And warning 2) is correct. See https://lkml.org/lkml/2014/6/2/511 for a
> possible solution.

That warning should actually be not "maybe" but really -Wuninitialized.

> So, in summary, I believe that the signal/noise ratio actually isn't
> too bad. Moreover I think that the noise isn't merely noise, as it
> points to possibly problematic sections of code.

It seems you're missing the point so let me explain to you what I mean:

Here's the relevant snippet from the gcc manpage:

       -Wmaybe-uninitialized
           For an automatic variable, if there exists a path from the function entry to
           a use of the variable that is initialized, but there exist some other paths
           for which the variable is not initialized, the compiler emits a warning if
           it cannot prove the uninitialized paths are not executed at run time. These
           warnings are made optional because GCC is not smart enough to see all the
           reasons why the code might be correct in spite of appearing to have an
           error.

Now read it again: "gcc cannot prove the uninitialized paths are not
executed at run time. These warnings are made optional because GCC is
not smart enough ..."

And this is exactly what I'm proposing: keep the warning but downgrade
it so that it doesn't generate false positives noise. I'm not arguing it
is a bad check - I'm just saying it is more of a heuristic and should
belong to the rest of the W= checks.

Oh, moving it to W=1 has another reason - to hide it from overeager
people who really want to *fix* the code and thus obfuscate it for no
sensible reason what*so*f*ckin*ever! Just so that they can please the
compiler which says itself it cannot be that smart!

You have given perfect examples in 0) and 1) above for exactly that.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--