* [PATCH] EDAC/amd64: remove unneeded call to reserve_mc_sibling_devs() @ 2023-02-13 19:15 Tom Rix 2023-02-13 20:12 ` Yazen Ghannam 0 siblings, 1 reply; 12+ messages in thread From: Tom Rix @ 2023-02-13 19:15 UTC (permalink / raw) To: yazen.ghannam, bp, tony.luck, james.morse, mchehab, rric Cc: linux-edac, linux-kernel, Tom Rix cpp_check reports drivers/edac/amd64_edac.c:3943:37: error: Uninitialized variable: pci_id1 [uninitvar] ret = reserve_mc_sibling_devs(pvt, pci_id1, pci_id2); ^ drivers/edac/amd64_edac.c:3943:46: error: Uninitialized variable: pci_id2 [uninitvar] ret = reserve_mc_sibling_devs(pvt, pci_id1, pci_id2); ^ The call to reserve_mc_sibling_devs() will not fail because if (pvt->umc) return 0; reserve_mc_sibling_devs() is only called by hw_info_get() and pvt->umc is only set in hw_info_get(), so with fam >= 0x17, the call to reserver_mc_siblings will just return, so the call the call is not needed. And when that call is moved the check for umc is not needed. Signed-off-by: Tom Rix <trix@redhat.com> --- drivers/edac/amd64_edac.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c index 1c4bef1cdf28..f6d50561c106 100644 --- a/drivers/edac/amd64_edac.c +++ b/drivers/edac/amd64_edac.c @@ -3179,9 +3179,6 @@ static void decode_umc_error(int node_id, struct mce *m) static int reserve_mc_sibling_devs(struct amd64_pvt *pvt, u16 pci_id1, u16 pci_id2) { - if (pvt->umc) - return 0; - /* Reserve the ADDRESS MAP Device */ pvt->F1 = pci_get_related_function(pvt->F3->vendor, pci_id1, pvt->F3); if (!pvt->F1) { @@ -3938,11 +3935,11 @@ static int hw_info_get(struct amd64_pvt *pvt) } else { pci_id1 = fam_type->f1_id; pci_id2 = fam_type->f2_id; - } - ret = reserve_mc_sibling_devs(pvt, pci_id1, pci_id2); - if (ret) - return ret; + ret = reserve_mc_sibling_devs(pvt, pci_id1, pci_id2); + if (ret) + return ret; + } read_mc_regs(pvt); -- 2.26.3 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] EDAC/amd64: remove unneeded call to reserve_mc_sibling_devs() 2023-02-13 19:15 [PATCH] EDAC/amd64: remove unneeded call to reserve_mc_sibling_devs() Tom Rix @ 2023-02-13 20:12 ` Yazen Ghannam 2023-02-13 20:23 ` Borislav Petkov 0 siblings, 1 reply; 12+ messages in thread From: Yazen Ghannam @ 2023-02-13 20:12 UTC (permalink / raw) To: Tom Rix, Nathan Chancellor Cc: bp, tony.luck, james.morse, mchehab, rric, linux-edac, linux-kernel On Mon, Feb 13, 2023 at 11:15:10AM -0800, Tom Rix wrote: > cpp_check reports > drivers/edac/amd64_edac.c:3943:37: error: Uninitialized variable: pci_id1 [uninitvar] > ret = reserve_mc_sibling_devs(pvt, pci_id1, pci_id2); > ^ > drivers/edac/amd64_edac.c:3943:46: error: Uninitialized variable: pci_id2 [uninitvar] > ret = reserve_mc_sibling_devs(pvt, pci_id1, pci_id2); > ^ > The call to reserve_mc_sibling_devs() will not fail because > if (pvt->umc) > return 0; > > reserve_mc_sibling_devs() is only called by hw_info_get() and pvt->umc is only set > in hw_info_get(), so with fam >= 0x17, the call to reserver_mc_siblings will > just return, so the call the call is not needed. And when that call is moved > the check for umc is not needed. > > Signed-off-by: Tom Rix <trix@redhat.com> > --- Link to similar patch from Nathan: https://lore.kernel.org/linux-edac/20230213-amd64_edac-wsometimes-uninitialized-v1-1-5bde32b89e02@kernel.org/ Hi Tom and Nathan, These errors are encountered when extra warnings are enabled, correct? I think the following patch would resolve this issue. This is part of a set that isn't fully applied. https://lore.kernel.org/linux-edac/20230127170419.1824692-12-yazen.ghannam@amd.com/ Boris, Do you think one of these patches should be applied or just hold off until the entire original set is applied? As for myself, I'll start including builds with extra warnings enabled for each patch in my workflow. Currently I do a regular build for each patch and a build with extra warnings for the entire set. Thanks, Yazen ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] EDAC/amd64: remove unneeded call to reserve_mc_sibling_devs() 2023-02-13 20:12 ` Yazen Ghannam @ 2023-02-13 20:23 ` Borislav Petkov 2023-02-13 20:28 ` Nathan Chancellor 0 siblings, 1 reply; 12+ messages in thread From: Borislav Petkov @ 2023-02-13 20:23 UTC (permalink / raw) To: Yazen Ghannam Cc: Tom Rix, Nathan Chancellor, tony.luck, james.morse, mchehab, rric, linux-edac, linux-kernel On Mon, Feb 13, 2023 at 08:12:38PM +0000, Yazen Ghannam wrote: > These errors are encountered when extra warnings are enabled, correct? It says so in the warning which one it is: -Werror,-Wsometimes-uninitialized Don't know if we enable that one for clang with W= or Nathan adds additional switches. > I think the following patch would resolve this issue. This is part of a set > that isn't fully applied. > https://lore.kernel.org/linux-edac/20230127170419.1824692-12-yazen.ghannam@amd.com/ > > Boris, > Do you think one of these patches should be applied or just hold off until the > entire original set is applied? I still wanted to go through the rest but I'm not sure I'll have time for it before the merge window. So unless this is breaking some silly testing scenario, I'd say I'll leave things as they are. Once I take yours, that silly false positive will go away and we can forget about it. > As for myself, I'll start including builds with extra warnings enabled > for each patch in my workflow. Currently I do a regular build for each > patch and a build with extra warnings for the entire set. Dunno, I'd say with false positives we have bigger fish to fry... Thx. -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] EDAC/amd64: remove unneeded call to reserve_mc_sibling_devs() 2023-02-13 20:23 ` Borislav Petkov @ 2023-02-13 20:28 ` Nathan Chancellor 2023-02-13 21:17 ` Tom Rix 0 siblings, 1 reply; 12+ messages in thread From: Nathan Chancellor @ 2023-02-13 20:28 UTC (permalink / raw) To: Borislav Petkov Cc: Yazen Ghannam, Tom Rix, tony.luck, james.morse, mchehab, rric, linux-edac, linux-kernel On Mon, Feb 13, 2023 at 09:23:47PM +0100, Borislav Petkov wrote: > On Mon, Feb 13, 2023 at 08:12:38PM +0000, Yazen Ghannam wrote: > > These errors are encountered when extra warnings are enabled, correct? > > It says so in the warning which one it is: -Werror,-Wsometimes-uninitialized > > Don't know if we enable that one for clang with W= or Nathan adds > additional switches. -Wsometimes-uninitialized is part of clang's -Wall so it is on by default in all builds, regardless of W= -Werror comes from CONFIG_WERROR, which is enabled with allmodconfig. > > I think the following patch would resolve this issue. This is part of a set > > that isn't fully applied. > > https://lore.kernel.org/linux-edac/20230127170419.1824692-12-yazen.ghannam@amd.com/ > > > > Boris, > > Do you think one of these patches should be applied or just hold off until the > > entire original set is applied? > > I still wanted to go through the rest but I'm not sure I'll have time > for it before the merge window. So unless this is breaking some silly > testing scenario, I'd say I'll leave things as they are. This breaks allmodconfig with clang, so it would be great if one of these solutions was applied in the meantime. Cheers, Nathan ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] EDAC/amd64: remove unneeded call to reserve_mc_sibling_devs() 2023-02-13 20:28 ` Nathan Chancellor @ 2023-02-13 21:17 ` Tom Rix 2023-02-13 22:11 ` Yazen Ghannam 0 siblings, 1 reply; 12+ messages in thread From: Tom Rix @ 2023-02-13 21:17 UTC (permalink / raw) To: Nathan Chancellor, Borislav Petkov Cc: Yazen Ghannam, tony.luck, james.morse, mchehab, rric, linux-edac, linux-kernel On 2/13/23 12:28 PM, Nathan Chancellor wrote: > On Mon, Feb 13, 2023 at 09:23:47PM +0100, Borislav Petkov wrote: >> On Mon, Feb 13, 2023 at 08:12:38PM +0000, Yazen Ghannam wrote: >>> These errors are encountered when extra warnings are enabled, correct? >> It says so in the warning which one it is: -Werror,-Wsometimes-uninitialized >> >> Don't know if we enable that one for clang with W= or Nathan adds >> additional switches. > -Wsometimes-uninitialized is part of clang's -Wall so it is on by > default in all builds, regardless of W= > > -Werror comes from CONFIG_WERROR, which is enabled with allmodconfig. > >>> I think the following patch would resolve this issue. This is part of a set >>> that isn't fully applied. >>> https://lore.kernel.org/linux-edac/20230127170419.1824692-12-yazen.ghannam@amd.com/ >>> >>> Boris, >>> Do you think one of these patches should be applied or just hold off until the >>> entire original set is applied? >> I still wanted to go through the rest but I'm not sure I'll have time >> for it before the merge window. So unless this is breaking some silly >> testing scenario, I'd say I'll leave things as they are. > This breaks allmodconfig with clang, so it would be great if one of > these solutions was applied in the meantime. This happens at least on allyesconfig clang W=1,2, i do not know about default, it's in a bad state as well. It would be great if the clang build was working. Nathan's patch is fine, go with that. Tom > > Cheers, > Nathan > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] EDAC/amd64: remove unneeded call to reserve_mc_sibling_devs() 2023-02-13 21:17 ` Tom Rix @ 2023-02-13 22:11 ` Yazen Ghannam 2023-02-13 22:16 ` Nathan Chancellor 0 siblings, 1 reply; 12+ messages in thread From: Yazen Ghannam @ 2023-02-13 22:11 UTC (permalink / raw) To: Tom Rix Cc: Nathan Chancellor, Borislav Petkov, tony.luck, james.morse, mchehab, rric, linux-edac, linux-kernel On Mon, Feb 13, 2023 at 01:17:51PM -0800, Tom Rix wrote: > > On 2/13/23 12:28 PM, Nathan Chancellor wrote: > > On Mon, Feb 13, 2023 at 09:23:47PM +0100, Borislav Petkov wrote: > > > On Mon, Feb 13, 2023 at 08:12:38PM +0000, Yazen Ghannam wrote: > > > > These errors are encountered when extra warnings are enabled, correct? > > > It says so in the warning which one it is: -Werror,-Wsometimes-uninitialized > > > > > > Don't know if we enable that one for clang with W= or Nathan adds > > > additional switches. > > -Wsometimes-uninitialized is part of clang's -Wall so it is on by > > default in all builds, regardless of W= > > > > -Werror comes from CONFIG_WERROR, which is enabled with allmodconfig. > > > > > > I think the following patch would resolve this issue. This is part of a set > > > > that isn't fully applied. > > > > https://lore.kernel.org/linux-edac/20230127170419.1824692-12-yazen.ghannam@amd.com/ > > > > > > > > Boris, > > > > Do you think one of these patches should be applied or just hold off until the > > > > entire original set is applied? > > > I still wanted to go through the rest but I'm not sure I'll have time > > > for it before the merge window. So unless this is breaking some silly > > > testing scenario, I'd say I'll leave things as they are. > > This breaks allmodconfig with clang, so it would be great if one of > > these solutions was applied in the meantime. > > This happens at least on allyesconfig clang W=1,2, i do not know about > default, it's in a bad state as well. > Yes, this breaks on a default clang build. I just used "make LLVM=1" with the same config I used before, and I see the error. GCC doesn't seem to have a comparable warning to "-Wsometimes-uninitialized". I went back and tried W=123 and no warnings in this code. Building with clang was straightforward, so I'll try to include it in my workflow in the future. > It would be great if the clang build was working. > > Nathan's patch is fine, go with that. > I agree Nathan's patch is fine, but would you all be okay with a simpler change? Initializing the variables (as below) will silence the warnings, and we know this is a false positive. Eventually this function will be reworked, so a trivial workaround seems okay. What do y'all think? Thanks, Yazen ------ 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) { ------ ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] EDAC/amd64: remove unneeded call to reserve_mc_sibling_devs() 2023-02-13 22:11 ` Yazen Ghannam @ 2023-02-13 22:16 ` Nathan Chancellor 2023-02-14 9:55 ` [PATCH] EDAC/amd64: Shut up an -Werror,-Wsometimes-uninitialized clang false positive Borislav Petkov 0 siblings, 1 reply; 12+ messages in thread From: Nathan Chancellor @ 2023-02-13 22:16 UTC (permalink / raw) To: Yazen Ghannam Cc: Tom Rix, Borislav Petkov, tony.luck, james.morse, mchehab, rric, linux-edac, linux-kernel On Mon, Feb 13, 2023 at 10:11:38PM +0000, Yazen Ghannam wrote: > On Mon, Feb 13, 2023 at 01:17:51PM -0800, Tom Rix wrote: > > > > On 2/13/23 12:28 PM, Nathan Chancellor wrote: > > > On Mon, Feb 13, 2023 at 09:23:47PM +0100, Borislav Petkov wrote: > > > > On Mon, Feb 13, 2023 at 08:12:38PM +0000, Yazen Ghannam wrote: > > > > > These errors are encountered when extra warnings are enabled, correct? > > > > It says so in the warning which one it is: -Werror,-Wsometimes-uninitialized > > > > > > > > Don't know if we enable that one for clang with W= or Nathan adds > > > > additional switches. > > > -Wsometimes-uninitialized is part of clang's -Wall so it is on by > > > default in all builds, regardless of W= > > > > > > -Werror comes from CONFIG_WERROR, which is enabled with allmodconfig. > > > > > > > > I think the following patch would resolve this issue. This is part of a set > > > > > that isn't fully applied. > > > > > https://lore.kernel.org/linux-edac/20230127170419.1824692-12-yazen.ghannam@amd.com/ > > > > > > > > > > Boris, > > > > > Do you think one of these patches should be applied or just hold off until the > > > > > entire original set is applied? > > > > I still wanted to go through the rest but I'm not sure I'll have time > > > > for it before the merge window. So unless this is breaking some silly > > > > testing scenario, I'd say I'll leave things as they are. > > > This breaks allmodconfig with clang, so it would be great if one of > > > these solutions was applied in the meantime. > > > > This happens at least on allyesconfig clang W=1,2, i do not know about > > default, it's in a bad state as well. > > > > Yes, this breaks on a default clang build. I just used "make LLVM=1" with the > same config I used before, and I see the error. > > GCC doesn't seem to have a comparable warning to "-Wsometimes-uninitialized". > I went back and tried W=123 and no warnings in this code. GCC's -Wmaybe-uninitialized uses interprocedural analysis I believe, which would allow it to see that it is not possible for these variables to be used uninitialized. However, that type of analysis can go wrong with optimizations pretty quickly, so it was disabled for the kernel under normal builds and W=1; W=2 will show those instances again but again, I would not expect there to be one here. > Building with clang was straightforward, so I'll try to include it in my > workflow in the future. > > > It would be great if the clang build was working. > > > > Nathan's patch is fine, go with that. > > > > I agree Nathan's patch is fine, but would you all be okay with a simpler > change? Initializing the variables (as below) will silence the warnings, and > we know this is a false positive. Eventually this function will be reworked, > so a trivial workaround seems okay. What do y'all think? I have no objections. Will you send the patch? Consider it Reviewed-by: Nathan Chancellor <nathan@kernel.org> in advanced. Thanks for taking a further look at this problem! Cheers, Nathan > > Thanks, > Yazen > > ------ > > 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) { > > ------ ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] EDAC/amd64: Shut up an -Werror,-Wsometimes-uninitialized clang false positive 2023-02-13 22:16 ` Nathan Chancellor @ 2023-02-14 9:55 ` Borislav Petkov 2023-02-14 14:32 ` Nathan Chancellor 0 siblings, 1 reply; 12+ messages in thread From: Borislav Petkov @ 2023-02-14 9:55 UTC (permalink / raw) To: Nathan Chancellor Cc: Yazen Ghannam, Tom Rix, tony.luck, james.morse, mchehab, rric, linux-edac, linux-kernel 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(-) 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] EDAC/amd64: Shut up an -Werror,-Wsometimes-uninitialized clang false positive 2023-02-14 9:55 ` [PATCH] EDAC/amd64: Shut up an -Werror,-Wsometimes-uninitialized clang false positive Borislav Petkov @ 2023-02-14 14:32 ` Nathan Chancellor 2023-02-14 15:04 ` Yazen Ghannam 2023-02-14 16:38 ` Borislav Petkov 0 siblings, 2 replies; 12+ messages in thread From: Nathan Chancellor @ 2023-02-14 14:32 UTC (permalink / raw) To: Borislav Petkov Cc: Yazen Ghannam, Tom Rix, tony.luck, james.morse, mchehab, rric, linux-edac, linux-kernel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] EDAC/amd64: Shut up an -Werror,-Wsometimes-uninitialized clang false positive 2023-02-14 14:32 ` Nathan Chancellor @ 2023-02-14 15:04 ` Yazen Ghannam 2023-02-14 16:27 ` Nathan Chancellor 2023-02-14 16:38 ` Borislav Petkov 1 sibling, 1 reply; 12+ messages in thread From: Yazen Ghannam @ 2023-02-14 15:04 UTC (permalink / raw) To: Nathan Chancellor Cc: Borislav Petkov, Tom Rix, tony.luck, james.morse, mchehab, rric, linux-edac, linux-kernel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] EDAC/amd64: Shut up an -Werror,-Wsometimes-uninitialized clang false positive 2023-02-14 15:04 ` Yazen Ghannam @ 2023-02-14 16:27 ` Nathan Chancellor 0 siblings, 0 replies; 12+ messages in thread From: Nathan Chancellor @ 2023-02-14 16:27 UTC (permalink / raw) To: Yazen Ghannam Cc: Borislav Petkov, Tom Rix, tony.luck, james.morse, mchehab, rric, linux-edac, linux-kernel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] EDAC/amd64: Shut up an -Werror,-Wsometimes-uninitialized clang false positive 2023-02-14 14:32 ` Nathan Chancellor 2023-02-14 15:04 ` Yazen Ghannam @ 2023-02-14 16:38 ` Borislav Petkov 1 sibling, 0 replies; 12+ messages in thread From: Borislav Petkov @ 2023-02-14 16:38 UTC (permalink / raw) To: Nathan Chancellor Cc: Yazen Ghannam, Tom Rix, tony.luck, james.morse, mchehab, rric, linux-edac, linux-kernel 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. -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2023-02-14 16:39 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-02-13 19:15 [PATCH] EDAC/amd64: remove unneeded call to reserve_mc_sibling_devs() Tom Rix 2023-02-13 20:12 ` Yazen Ghannam 2023-02-13 20:23 ` Borislav Petkov 2023-02-13 20:28 ` Nathan Chancellor 2023-02-13 21:17 ` Tom Rix 2023-02-13 22:11 ` Yazen Ghannam 2023-02-13 22:16 ` Nathan Chancellor 2023-02-14 9:55 ` [PATCH] EDAC/amd64: Shut up an -Werror,-Wsometimes-uninitialized clang false positive Borislav Petkov 2023-02-14 14:32 ` Nathan Chancellor 2023-02-14 15:04 ` Yazen Ghannam 2023-02-14 16:27 ` Nathan Chancellor 2023-02-14 16:38 ` Borislav Petkov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).