Message-ID: <YnWYHzQC4Y55sOsT@dev-arch.thelio-3990X>
Date:   Fri, 6 May 2022 14:50:23 -0700
From:   Nathan Chancellor <nathan@...nel.org>
To:     Dan Carpenter <dan.carpenter@...cle.com>
Cc:     ksummit-discuss@...ts.linuxfoundation.org,
        linux-kernel@...r.kernel.org, kbuild@...ts.01.org, lkp@...el.com,
        llvm@...ts.linux.dev, Nick Desaulniers <ndesaulniers@...gle.com>,
        Tom Rix <trix@...hat.com>
Subject: Re: uninitialized variables bugs

Hi Dan,

On Fri, May 06, 2022 at 12:13:38PM +0300, Dan Carpenter wrote:
> Ever since commit 78a5255ffb6a ("Stop the ad-hoc games with
> -Wno-maybe-initialized"), GCC's uninitialized variable warnings have
> been disabled by default.  Now, you have to turn on W=1 or W=2 to see
> the warnings which nobody except Arnd does.

Thank you a lot for bringing this up; the situation does leave much to
be desired from my side, as I am having to fix quite a number of these
issues because people just are not seeing them and they break our builds
because of CONFIG_WERROR (as they should).

> Disabling that has lead to a bunch of embarrassing bugs where variables
> are *never* initialized.  Very unsubtle bugs.  The bugs doesn't reach
> users because Nathan Chancellor and I review Clang and Smatch warnings
> respectively.  Also the kbuild-bot reports uninitialized variables.

Thankfully, I believe the situation is a lot less worse than it could be
because the kbuild test robot tests with clang and finds these before
they make it into any tree:

https://lore.kernel.org/llvm/?q=f%3Alkp%40intel.com+Wuninitialized
https://lore.kernel.org/llvm/?q=f%3Alkp%40intel.com+Wsometimes-uninitialized

> It's a lot to deal with.  Uninitialized variable bugs are probably the
> most common bug I have to deal with.

Agreed.

> It's frustrating.  Sometimes the false positives are hard to analyse
> because I have to read through multiple functions.  A lot of times
> when I write a patch and a commit message Nathan has already fixed it
> so it's just a waste of time.

Sorry :( I should be better about either cc'ing you directly or adding
the kernel-janitors mailing list, as there are others who would benefit
from seeing these patches fly by. I know that isn't really the point of
the email but I'll try to make your life easier in the future.

> It's risky as well.  The Smatch check for uninitialized variables was
> broken for most of 2021.  Nathan sometimes goes on vacation.
> 
> I guess I would hope that one day we can turn on the GCC uninitialized
> variable warnings again.  That would mean silencing false positives
> which a lot of people don't want to do...  Maybe Clang has fewer false
> positives than GCC?

Yes, clang does have fewer false positives than GCC for a couple of
reasons:

1. As Arnd touched on, Clang's -Wuninitialized and
-Wsometimes-uninitialized do not check for initializations across
function boundaries. In your example below with 'read(&val)', clang will
assume that read() initializes val. While that does mean that there is
slightly less coverage, it does drives the false positive rate way down,
almost to zero. There are occasionally times where clang fails to figure
out certain conditions which will avoid an uninitialized use but I
believe that means the code is not as clear as it could be. For example,
commit 118de6106735 ("net: ethernet: rmnet: Restructure if checks to
avoid uninitialized warning").

2. clang used to only have these warnings under
-Wconditional-uninitialized, which suffers from the same issue as
-Wmaybe-uninitialized ("maybe it is uninitialized?").
-Wsometimes-uninitialized was split off from that warning back in 2011
to be more assertive ("this IS uninitialized if these conditions hold"):

https://github.com/llvm/llvm-project/commit/4323bf8e2e5135c49f814940b2b546298c01ecbc

Perhaps GCC could consider something to this?

Clang's static analyzer, which Tom regularly runs, will check variables
across function boundaries. I am not sure what the false positive rate
on that check is but it does turn up issues like smatch does.

> The Smatch check for uninitialized variable was deliberately written to
> be more strict than GCC because GCC was missing bugs.  So I think
> leaving Smatch false positives is fine.  There is a trade off between
> fewer false positives and missing bugs and Smatch is meant to err on the
> side of finding bugs but with the cost of false positives.

I would agree with this too.

Cheers,
Nathan

Powered by blists - more mailing lists