* Re: [PATCH] mm: silence -Woverride-init/initializer-overrides
2019-08-27 23:25 ` Nick Desaulniers
@ 2019-08-27 23:29 ` Nick Desaulniers
2019-08-28 1:07 ` Qian Cai
1 sibling, 0 replies; 4+ messages in thread
From: Nick Desaulniers @ 2019-08-27 23:29 UTC (permalink / raw)
To: Qian Cai, Masahiro Yamada
Cc: Andrew Morton, clang-built-linux, Linux Memory Management List,
LKML, Mark Rutland, Arnd Bergmann
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
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] mm: silence -Woverride-init/initializer-overrides
2019-08-27 23:25 ` Nick Desaulniers
2019-08-27 23:29 ` Nick Desaulniers
@ 2019-08-28 1:07 ` Qian Cai
1 sibling, 0 replies; 4+ messages in thread
From: Qian Cai @ 2019-08-28 1:07 UTC (permalink / raw)
To: Nick Desaulniers
Cc: Masahiro Yamada, Andrew Morton, clang-built-linux,
Linux Memory Management List, LKML, Mark Rutland, Arnd Bergmann
> 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.
^ permalink raw reply [flat|nested] 4+ messages in thread