Re: [PATCH] lib: bitmap: Mute some odd section mismatch warning in xtensa kernel build

[Yury Norov]

On Sun, Aug 15, 2021 at 03:21:32PM +1200, Barry Song wrote:
> From: Barry Song <song.bao.hua@xxxxxxxxxxxxx>
> 
> Constanly there are some section mismatch issues reported in test_bitmap
> for xtensa platform such as:
> 
>   Section mismatch in reference from the function bitmap_equal() to the
>   variable .init.data:initcall_level_names
>   The function bitmap_equal() references the variable __initconst
>   __setup_str_initcall_blacklist. This is often because bitmap_equal
>   lacks a __initconst annotation or the annotation of
>   __setup_str_initcall_blacklist is wrong.
> 
>   Section mismatch in reference from the function bitmap_copy_clear_tail()
>   to the variable .init.rodata:__setup_str_initcall_blacklist
>   The function bitmap_copy_clear_tail() references the variable __initconst
>   __setup_str_initcall_blacklist.
>   This is often because bitmap_copy_clear_tail lacks a __initconst
>   annotation or the annotation of __setup_str_initcall_blacklist is wrong.
> 
> To be honest, hardly to believe kernel code is wrong since bitmap_equal is
> always called in __init function in test_bitmap.c just like __bitmap_equal.
> But gcc doesn't report any issue for __bitmap_equal even when bitmap_equal
> and __bitmap_equal show in the same function such as:
> 
>   static void noinline __init test_mem_optimisations(void)
>   {
> 	...
>           for (start = 0; start < 1024; start += 8) {
>                   for (nbits = 0; nbits < 1024 - start; nbits += 8) {
>                           if (!bitmap_equal(bmap1, bmap2, 1024)) {
>                                   failed_tests++;
>                           }
>                           if (!__bitmap_equal(bmap1, bmap2, 1024)) {
>                                   failed_tests++;
>                           }
>   			...
>                   }
>           }
>   }
> 
> The different between __bitmap_equal() and bitmap_equal() is that the
> former is extern and a EXPORT_SYMBOL. So noinline, and probably in fact
> noclone. But the later is static and unfortunately not inlined at this
> time though it has a "inline" flag.
> 
> bitmap_copy_clear_tail(), on the other hand, seems more innocent as it is
> accessing stack only by its wrapper bitmap_from_arr32() in function
> test_bitmap_arr32():
> static void __init test_bitmap_arr32(void)
> {
>         unsigned int nbits, next_bit;
>         u32 arr[EXP1_IN_BITS / 32];
>         DECLARE_BITMAP(bmap2, EXP1_IN_BITS);
> 
>         memset(arr, 0xa5, sizeof(arr));
> 
>         for (nbits = 0; nbits < EXP1_IN_BITS; ++nbits) {
>                 bitmap_to_arr32(arr, exp1, nbits);
>                 bitmap_from_arr32(bmap2, arr, nbits);
>                 expect_eq_bitmap(bmap2, exp1, nbits);
> 		...
>         }
> }
> Looks like gcc optimized arr, bmap2 things to .init.data but it seems
> nothing is wrong in kernel since test_bitmap_arr32() is __init.
> 
> Max Filippov reported a bug to gcc but gcc people don't ack. So here
> this patch removes the involved symbols by forcing inline. It might
> not be that elegant but I don't see any harm as bitmap_equal() and
> bitmap_copy_clear_tail() are both quite small. In addition, kernel
> doc also backs this modification "We don't use the 'inline' keyword
> because it's broken": www.kernel.org/doc/local/inline.html

This is a 2006 article. Are you sure nothing has been changed over the
last 15 years?
 
> Another possible way to "fix" the warning is moving the involved
> symboms to lib/bitmap.c:

So, it's a GCC issue already reported to GCC? For me it sounds like
nothing to fix in kernel. If I was a GCC developer, I'd prefer to have
all bugs clearly reproducible. 

Let's wait for GCC and xtensa people comments. (CC xtensa and GCC
lists)

Yury

>   +int bitmap_equal(const unsigned long *src1,
>   +                       const unsigned long *src2, unsigned int nbits)
>   +{
>   +       if (small_const_nbits(nbits))
>   +               return !((*src1 ^ *src2) & BITMAP_LAST_WORD_MASK(nbits));
>   +       if (__builtin_constant_p(nbits & BITMAP_MEM_MASK) &&
>   +           IS_ALIGNED(nbits, BITMAP_MEM_ALIGNMENT))
>   +               return !memcmp(src1, src2, nbits / 8);
>   +       return __bitmap_equal(src1, src2, nbits);
>   +}
>   +EXPORT_SYMBOL(bitmap_equal);
> 
> This is harmful to the performance.
> 
> Reported-by: kernel test robot <lkp@xxxxxxxxx>
> Cc: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> Cc: Max Filippov <jcmvbkbc@xxxxxxxxx>
> Cc: Andrew Pinski <pinskia@xxxxxxxxx>
> Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92938
> Signed-off-by: Barry Song <song.bao.hua@xxxxxxxxxxxxx>
> ---
>  include/linux/bitmap.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
> index 37f36dad18bd..3eec9f68a0b6 100644
> --- a/include/linux/bitmap.h
> +++ b/include/linux/bitmap.h
> @@ -258,7 +258,7 @@ static inline void bitmap_copy(unsigned long *dst, const unsigned long *src,
>  /*
>   * Copy bitmap and clear tail bits in last word.
>   */
> -static inline void bitmap_copy_clear_tail(unsigned long *dst,
> +static __always_inline void bitmap_copy_clear_tail(unsigned long *dst,
>  		const unsigned long *src, unsigned int nbits)
>  {
>  	bitmap_copy(dst, src, nbits);
> @@ -334,7 +334,7 @@ static inline void bitmap_complement(unsigned long *dst, const unsigned long *sr
>  #endif
>  #define BITMAP_MEM_MASK (BITMAP_MEM_ALIGNMENT - 1)
>  
> -static inline int bitmap_equal(const unsigned long *src1,
> +static __always_inline int bitmap_equal(const unsigned long *src1,
>  			const unsigned long *src2, unsigned int nbits)
>  {
>  	if (small_const_nbits(nbits))
> -- 
> 2.25.1