Date:   Tue, 14 Feb 2023 07:32:36 -0700
From:   Nathan Chancellor <nathan@kernel.org>
To:     Borislav Petkov <bp@alien8.de>
Cc:     Yazen Ghannam <yazen.ghannam@amd.com>, 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
Message-ID: <Y+ubhHlWFv4ifmGn@dev-arch.thelio-3990X>
References: <20230213191510.2237360-1-trix@redhat.com>
 <Y+qZthCMRL1m0p4B@yaz-fattaah>
 <Y+qcU2M5gchfzbky@zn.tnic>
 <Y+qdVHidnrrKvxiD@dev-arch.thelio-3990X>
 <03b91ce8-c6d0-63e7-561c-8cada0ece2fe@redhat.com>
 <Y+q1mhrAKTobp3fa@yaz-fattaah>
 <Y+q2pXYI02qAje8N@dev-arch.thelio-3990X>
 <Y+tapzerW7h9vMvp@zn.tnic>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Y+tapzerW7h9vMvp@zn.tnic>
Precedence: bulk

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