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

[Nick Desaulniers]

On Tue, Aug 27, 2019 at 4:25 PM Nick Desaulniers
<ndesaulniers@xxxxxxxxxx> wrote:
>
> On Tue, Aug 27, 2019 at 8:49 AM Qian Cai <cai@xxxxxx> 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@xxxxxx>
> > ---
> >  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