Re: [PATCH] EDAC/amd64: Shut up an -Werror,-Wsometimes-uninitialized clang false positive

From: Nathan Chancellor <nathan@kernel.org>
To: Yazen Ghannam <yazen.ghannam@amd.com>
Cc: Borislav Petkov <bp@alien8.de>, Tom Rix <trix@redhat.com>,
	tony.luck@intel.com, james.morse@arm.com, mchehab@kernel.org,
	rric@kernel.org, linux-edac@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] EDAC/amd64: Shut up an -Werror,-Wsometimes-uninitialized clang false positive
Date: Tue, 14 Feb 2023 09:27:10 -0700	[thread overview]
Message-ID: <Y+u2XipFSIadP3IP@dev-arch.thelio-3990X> (raw)
In-Reply-To: <Y+ujA+Kr05Adky1l@yaz-fattaah>

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@amd.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