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 <[email protected]>
---
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
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 <[email protected]>
> ---
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/[email protected]/
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
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/[email protected]/
>
> 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
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/[email protected]/
> >
> > 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
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/[email protected]/
>>>
>>> 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
>
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/[email protected]/
> > > >
> > > > 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) {
------
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/[email protected]/
> > > > >
> > > > > 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 <[email protected]>
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) {
>
> ------
From: Yazen Ghannam <[email protected]>
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 <[email protected]>
Signed-off-by: Borislav Petkov (AMD) <[email protected]>
Reviewed-by: Nathan Chancellor <[email protected]>
Link: https://lore.kernel.org/r/Y%[email protected]
---
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
On Tue, Feb 14, 2023 at 10:55:51AM +0100, Borislav Petkov wrote:
> From: Yazen Ghannam <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Borislav Petkov (AMD) <[email protected]>
> Reviewed-by: Nathan Chancellor <[email protected]>
> Link: https://lore.kernel.org/r/Y%[email protected]
> ---
> 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
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 <[email protected]>
> >
> > 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
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 <[email protected]>
> > >
> > > 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
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