[PATCH 1/4] Adds -Wshadow=local on KBUILD_HOSTCFLAGS

[PATCH 1/4] Adds -Wshadow=local on KBUILD_HOSTCFLAGS

[Leonardo Brás]

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)
-- 
2.19.1

Re: [Lkcamp] [PATCH 1/4] Adds -Wshadow=local on KBUILD_HOSTCFLAGS

[Helen Koike]

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

Re: [PATCH 1/4] Adds -Wshadow=local on KBUILD_HOSTCFLAGS

[Borislav Petkov]

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.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH 1/4] Adds -Wshadow=local on KBUILD_HOSTCFLAGS

[Masahiro Yamada]

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).



-- 
Best Regards
Masahiro Yamada

Re: [PATCH 1/4] Adds -Wshadow=local on KBUILD_HOSTCFLAGS

[Borislav Petkov]

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?

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [Lkcamp] [PATCH 1/4] Adds -Wshadow=local on KBUILD_HOSTCFLAGS

[Leonardo Bras]

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

Re: [PATCH 1/4] Adds -Wshadow=local on KBUILD_HOSTCFLAGS

[Leonardo Bras]

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

Re: [PATCH 1/4] Adds -Wshadow=local on KBUILD_HOSTCFLAGS

[Leonardo Bras]

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.

Re: [PATCH 1/4] Adds -Wshadow=local on KBUILD_HOSTCFLAGS

[Borislav Petkov]

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.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH 1/4] Adds -Wshadow=local on KBUILD_HOSTCFLAGS

[Masahiro Yamada]

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

Re: [PATCH 1/4] Adds -Wshadow=local on KBUILD_HOSTCFLAGS

[Borislav Petkov]

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.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH 1/4] Adds -Wshadow=local on KBUILD_HOSTCFLAGS

[Masahiro Yamada]

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

Re: [PATCH 1/4] Adds -Wshadow=local on KBUILD_HOSTCFLAGS

[Leonardo Bras]

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

Re: [PATCH 1/4] Adds -Wshadow=local on KBUILD_HOSTCFLAGS

[Borislav Petkov]

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.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

RE: [PATCH 1/4] Adds -Wshadow=local on KBUILD_HOSTCFLAGS

[David Laight]

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)

Re: [PATCH 1/4] Adds -Wshadow=local on KBUILD_HOSTCFLAGS

[Masahiro Yamada]

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