[PATCH] mm: silence -Woverride-init/initializer-overrides

[PATCH] mm: silence -Woverride-init/initializer-overrides

[Qian Cai]

When compiling a kernel with W=1, there are several of those warnings
due to arm64 override a field by purpose. Just disable those warnings
for both GCC and Clang of this file, so it will help dig "gems" hidden
in the W=1 warnings by reducing some noises.

mm/init-mm.c:39:2: warning: initializer overrides prior initialization
of this subobject [-Winitializer-overrides]
        INIT_MM_CONTEXT(init_mm)
        ^~~~~~~~~~~~~~~~~~~~~~~~
./arch/arm64/include/asm/mmu.h:133:9: note: expanded from macro
'INIT_MM_CONTEXT'
        .pgd = init_pg_dir,
               ^~~~~~~~~~~
mm/init-mm.c:30:10: note: previous initialization is here
        .pgd            = swapper_pg_dir,
                          ^~~~~~~~~~~~~~

Note: there is a side project trying to support explicitly allowing
specific initializer overrides in Clang, but there is no guarantee it
will happen or not.

https://github.com/ClangBuiltLinux/linux/issues/639

Signed-off-by: Qian Cai <cai@lca.pw>
---
 mm/Makefile | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/mm/Makefile b/mm/Makefile
index d0b295c3b764..5a30b8ecdc55 100644
--- a/mm/Makefile
+++ b/mm/Makefile
@@ -21,6 +21,9 @@ KCOV_INSTRUMENT_memcontrol.o := n
 KCOV_INSTRUMENT_mmzone.o := n
 KCOV_INSTRUMENT_vmstat.o := n
 
+CFLAGS_init-mm.o += $(call cc-disable-warning, override-init)
+CFLAGS_init-mm.o += $(call cc-disable-warning, initializer-overrides)
+
 mmu-y			:= nommu.o
 mmu-$(CONFIG_MMU)	:= highmem.o memory.o mincore.o \
 			   mlock.o mmap.o mmu_gather.o mprotect.o mremap.o \
-- 
1.8.3.1

Re: [PATCH] mm: silence -Woverride-init/initializer-overrides

[Nick Desaulniers]

On Tue, Aug 27, 2019 at 8:49 AM Qian Cai <cai@lca.pw> wrote:
>
> When compiling a kernel with W=1, there are several of those warnings
> due to arm64 override a field by purpose. Just disable those warnings
> for both GCC and Clang of this file, so it will help dig "gems" hidden
> in the W=1 warnings by reducing some noises.
>
> mm/init-mm.c:39:2: warning: initializer overrides prior initialization
> of this subobject [-Winitializer-overrides]
>         INIT_MM_CONTEXT(init_mm)
>         ^~~~~~~~~~~~~~~~~~~~~~~~
> ./arch/arm64/include/asm/mmu.h:133:9: note: expanded from macro
> 'INIT_MM_CONTEXT'
>         .pgd = init_pg_dir,
>                ^~~~~~~~~~~
> mm/init-mm.c:30:10: note: previous initialization is here
>         .pgd            = swapper_pg_dir,
>                           ^~~~~~~~~~~~~~
>
> Note: there is a side project trying to support explicitly allowing
> specific initializer overrides in Clang, but there is no guarantee it
> will happen or not.
>
> https://github.com/ClangBuiltLinux/linux/issues/639
>
> Signed-off-by: Qian Cai <cai@lca.pw>
> ---
>  mm/Makefile | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/mm/Makefile b/mm/Makefile
> index d0b295c3b764..5a30b8ecdc55 100644
> --- a/mm/Makefile
> +++ b/mm/Makefile

Hi Qian, thanks for the patch.
Rather than disable the warning outright, and bury the disabling in a
directory specific Makefile, why not move it to W=2 in
scripts/Makefile.extrawarn?


I think even better would be to use pragma's to disable the warning in
mm/init.c.  Looks like __diag support was never ported for clang yet
from include/linux/compiler-gcc.h to include/linux/compiler-clang.h.

Then you could do:

 28 struct mm_struct init_mm = {
 29   .mm_rb    = RB_ROOT,
 30   .pgd    = swapper_pg_dir,
 31   .mm_users = ATOMIC_INIT(2),
 32   .mm_count = ATOMIC_INIT(1),
 33   .mmap_sem = __RWSEM_INITIALIZER(init_mm.mmap_sem),
 34   .page_table_lock =
__SPIN_LOCK_UNLOCKED(init_mm.page_table_lock),
 35   .arg_lock =  __SPIN_LOCK_UNLOCKED(init_mm.arg_lock),
 36   .mmlist   = LIST_HEAD_INIT(init_mm.mmlist),
 37   .user_ns  = &init_user_ns,
 38   .cpu_bitmap = { [BITS_TO_LONGS(NR_CPUS)] = 0},
__diag_push();
__diag_ignore(CLANG, 4, "-Winitializer-overrides")
 39   INIT_MM_CONTEXT(init_mm)
__diag_pop();
 40 };


