diff mbox series

EDAC/amd64: Shut up an -Werror,-Wsometimes-uninitialized clang false positive

Message ID Y+tapzerW7h9vMvp@zn.tnic (mailing list archive)
State New, archived
Headers show
Series EDAC/amd64: Shut up an -Werror,-Wsometimes-uninitialized clang false positive | expand

Commit Message

Borislav Petkov Feb. 14, 2023, 9:55 a.m. UTC 
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.

  [ bp: Write a commit message. ]

Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
Reviewed-by: Nathan Chancellor <nathan@kernel.org>
Link: https://lore.kernel.org/r/Y%2BqdVHidnrrKvxiD@dev-arch.thelio-3990X
---
 drivers/edac/amd64_edac.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Nathan Chancellor Feb. 14, 2023, 2:32 p.m. UTC | #1 
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.

Cheers,
Nathan

>   [ bp: Write a commit message. ]
> 
> Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
> Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
> Reviewed-by: Nathan Chancellor <nathan@kernel.org>
> Link: https://lore.kernel.org/r/Y%2BqdVHidnrrKvxiD@dev-arch.thelio-3990X
> ---
>  drivers/edac/amd64_edac.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
> index 1c4bef1cdf28..5b42533f306a 100644
> --- a/drivers/edac/amd64_edac.c
> +++ b/drivers/edac/amd64_edac.c
> @@ -3928,7 +3928,7 @@ static const struct attribute_group *amd64_edac_attr_groups[] = {
>  
>  static int hw_info_get(struct amd64_pvt *pvt)
>  {
> -	u16 pci_id1, pci_id2;
> +	u16 pci_id1 = 0, pci_id2 = 0;
>  	int ret;
>  
>  	if (pvt->fam >= 0x17) {
> -- 
> 2.35.1
> 
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette
Yazen Ghannam Feb. 14, 2023, 3:04 p.m. UTC | #2 
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?

Thanks,
Yazen
Nathan Chancellor Feb. 14, 2023, 4:27 p.m. UTC | #3 
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
Borislav Petkov Feb. 14, 2023, 4:38 p.m. UTC | #4 
On Tue, Feb 14, 2023 at 07:32:36AM -0700, Nathan Chancellor wrote:
> 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).

I'm sure you can create all kinds of cases like this one if
interprocedural analysis or aggressive inlining doesn't happen. So I'm
rather surprised that this is the first false positive to happen. But
whateva.

And since we're disagreeing with things: I don't mind if this is a false
positive - I don't care. What I don't agree with is having -Werror fail
the build because of it and forcing us to "wag the dog", so to speak.

And you can imagine that this has been happening for a while now. And it
can explain my reaction to yet another compiler fix.

But ok, we've wasted enough time on this, lemme tone down the commit
message and commit it.

Thx.
diff mbox series

Patch

diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
index 1c4bef1cdf28..5b42533f306a 100644
--- a/drivers/edac/amd64_edac.c
+++ b/drivers/edac/amd64_edac.c
@@ -3928,7 +3928,7 @@  static const struct attribute_group *amd64_edac_attr_groups[] = {
 
 static int hw_info_get(struct amd64_pvt *pvt)
 {
-	u16 pci_id1, pci_id2;
+	u16 pci_id1 = 0, pci_id2 = 0;
 	int ret;
 
 	if (pvt->fam >= 0x17) {