Message-ID: <Y+u2XipFSIadP3IP@dev-arch.thelio-3990X>
Date:   Tue, 14 Feb 2023 09:27:10 -0700
From:   Nathan Chancellor <nathan@...nel.org>
To:     Yazen Ghannam <yazen.ghannam@....com>
Cc:     Borislav Petkov <bp@...en8.de>, Tom Rix <trix@...hat.com>,
        tony.luck@...el.com, james.morse@....com, mchehab@...nel.org,
        rric@...nel.org, linux-edac@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] EDAC/amd64: Shut up an -Werror,-Wsometimes-uninitialized
 clang false positive

On Tue, Feb 14, 2023 at 03:04:35PM +0000, Yazen Ghannam wrote:
> On Tue, Feb 14, 2023 at 07:32:36AM -0700, Nathan Chancellor wrote:
> > On Tue, Feb 14, 2023 at 10:55:51AM +0100, Borislav Petkov wrote:
> > > From: Yazen Ghannam <yazen.ghannam@....com>
> > > 
> > > Yeah, the code's fine even without this.
> > > 
> > > What this is fixing is a compiler which is overeager to report false
> > > positives which then get automatically enabled in -Wall builds and when
> > > CONFIG_WERROR is set in allmodconfig builds, the build fails.
> > > 
> > > It doesn't happen with gcc.
> > > 
> > > Maybe clang should be more conservative when enabling such warnings
> > > under -Wall as, apparently, this has an impact beyond just noisy output.
> > 
> > For the record, this is the first false positive that I have seen from
> > this warning in quite some time. You can flip through our issue tracker
> > and see how many instances of the uninitialized warnings there have been
> > and the vast majority of the ones in 2022 at least are all true
> > positives:
> > 
> > https://github.com/ClangBuiltLinux/linux/issues?q=label%3A-Wsometimes-uninitialized%2C-Wuninitialized
> > 
> > So I disagree with the characterization that clang is "overeager to
> > report false positives" and I think the opinionated parts of the commit
> > message could be replaced with some of the technical analysis that Tom
> > and I did to show why this is a false positive but not one clang can
> > reason about with the way the code is structured (since the warning does
> > not perform interprocedural analysis). However, not my circus, not my
> > monkeys, so feel free to ignore all this :)
> > 
> > Regardless, my review still stands and thank you again for the fix.
> >
> 
> Thanks Nathan for the feedback and thanks Boris for the patch.
> 
> Nathan,
> I see there's a ClangBuiltLinux/continuous-integration2 project on github.
> Is this something developers should try to leverage? Maybe just fork it and
> update the action/workflows to use test branches?

Our continuous integration relies on TuxSuite [1], which in turn
requires access to their service. TuxMake [2] is the backend for
TuxSuite, which is what I use doing a lot of my build testing. It can
use your local toolchains or it can use Docker/Podman to build in their
curated containers, which have a wide variety of versions, if that
matters to you.

I have thought about writing a wrapper around tuxmake to build our
TuxSuite configurations (the tuxsuite/ folder within our repo) locally,
maybe this is time to do so :) it would be useful to have something like

  $ scripts/build-local.py tuxsuite/tip-clang-15.yml tuxsuite/tip-clang-16.yml

which would allow people to easily test the configurations that we
generally care about for -tip with recent/stable versions of clang/LLVM.
Otherwise, a simple

  $ tuxmake -a x86_64 -k allmodconfig -t llvm default

or

  $ make -skj"$(nproc)" ARCH=x86_64 LLVM=1 allmodconfig all

is generally good enough to catch the majority of problems visible with
clang, assuming your distribution has a version of LLVM that the kernel
supports (11.x+).

[1]: https://tuxsuite.com
[2]: https://tuxmake.org

Cheers,
Nathan

Powered by blists - more mailing lists