I mean, the arm64 case is not a bug, but I worry about turning this
warning off.  I'd expect it to only warn once during an arm64 build,
so does the warning really detract from "W=1 gem finding?"

> @@ -21,6 +21,9 @@ KCOV_INSTRUMENT_memcontrol.o := n
>  KCOV_INSTRUMENT_mmzone.o := n
>  KCOV_INSTRUMENT_vmstat.o := n
>
> +CFLAGS_init-mm.o += $(call cc-disable-warning, override-init)

-Woverride-init isn't mentioned in the commit message, so not sure if
it's meant to ride along?

> +CFLAGS_init-mm.o += $(call cc-disable-warning, initializer-overrides)
> +

-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] mm: silence -Woverride-init/initializer-overrides

[Nick Desaulniers]

On Tue, Aug 27, 2019 at 4:25 PM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Tue, Aug 27, 2019 at 8:49 AM Qian Cai <cai@lca.pw> wrote:
> >
> > When compiling a kernel with W=1, there are several of those warnings
> > due to arm64 override a field by purpose. Just disable those warnings
> > for both GCC and Clang of this file, so it will help dig "gems" hidden
> > in the W=1 warnings by reducing some noises.
> >
> > mm/init-mm.c:39:2: warning: initializer overrides prior initialization
> > of this subobject [-Winitializer-overrides]
> >         INIT_MM_CONTEXT(init_mm)
> >         ^~~~~~~~~~~~~~~~~~~~~~~~
> > ./arch/arm64/include/asm/mmu.h:133:9: note: expanded from macro
> > 'INIT_MM_CONTEXT'
> >         .pgd = init_pg_dir,
> >                ^~~~~~~~~~~
> > mm/init-mm.c:30:10: note: previous initialization is here
> >         .pgd            = swapper_pg_dir,
> >                           ^~~~~~~~~~~~~~
> >
> > Note: there is a side project trying to support explicitly allowing
> > specific initializer overrides in Clang, but there is no guarantee it
> > will happen or not.
> >
> > https://github.com/ClangBuiltLinux/linux/issues/639
> >
> > Signed-off-by: Qian Cai <cai@lca.pw>
> > ---
> >  mm/Makefile | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/mm/Makefile b/mm/Makefile
> > index d0b295c3b764..5a30b8ecdc55 100644
> > --- a/mm/Makefile
> > +++ b/mm/Makefile
>
> Hi Qian, thanks for the patch.
> Rather than disable the warning outright, and bury the disabling in a
> directory specific Makefile, why not move it to W=2 in
> scripts/Makefile.extrawarn?
>
>
> I think even better would be to use pragma's to disable the warning in
> mm/init.c.  Looks like __diag support was never ported for clang yet
> from include/linux/compiler-gcc.h to include/linux/compiler-clang.h.
>
> Then you could do:
>
>  28 struct mm_struct init_mm = {
>  29   .mm_rb    = RB_ROOT,
>  30   .pgd    = swapper_pg_dir,
>  31   .mm_users = ATOMIC_INIT(2),
>  32   .mm_count = ATOMIC_INIT(1),
>  33   .mmap_sem = __RWSEM_INITIALIZER(init_mm.mmap_sem),
>  34   .page_table_lock =
> __SPIN_LOCK_UNLOCKED(init_mm.page_table_lock),
>  35   .arg_lock =  __SPIN_LOCK_UNLOCKED(init_mm.arg_lock),
>  36   .mmlist   = LIST_HEAD_INIT(init_mm.mmlist),
>  37   .user_ns  = &init_user_ns,
>  38   .cpu_bitmap = { [BITS_TO_LONGS(NR_CPUS)] = 0},
> __diag_push();
> __diag_ignore(CLANG, 4, "-Winitializer-overrides")
>  39   INIT_MM_CONTEXT(init_mm)
> __diag_pop();
>  40 };
>
>
> I mean, the arm64 case is not a bug, but I worry about turning this
> warning off.  I'd expect it to only warn once during an arm64 build,
> so does the warning really detract from "W=1 gem finding?"
>
> > @@ -21,6 +21,9 @@ KCOV_INSTRUMENT_memcontrol.o := n
> >  KCOV_INSTRUMENT_mmzone.o := n
> >  KCOV_INSTRUMENT_vmstat.o := n
> >
> > +CFLAGS_init-mm.o += $(call cc-disable-warning, override-init)
>
> -Woverride-init isn't mentioned in the commit message, so not sure if
> it's meant to ride along?
>
> > +CFLAGS_init-mm.o += $(call cc-disable-warning, initializer-overrides)

