linux-kernel - Re: [PATCH] mm/slub: quiet the clang warning with -Wunused-function enabled

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CAJuCfpG08jmspY11dGej5Wdz2hT4iC=nr4V3HH-S4YfhXV7wyg@mail.gmail.com>
Date: Thu, 11 Jul 2024 10:06:19 -0700
From: Suren Baghdasaryan <surenb@...gle.com>
To: Vlastimil Babka <vbabka@...e.cz>
Cc: Nathan Chancellor <nathan@...nel.org>, Matthew Wilcox <willy@...radead.org>, sxwjean@...com, cl@...ux.co, 
	penberg@...nel.org, rientjes@...gle.com, iamjoonsoo.kim@....com, 
	akpm@...ux-foundation.org, roman.gushchin@...ux.dev, 42.hyeyoo@...il.com, 
	xiongwei.song@...ux.dev, linux-mm@...ck.org, linux-kernel@...r.kernel.org, 
	kernel test robot <lkp@...el.com>, llvm@...ts.linux.dev, 
	Nick Desaulniers <ndesaulniers@...gle.com>
Subject: Re: [PATCH] mm/slub: quiet the clang warning with -Wunused-function enabled

On Thu, Jul 11, 2024 at 6:56 AM Suren Baghdasaryan <surenb@...gle.com> wrote:
>
> On Thu, Jul 11, 2024 at 12:43 AM Vlastimil Babka <vbabka@...e.cz> wrote:
> >
> > On 7/10/24 11:40 PM, Suren Baghdasaryan wrote:
> > > On Wed, Jul 10, 2024 at 8:02 AM Nathan Chancellor <nathan@...nel.org> wrote:
> > >>
> > >> On Wed, Jul 10, 2024 at 04:03:33AM +0100, Matthew Wilcox wrote:
> > >> > On Wed, Jul 10, 2024 at 10:54:18AM +0800, sxwjean@...com wrote:
> > >> > > From: Xiongwei Song <xiongwei.song@...ux.dev>
> > >> > >
> > >> > > The only user of prepare_slab_obj_exts_hook() is
> > >> > > alloc_tagging_slab_alloc_hook(), which can build with
> > >> > > CONFIG_MEM_ALLOC_PROFILING enabled. So, the warning was triggerred
> > >> > > when disabling CONFIG_MEM_ALLOC_PROFILING. Let's add "__maybe_unused"
> > >> > > for prepare_slab_obj_exts_hook().
> > >> >
> > >> > Perhaps instead clang can be fixed to match gcc's behaviour?
> > >>
> > >> Clang only differs from GCC on warning for unused static inline functions in .c
> > >> files, not .h files. The kernel already handles this in
> > >> include/linux/compiler_types.h but it disables this workaround for W=1 to catch
> > >> unused functions like this as a result of commit 6863f5643dd7 ("kbuild: allow
> > >> Clang to find unused static inline functions for W=1 build"):
> > >>
> > >> /*
> > >>  * GCC does not warn about unused static inline functions for -Wunused-function.
> > >>  * Suppress the warning in clang as well by using __maybe_unused, but enable it
> > >>  * for W=1 build. This will allow clang to find unused functions. Remove the
> > >>  * __inline_maybe_unused entirely after fixing most of -Wunused-function warnings.
> > >>  */
> > >> #ifdef KBUILD_EXTRA_WARN1
> > >> #define __inline_maybe_unused
> > >> #else
> > >> #define __inline_maybe_unused __maybe_unused
> > >> #endif
> > >>
> > >> So I don't really think there is much for clang to do here and I think having
> > >> the ability to find unused static inline functions in .c files is useful (you
> > >> might disagree, perhaps a revert could still be discussed). I guess
> > >> IS_ENABLED() can't be used there, so it seems like either taking this patch,
> > >> ignoring the warning, or refactoring the code in some other way are the only
> > >> options I see.
> > >
> > > I think this is the consequence of the recent refactoring I've done in
> > > https://lore.kernel.org/all/20240704135941.1145038-1-surenb@google.com/.
> > > There should be a cleaner way to fix this. I'll post it later today or
> > > tomorrow morning.
> >
> > Yeah looks like the non-empty prepare_slab_obj_exts_hook() could move to the
> > #ifdef CONFIG_MEM_ALLOC_PROFILING section above
> > alloc_tagging_slab_alloc_hook() and the empty one just removed.
>
> Exactly my plan. I'll post a patch once I reach the office.

Actually I was wrong and the problem exists even without my
refactoring in [1]. I posted the fix at [2] and I based it on
slab/for-next because that's the only branch that contains [1]. Not
because [2] requires [1] but because they are changing adjacent code,
so would create merge problems if merged separately.

[1] https://lore.kernel.org/all/20240704135941.1145038-1-surenb@google.com/
[2] https://lore.kernel.org/all/20240711170216.1149695-1-surenb@google.com/

>
> >
> > > Thanks,
> > > Suren.
> > >
> > >>
> > >> > > Reported-by: kernel test robot <lkp@...el.com>
> > >> > > Closes: https://lore.kernel.org/oe-kbuild-all/202407050845.zNONqauD-lkp@intel.com/
> > >> > > Signed-off-by: Xiongwei Song <xiongwei.song@...ux.dev>
> > >> > > ---
> > >> > >  mm/slub.c | 4 ++--
> > >> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >> > >
> > >> > > diff --git a/mm/slub.c b/mm/slub.c
> > >> > > index ce39544acf7c..2e26f20759c0 100644
> > >> > > --- a/mm/slub.c
> > >> > > +++ b/mm/slub.c
> > >> > > @@ -2027,7 +2027,7 @@ static inline bool need_slab_obj_ext(void)
> > >> > >     return false;
> > >> > >  }
> > >> > >
> > >> > > -static inline struct slabobj_ext *
> > >> > > +static inline struct slabobj_ext * __maybe_unused
> > >> > >  prepare_slab_obj_exts_hook(struct kmem_cache *s, gfp_t flags, void *p)
> > >> > >  {
> > >> > >     struct slab *slab;
> > >> > > @@ -2068,7 +2068,7 @@ static inline bool need_slab_obj_ext(void)
> > >> > >     return false;
> > >> > >  }
> > >> > >
> > >> > > -static inline struct slabobj_ext *
> > >> > > +static inline struct slabobj_ext * __maybe_unused
> > >> > >  prepare_slab_obj_exts_hook(struct kmem_cache *s, gfp_t flags, void *p)
> > >> > >  {
> > >> > >     return NULL;
> > >> > > --
> > >> > > 2.34.1
> > >> > >
> > >> > >
> >