Adds -Wshadow=local on KBUILD_HOSTCFLAGS
PATCH 1/4 Adds -Wshadow=local on KBUILD_HOSTCFLAGS
Adds -Wshadow=local on KBUILD_HOSTCFLAGS to show shadow warnings on tools built for HOST.
--- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Makefile +1 -1
@@ -360,7 +360,7 @@ HOST_LFS_LIBS := $(shell getconf LFS_LIBS 2>/dev/null) HOSTCC = gcc HOSTCXX = g++ -KBUILD_HOSTCFLAGS := -Wall -Wmissing-prototypes -Wstrict-prototypes -O2 \ +KBUILD_HOSTCFLAGS := -Wall -Wshadow=local -Wmissing-prototypes -Wstrict-prototypes -O2 \ -fomit-frame-pointer -std=gnu89 $(HOST_LFS_CFLAGS) \ $(HOSTCFLAGS) KBUILD_HOSTCXXFLAGS := -O2 $(HOST_LFS_CFLAGS) $(HOSTCXXFLAGS)
Signature · 1 line
2.19.1
Hi Leonardo,
Thanks for the patch.
Quoted 14 lines · Leonardo Brás
Adds -Wshadow=local on
KBUILD_HOSTCFLAGSto show shadow warnings on tools built for HOST.Signed-off-by Leonardo Brás <[email protected]>--- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)Makefile +0
-0
Makefile HOST_LFS_LIBS := $(shell getconf LFS_LIBS 2>/dev/null) · line 366
Quoted 8 lines
Makefile +1
-1@@ -360,7 +360,7 @@ HOST_LFS_LIBS := $(shell getconf LFS_LIBS 2>/dev/null) HOSTCC = gcc HOSTCXX = g++ -KBUILD_HOSTCFLAGS := -Wall -Wmissing-prototypes -Wstrict-prototypes -O2 \ +KBUILD_HOSTCFLAGS := -Wall -Wshadow=local -Wmissing-prototypes -Wstrict-prototypes -O2 \ -fomit-frame-pointer -std=gnu89 $(HOST_LFS_CFLAGS) \ $(HOSTCFLAGS) KBUILD_HOSTCXXFLAGS := -O2 $(HOST_LFS_CFLAGS) $(HOSTCXXFLAGS)
I believe this should be the last patch on this series and not the first one to avoid commits in between where we have those warnings.
Regards
Helen
Quoted 14 lines · Leonardo Brás
Adds -Wshadow=local on
KBUILD_HOSTCFLAGSto show shadow warnings on tools built for HOST.Signed-off-by Leonardo Brás <[email protected]>--- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)Makefile +0
-0
Makefile HOST_LFS_LIBS := $(shell getconf LFS_LIBS 2>/dev/null) · line 366
Quoted 8 lines
Makefile +1
-1@@ -360,7 +360,7 @@ HOST_LFS_LIBS := $(shell getconf LFS_LIBS 2>/dev/null) HOSTCC = gcc HOSTCXX = g++ -KBUILD_HOSTCFLAGS := -Wall -Wmissing-prototypes -Wstrict-prototypes -O2 \ +KBUILD_HOSTCFLAGS := -Wall -Wshadow=local -Wmissing-prototypes -Wstrict-prototypes -O2 \ -fomit-frame-pointer -std=gnu89 $(HOST_LFS_CFLAGS) \ $(HOSTCFLAGS) KBUILD_HOSTCXXFLAGS := -O2 $(HOST_LFS_CFLAGS) $(HOSTCXXFLAGS)
You might wanna take a look at scripts/Makefile.extrawarn which already has -Wshadow.
Signature · 4 lines
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.Quoted 16 lines · Borislav Petkov
Quoted 14 lines · Leonardo Brás
Adds -Wshadow=local on
KBUILD_HOSTCFLAGSto show shadow warnings on tools built for HOST.Signed-off-by Leonardo Brás <[email protected]>--- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)Makefile +0
-0@@ -360,7 +360,7 @@ HOST_LFS_LIBS := $(shell getconf LFS_LIBS 2>/dev/null)
Quoted 11 lines
Makefile HOST_LFS_LIBS := $(shell getconf LFS_LIBS 2>/dev/null)
Quoted 8 lines
Makefile +1
-2HOSTCC = gcc HOSTCXX = g++ -KBUILD_HOSTCFLAGS := -Wall -Wmissing-prototypes -Wstrict-prototypes -O2 \ +KBUILD_HOSTCFLAGS := -Wall -Wshadow=local -Wmissing-prototypes -Wstrict-prototypes -O2 \ -fomit-frame-pointer -std=gnu89 $(HOST_LFS_CFLAGS) \ $(HOSTCFLAGS) KBUILD_HOSTCXXFLAGS := -O2 $(HOST_LFS_CFLAGS) $(HOSTCXXFLAGS) --You might wanna take a look at
scripts/Makefile.extrawarnwhich already has -Wshadow.
scripts/Makefile.extrawarn provides options for the target compiler (CC), whereas this patch adds -Wshadow=local for the host compiler (HOSTCC).
Signature · 2 lines
Best Regards Masahiro Yamada
Quoted 2 lines · Masahiro Yamada
scripts/Makefile.extrawarnprovides options for the target compiler (CC), whereas this patch adds -Wshadow=local for the host compiler (HOSTCC).
Aha, this wants to fix shadowing for the host tools, ok.
Next question: why not -Wshadow simply?
Also, -Wshadow for the target compiler is an extra warning (W=2). Why is it enabled by default here?
Signature · 4 lines
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.Hello Helen,
Quoted 20 lines · Helen Koike
Hi Leonardo,
Thanks for the patch.
Quoted 14 lines · Leonardo Brás
Adds -Wshadow=local on
KBUILD_HOSTCFLAGSto show shadow warnings on tools built for HOST.Signed-off-by Leonardo Brás <[email protected]>--- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)Makefile +0
-0@@ -360,7 +360,7 @@ HOST_LFS_LIBS := $(shell getconf LFS_LIBS 2>/dev/null)
Quoted 12 lines
Makefile HOST_LFS_LIBS := $(shell getconf LFS_LIBS 2>/dev/null) · line 366
Quoted 8 lines
Makefile +1
-1HOSTCC = gcc HOSTCXX = g++ -KBUILD_HOSTCFLAGS := -Wall -Wmissing-prototypes -Wstrict-prototypes -O2 \ +KBUILD_HOSTCFLAGS := -Wall -Wshadow=local -Wmissing-prototypes -Wstrict-prototypes -O2 \ -fomit-frame-pointer -std=gnu89 $(HOST_LFS_CFLAGS) \ $(HOSTCFLAGS) KBUILD_HOSTCXXFLAGS := -O2 $(HOST_LFS_CFLAGS) $(HOSTCXXFLAGS)I believe this should be the last patch on this series and not the first one to avoid commits in between where we have those warnings.
You are right, I will change the order for v2.
Thanks!
Quoted 2 lines
Regards
Helen
Regards,
Leonardo
Quoted 31 lines · Masahiro Yamada
Quoted 16 lines · Borislav Petkov
Quoted 14 lines · Leonardo Brás
Adds -Wshadow=local on
KBUILD_HOSTCFLAGSto show shadow warnings on tools built for HOST.Signed-off-by Leonardo Brás <[email protected]>--- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)Makefile +0
-0@@ -360,7 +360,7 @@ HOST_LFS_LIBS := $(shell getconf LFS_LIBS 2>/dev/null)Quoted 11 lines
Makefile HOST_LFS_LIBS := $(shell getconf LFS_LIBS 2>/dev/null)
Quoted 8 lines
Makefile +1
-2HOSTCC = gcc HOSTCXX = g++ -KBUILD_HOSTCFLAGS := -Wall -Wmissing-prototypes -Wstrict-prototypes -O2 \ +KBUILD_HOSTCFLAGS := -Wall -Wshadow=local -Wmissing-prototypes -Wstrict-prototypes -O2 \ -fomit-frame-pointer -std=gnu89 $(HOST_LFS_CFLAGS) \ $(HOSTCFLAGS) KBUILD_HOSTCXXFLAGS := -O2 $(HOST_LFS_CFLAGS) $(HOSTCXXFLAGS) --You might wanna take a look at
scripts/Makefile.extrawarnwhich already has -Wshadow.
Quoted 4 lines
scripts/Makefile.extrawarnprovides options for the target compiler (CC), whereas this patch adds -Wshadow=local for the host compiler (HOSTCC).
Thanks for helping, :)
Leonardo Bras
Quoted 4 lines
--
Best Regards
Masahiro Yamada
Hello Boris,
Quoted 1 line
Next question: why not -Wshadow simply?
Good question. I can change it to -Wshadow and fix stuff for v2.
Quoted 3 lines
Also, -Wshadow for the target compiler is an extra warning (W=2). Why is it enabled by default here?
The idea was to put it as default and fix all the shadowing warnings. What do you think? I am open to suggestions.
Thank you,
Leonardo Bras
Quoted 5 lines
--
Regards/Gruss,
Boris.Good mailing practices for 400: avoid top-posting and trim the reply.
Quoted 2 lines · Leonardo Bras
The idea was to put it as default and fix all the shadowing warnings. What do you think? I am open to suggestions.
That's Masahiro's call. In the rest of the kernel, those warnings are behind the W=2 switch - i.e., not enabled by default.
Signature · 4 lines
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.Quoted 7 lines · Borislav Petkov
Quoted 2 lines · Leonardo Bras
The idea was to put it as default and fix all the shadowing warnings. What do you think? I am open to suggestions.
That's Masahiro's call. In the rest of the kernel, those warnings are behind the W=2 switch - i.e., not enabled by default.
It is not realistic to enable this warning option by default. Even -Wshadow=local emits tons of warnings.
(More with -Wshadow)
The problem of this flag is,
it is false positive in macro expansions.
For example, I think the following is a legitimate case.
In file included from ./arch/arm64/include/asm/cputype.h:126:0,
from ./arch/arm64/include/asm/cache.h:19,
from ./include/linux/cache.h:6,
from ./include/linux/printk.h:9,
from ./include/linux/kernel.h:14,
from ./include/linux/bitmap.h:10,
from arch/arm64/kernel/fpsimd.c:20:
arch/arm64/kernel/fpsimd.c: In function ‘sve_kernel_enable’:
./arch/arm64/include/asm/sysreg.h:707:6: warning: declaration of
‘__val’ shadows a previous local [-Wshadow=compatible-local]
u64 __val; \
^
./arch/arm64/include/asm/sysreg.h:717:20: note: in definition of macro
‘write_sysreg’
u64 __val = (u64)(v); \
^
arch/arm64/kernel/fpsimd.c:713:15: note: in expansion of macro ‘read_sysreg’
write_sysreg(read_sysreg(CPACR_EL1) | CPACR_EL1_ZEN_EL1EN, CPACR_EL1);
^~~~~~~~~~~
./arch/arm64/include/asm/sysreg.h:717:6: note: shadowed declaration is here
u64 __val = (u64)(v); \
^arch/arm64/kernel/fpsimd.c:713:2: note: in expansion of macro ‘write_sysreg’
write_sysreg(read_sysreg(CPACR_EL1) | CPACR_EL1_ZEN_EL1EN, CPACR_EL1); ^~~~~~~~~~~~
--
Best Regards
Masahiro Yamada
Quoted 1 line · Masahiro Yamada
It is not realistic to enable this warning option by default.
I believe the question is whether to enable that warning by default in KBUILD_HOSTCFLAGS. Enabling it by default for the target kernel is of course, too noisy. That's why it is hidden behind W=2 there.
Signature · 4 lines
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.Quoted 7 lines · Borislav Petkov
Quoted 1 line · Masahiro Yamada
It is not realistic to enable this warning option by default.
I believe the question is whether to enable that warning by default in
KBUILD_HOSTCFLAGS. Enabling it by default for the target kernel is of course, too noisy. That's why it is hidden behind W=2 there.
Sorry, I misunderstood the question.
The difference about the noisiness between CC and HOSTCC is just comes from the amount of source code.
Adding -Wshadow to KBUILD_HOSTCFLAGS emits another warning in Kconfig. Of course, it is easy to fix.
But, I just started to think this option is a kind of harsh...
--
Best Regards
Masahiro Yamada
Quoted 2 lines · Masahiro Yamada
Adding -Wshadow to
KBUILD_HOSTCFLAGSemits another warning inKconfig. Of course, it is easy to fix.
For v2, I already replaced '-Wshadow=local' for '-Wshadow' and fixed this warning.
Quoted 1 line
But, I just started to think this option is a kind of harsh...
The v2 it's almost done. You think it will be useful, or should I drop it?
Regards,
Leonardo Bras
Quoted 3 lines · Masahiro Yamada
Adding -Wshadow to
KBUILD_HOSTCFLAGSemits another warning inKconfig. Of course, it is easy to fix.
But, I just started to think this option is a kind of harsh...
What is more, if we added -Wshadow to KBUILD_HOSTCFLAGS, then there'll be a difference in build options between host and target kernel in that the host kernel build will be stricter wrt shadowing. Thus, it is a maintainer decision, IMHO.
Signature · 4 lines
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.From: Masahiro Yamada
Quoted 8 lines
Sent: 18 October 2018 17:39
Quoted 5 lines · Borislav Petkov
Quoted 2 lines · Leonardo Bras
The idea was to put it as default and fix all the shadowing warnings. What do you think? I am open to suggestions.
Quoted 7 lines
Quoted 2 lines
That's Masahiro's call. In the rest of the kernel, those warnings are behind the W=2 switch - i.e., not enabled by default.
It is not realistic to enable this warning option by default. Even -Wshadow=local emits tons of warnings.
(More with -Wshadow)
The question is, how many of them are actual bugs. IMHO -Wshadow is a good idea.
Quoted 2 lines
The problem of this flag is,
it is false positive in macro expansions.
Right, but macro expansions inside macro definitions could accidentally use the same local variable - leading to choas.
Quoted 1 line
For example, I think the following is a legitimate case.
...
Quoted 3 lines
arch/arm64/kernel/fpsimd.c:713:2: note: in expansion of macro ‘write_sysreg’
write_sysreg(read_sysreg(CPACR_EL1) | CPACR_EL1_ZEN_EL1EN, CPACR_EL1); ^~~~~~~~~~~~
Easily fixed by using different named temporaries in the two macros. There probably aren't that many macro pairs where that happens. Especially since many are now inlined functions.
It might be that a small number of changes get rid of most of the warnings.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Quoted 18 lines · David Laight
From: Masahiro Yamada
Quoted 8 lines
Sent: 18 October 2018 17:39
Quoted 5 lines · Borislav Petkov
Quoted 2 lines · Leonardo Bras
The idea was to put it as default and fix all the shadowing warnings. What do you think? I am open to suggestions.
Quoted 7 lines
Quoted 2 lines
That's Masahiro's call. In the rest of the kernel, those warnings are behind the W=2 switch - i.e., not enabled by default.
It is not realistic to enable this warning option by default. Even -Wshadow=local emits tons of warnings.
(More with -Wshadow)
Quoted 8 lines
The question is, how many of them are actual bugs. IMHO -Wshadow is a good idea.
Quoted 2 lines
The problem of this flag is,
it is false positive in macro expansions.Right, but macro expansions inside macro definitions could accidentally use the same local variable - leading to choas.
I do not think so.
The macro definitions are surrounded by { ... } so that local variables are properly separated from the outside world.
Quoted 9 lines
Quoted 1 line
For example, I think the following is a legitimate case.
...
Quoted 3 lines
arch/arm64/kernel/fpsimd.c:713:2: note: in expansion of macro ‘write_sysreg’
write_sysreg(read_sysreg(CPACR_EL1) | CPACR_EL1_ZEN_EL1EN, CPACR_EL1); ^~~~~~~~~~~~Easily fixed by using different named temporaries in the two macros. There probably aren't that many macro pairs where that happens. Especially since many are now inlined functions.
But, theoretically, any arbitrary macros could be used in pairs.
This means a new constraint where a local variable name must be unique, it means 'local variable' is not literally 'local'.
I'd like to use short names such as 'x', 'tmp', etc. for local variables.
Quoted 7 lines
It might be that a small number of changes get rid of most of the warnings.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
--
Best Regards
Masahiro Yamada