That said, it's not too bad to disable it for one object file that
contains a single struct definition.

-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] mm: silence -Woverride-init/initializer-overrides

[Qian Cai]

> On Aug 27, 2019, at 7:25 PM, Nick Desaulniers <ndesaulniers@google.com> wrote:
> 
> On Tue, Aug 27, 2019 at 8:49 AM Qian Cai <cai@lca.pw> wrote:
>> 
>> When compiling a kernel with W=1, there are several of those warnings
>> due to arm64 override a field by purpose. Just disable those warnings
>> for both GCC and Clang of this file, so it will help dig "gems" hidden
>> in the W=1 warnings by reducing some noises.
>> 
>> mm/init-mm.c:39:2: warning: initializer overrides prior initialization
>> of this subobject [-Winitializer-overrides]
>>        INIT_MM_CONTEXT(init_mm)
>>        ^~~~~~~~~~~~~~~~~~~~~~~~
>> ./arch/arm64/include/asm/mmu.h:133:9: note: expanded from macro
>> 'INIT_MM_CONTEXT'
>>        .pgd = init_pg_dir,
>>               ^~~~~~~~~~~
>> mm/init-mm.c:30:10: note: previous initialization is here
>>        .pgd            = swapper_pg_dir,
>>                          ^~~~~~~~~~~~~~
>> 
>> Note: there is a side project trying to support explicitly allowing
>> specific initializer overrides in Clang, but there is no guarantee it
>> will happen or not.
>> 
>> https://github.com/ClangBuiltLinux/linux/issues/639
>> 
>> Signed-off-by: Qian Cai <cai@lca.pw>
>> ---
>> mm/Makefile | 3 +++
>> 1 file changed, 3 insertions(+)
>> 
>> diff --git a/mm/Makefile b/mm/Makefile
>> index d0b295c3b764..5a30b8ecdc55 100644
>> --- a/mm/Makefile
>> +++ b/mm/Makefile
> 
> Hi Qian, thanks for the patch.
> Rather than disable the warning outright, and bury the disabling in a
> directory specific Makefile, why not move it to W=2 in
> scripts/Makefile.extrawarn?

It could still be useful to have -Woverride-init/initializer-overrides in W=1
for people only running W=1 to catch some real developer mistakes. W=2
might be too noisy to start with.

> 
> 
> I think even better would be to use pragma's to disable the warning in
> mm/init.c.  Looks like __diag support was never ported for clang yet
> from include/linux/compiler-gcc.h to include/linux/compiler-clang.h.
> 
> Then you could do:
> 
> 28 struct mm_struct init_mm = {
> 29   .mm_rb    = RB_ROOT,
> 30   .pgd    = swapper_pg_dir,
> 31   .mm_users = ATOMIC_INIT(2),
> 32   .mm_count = ATOMIC_INIT(1),
> 33   .mmap_sem = __RWSEM_INITIALIZER(init_mm.mmap_sem),
> 34   .page_table_lock =
> __SPIN_LOCK_UNLOCKED(init_mm.page_table_lock),
> 35   .arg_lock =  __SPIN_LOCK_UNLOCKED(init_mm.arg_lock),
> 36   .mmlist   = LIST_HEAD_INIT(init_mm.mmlist),
> 37   .user_ns  = &init_user_ns,
> 38   .cpu_bitmap = { [BITS_TO_LONGS(NR_CPUS)] = 0},
> __diag_push();
> __diag_ignore(CLANG, 4, "-Winitializer-overrides")
> 39   INIT_MM_CONTEXT(init_mm)
> __diag_pop();
> 40 };

The pragma might be fine for Clang, although it seems a bit overkill.
Then, it needs to add something for GCC’s "override-init" as well.
If that mm_init.c grows in the future to have more structs, it may become
more desirable to use “pragma” to only disable this particular struct.

> 
> I mean, the arm64 case is not a bug, but I worry about turning this
> warning off.  I'd expect it to only warn once during an arm64 build,
> so does the warning really detract from "W=1 gem finding?”

I am running this every day and seeing this every time, so definitely
appreciate disabling it in the kernel itself if not adding too much work
for maintainers. See the end of this file for my current filtering,

https://github.com/cailca/linux-mm/blob/master/compile.sh

> 
>> @@ -21,6 +21,9 @@ KCOV_INSTRUMENT_memcontrol.o := n
>> KCOV_INSTRUMENT_mmzone.o := n
>> KCOV_INSTRUMENT_vmstat.o := n
>> 
>> +CFLAGS_init-mm.o += $(call cc-disable-warning, override-init)
> 
> -Woverride-init isn't mentioned in the commit message, so not sure if
> it's meant to ride along?

Yes, I did also mention GCC will also warn those (from -Woverride-init) but
did not include in the warning output which seems redundant.