Message ID | 1566920867-27453-1-git-send-email-cai@lca.pw (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | mm: silence -Woverride-init/initializer-overrides | expand |
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) > +
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.
> 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.
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 \
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(+)