| Message ID | 20181017000809.GA21292@WindFlash (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Adds -Wshadow=local on KBUILD_HOSTCFLAGS | expand |
Hi Leonardo, Thanks for the patch. On 10/16/18 9:08 PM, Leonardo Brás wrote: > Adds -Wshadow=local on KBUILD_HOSTCFLAGS to show shadow warnings > on tools built for HOST. > > Signed-off-by: Leonardo Brás <leobras.c@gmail.com> > --- > Makefile | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/Makefile b/Makefile > index e8b599b4dcde..fb0a9ac195e7 100644 > --- a/Makefile > +++ b/Makefile > @@ -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
On Tue, Oct 16, 2018 at 09:08:09PM -0300, Leonardo Brás wrote: > Adds -Wshadow=local on KBUILD_HOSTCFLAGS to show shadow warnings > on tools built for HOST. > > Signed-off-by: Leonardo Brás <leobras.c@gmail.com> > --- > Makefile | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/Makefile b/Makefile > index e8b599b4dcde..fb0a9ac195e7 100644 > --- a/Makefile > +++ b/Makefile > @@ -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.
On Wed, Oct 17, 2018 at 5:11 PM Borislav Petkov <bp@alien8.de> wrote: > > On Tue, Oct 16, 2018 at 09:08:09PM -0300, Leonardo Brás wrote: > > Adds -Wshadow=local on KBUILD_HOSTCFLAGS to show shadow warnings > > on tools built for HOST. > > > > Signed-off-by: Leonardo Brás <leobras.c@gmail.com> > > --- > > Makefile | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/Makefile b/Makefile > > index e8b599b4dcde..fb0a9ac195e7 100644 > > --- a/Makefile > > +++ b/Makefile > > @@ -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. scripts/Makefile.extrawarn provides options for the target compiler (CC), whereas this patch adds -Wshadow=local for the host compiler (HOSTCC).
On Wed, Oct 17, 2018 at 05:21:01PM +0900, Masahiro Yamada wrote: > scripts/Makefile.extrawarn provides 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?
Hello Helen, On Wed, Oct 17, 2018 at 1:32 AM Helen Koike <helen@koikeco.de> wrote: > > Hi Leonardo, > > Thanks for the patch. > > On 10/16/18 9:08 PM, Leonardo Brás wrote: > > Adds -Wshadow=local on KBUILD_HOSTCFLAGS to show shadow warnings > > on tools built for HOST. > > > > Signed-off-by: Leonardo Brás <leobras.c@gmail.com> > > --- > > Makefile | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/Makefile b/Makefile > > index e8b599b4dcde..fb0a9ac195e7 100644 > > --- a/Makefile > > +++ b/Makefile > > @@ -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. > You are right, I will change the order for v2. Thanks! > Regards > Helen Regards, Leonardo
On Wed, Oct 17, 2018 at 5:21 AM Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > > On Wed, Oct 17, 2018 at 5:11 PM Borislav Petkov <bp@alien8.de> wrote: > > > > On Tue, Oct 16, 2018 at 09:08:09PM -0300, Leonardo Brás wrote: > > > Adds -Wshadow=local on KBUILD_HOSTCFLAGS to show shadow warnings > > > on tools built for HOST. > > > > > > Signed-off-by: Leonardo Brás <leobras.c@gmail.com> > > > --- > > > Makefile | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/Makefile b/Makefile > > > index e8b599b4dcde..fb0a9ac195e7 100644 > > > --- a/Makefile > > > +++ b/Makefile > > > @@ -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. > > > scripts/Makefile.extrawarn provides options for the target compiler (CC), > whereas this patch adds -Wshadow=local for the host compiler (HOSTCC). > > Thanks for helping, :) Leonardo Bras > > -- > Best Regards > Masahiro Yamada
Hello Boris, > Next question: why not -Wshadow simply? Good question. I can change it to -Wshadow and fix stuff for v2. > > 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 > -- > Regards/Gruss, > Boris. > > Good mailing practices for 400: avoid top-posting and trim the reply.
On Wed, Oct 17, 2018 at 09:40:53PM -0300, Leonardo Bras wrote: > 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.
On Thu, Oct 18, 2018 at 6:18 PM Borislav Petkov <bp@alien8.de> wrote: > > On Wed, Oct 17, 2018 at 09:40:53PM -0300, Leonardo Bras wrote: > > 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
On Fri, Oct 19, 2018 at 01:39:21AM +0900, Masahiro Yamada wrote:
> 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.
On Fri, Oct 19, 2018 at 1:53 AM Borislav Petkov <bp@alien8.de> wrote: > > On Fri, Oct 19, 2018 at 01:39:21AM +0900, Masahiro Yamada wrote: > > 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
On Thu, Oct 18, 2018 at 11:42 PM Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > Adding -Wshadow to KBUILD_HOSTCFLAGS emits another warning in Kconfig. > Of course, it is easy to fix. For v2, I already replaced '-Wshadow=local' for '-Wshadow' and fixed this warning. > 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
On Fri, Oct 19, 2018 at 11:41:31AM +0900, Masahiro Yamada wrote: > 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... 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.
From: Masahiro Yamada > Sent: 18 October 2018 17:39 > > On Thu, Oct 18, 2018 at 6:18 PM Borislav Petkov <bp@alien8.de> wrote: > > > > On Wed, Oct 17, 2018 at 09:40:53PM -0300, Leonardo Bras wrote: > > > 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 question is, how many of them are actual bugs. IMHO -Wshadow is a good idea. > 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. > For example, I think the following is a legitimate case. ... > 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)
On Fri, Oct 19, 2018 at 8:28 PM David Laight <David.Laight@aculab.com> wrote: > > From: Masahiro Yamada > > Sent: 18 October 2018 17:39 > > > > On Thu, Oct 18, 2018 at 6:18 PM Borislav Petkov <bp@alien8.de> wrote: > > > > > > On Wed, Oct 17, 2018 at 09:40:53PM -0300, Leonardo Bras wrote: > > > > 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 question is, how many of them are actual bugs. > IMHO -Wshadow is a good idea. > > > 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. > > For example, I think the following is a legitimate case. > ... > > 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. > 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
diff --git a/Makefile b/Makefile index e8b599b4dcde..fb0a9ac195e7 100644 --- a/Makefile +++ b/Makefile @@ -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)
Adds -Wshadow=local on KBUILD_HOSTCFLAGS to show shadow warnings on tools built for HOST. Signed-off-by: Leonardo Brás <leobras.c@gmail.com> --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)