Re: [PATCH] lib: bitmap: Mute some odd section mismatch warning in xtensa kernel build
On Sun, Aug 15, 2021 at 03:21:32PM +1200, Barry Song wrote:
> From: Barry Song
>
> 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
> Cc: Andy Shevchenko
> Cc: Max Filippov
> Cc: Andrew Pinski
> Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92938
> Signed-off-by: Barry Song
> ---
> 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 __alway
Re: [PATCH] linux/find: ignore -Wtype-limits to reduce W=2 warnings by 34% tree-wide
+ gcc@gcc.gnu.org
+ Rikard Falkeborn
On Wed, Apr 27, 2022 at 01:16:58AM +0900, Vincent Mailhol wrote:
> find_first_bit(), find_first_and_bit(), find_first_and_bit() and
> find_first_and_bit() all invokes GENMASK(size - 1, 0).
>
> This triggers below warning when compiled with W=2.
>
> | ./include/linux/find.h: In function 'find_first_bit':
> | ./include/linux/bits.h:25:36: warning: comparison of unsigned
> | expression in '< 0' is always false [-Wtype-limits]
> |25 | __is_constexpr((l) > (h)), (l) > (h), 0)))
> | |^
> | ./include/linux/build_bug.h:16:62: note: in definition of macro
> | 'BUILD_BUG_ON_ZERO'
> |16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e));
> })))
> | | ^
> | ./include/linux/bits.h:25:17: note: in expansion of macro '__is_constexpr'
> |25 | __is_constexpr((l) > (h)), (l) > (h), 0)))
> | | ^~
> | ./include/linux/bits.h:38:10: note: in expansion of macro
> 'GENMASK_INPUT_CHECK'
> |38 | (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> | | ^~~
> | ./include/linux/find.h:119:45: note: in expansion of macro 'GENMASK'
> | 119 | unsigned long val = *addr & GENMASK(size - 1, 0);
> | | ^~~
>
> linux/find.h being a widely used header file, above warning show up in
> thousand of files which include this header (either directly on
> indirectly).
>
> Because it is a false positive, we just silence -Wtype-limits flag
> locally to remove the spam. clang does not warn about it, so we just
> apply the diag_ignore() directive to gcc.
>
> By doing so, the warnings for a W=2 build are reduced by
> 34%. Benchmark was done with gcc 11.2.1 on Linux v5.17 x86_64
> allyesconfig (except CONFIG_WERROR). Beforethe patch: 515496 warnings
> and after: 340097.
>
> For reference, past proposal to modify GENMASK_INPUT_CHECK() was
> rejected in:
> https://lore.kernel.org/all/20220304124416.1181029-1-mailhol.vinc...@wanadoo.fr/
So, here is nothing wrong with the kernel code and we have an alternative
compiler (clang) that doesn't throw Wtype-limits. It sounds to me like an
internal GCC problem, and I don't understand how hiding broken Wtype-limits
on kernel side would help people to improve GCC.
On the thread you mentioned above:
> > > > Have you fixed W=1 warnings?
> > > > Without fixing W=1 (which makes much more sense, when used with
> > > > WERROR=y && COMPILE_TEST=y) this has no value.
> > >
> > > How is this connected?
> >
> > By priorities.
> > I don't see much value in fixing W=2 per se if the code doesn't compile for
> > W=1.
>
> *My code* compiles for W=1. For me, fixing this W=2 in the next in line
> if speaking of priorities.
>
> I do not understand why I should be forbidden to fix a W=2 in the
> file which I am maintaining on the grounds that some code to which
> I do not care still has some W=1.
If you are concerned about a particular driver - why don't you silence
the warning in there? Or alternatively build it with clang?
With all that, I think that the right way to go is to fix the root
cause of this churn - broken Wtype-limits in GCC, and after that move
Wtype-limits to W=1. Anything else looks hacky to me.
Thanks,
Yury
> Signed-off-by: Vincent Mailhol
> ---
> include/linux/find.h | 6 ++
> 1 file changed, 6 insertions(+)
>
> diff --git a/include/linux/find.h b/include/linux/find.h
> index 5bb6db213bcb..efd4b3f7dd17 100644
> --- a/include/linux/find.h
> +++ b/include/linux/find.h
> @@ -103,6 +103,10 @@ unsigned long find_next_zero_bit(const unsigned long
> *addr, unsigned long size,
> }
> #endif
>
> +__diag_push();
> +__diag_ignore(GCC, 8, "-Wtype-limits",
> + "GENMASK(size - 1, 0) yields 'comparison of unsigned expression
> in < 0 is always false' which is OK");
> +
> #ifndef find_first_bit
> /**
> * find_first_bit - find the first set bit in a memory region
> @@ -193,6 +197,8 @@ unsigned long find_last_bit(const unsigned long *addr,
> unsigned long size)
> }
> #endif
>
> +__diag_pop(); /* ignore -Wtype-limits */
> +
> /**
> * find_next_clump8 - find next 8-bit clump with set bits in a memory region
> * @clump: location to store copy of found clump
> --
> 2.35.1
Re: [PATCH] linux/find: ignore -Wtype-limits to reduce W=2 warnings by 34% tree-wide
On Wed, Apr 27, 2022 at 11:58:58AM +0900, Vincent MAILHOL wrote:
> + Alexander Lobakin
>
> On Wed. 27 Apr 2022 at 05:42, Yury Norov wrote:
> > + gcc@gcc.gnu.org
> > + Rikard Falkeborn
> >
> > On Wed, Apr 27, 2022 at 01:16:58AM +0900, Vincent Mailhol wrote:
> > > find_first_bit(), find_first_and_bit(), find_first_and_bit() and
> > > find_first_and_bit() all invokes GENMASK(size - 1, 0).
> > >
> > > This triggers below warning when compiled with W=2.
> > >
> > > | ./include/linux/find.h: In function 'find_first_bit':
> > > | ./include/linux/bits.h:25:36: warning: comparison of unsigned
> > > | expression in '< 0' is always false [-Wtype-limits]
> > > |25 | __is_constexpr((l) > (h)), (l) > (h), 0)))
> > > | |^
> > > | ./include/linux/build_bug.h:16:62: note: in definition of macro
> > > | 'BUILD_BUG_ON_ZERO'
> > > |16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct {
> > > int:(-!!(e)); })))
> > > | | ^
> > > | ./include/linux/bits.h:25:17: note: in expansion of macro
> > > '__is_constexpr'
> > > |25 | __is_constexpr((l) > (h)), (l) > (h), 0)))
> > > | | ^~
> > > | ./include/linux/bits.h:38:10: note: in expansion of macro
> > > 'GENMASK_INPUT_CHECK'
> > > |38 | (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> > > | | ^~~
> > > | ./include/linux/find.h:119:45: note: in expansion of macro 'GENMASK'
> > > | 119 | unsigned long val = *addr & GENMASK(size - 1,
> > > 0);
> > > | | ^~~
> > >
> > > linux/find.h being a widely used header file, above warning show up in
> > > thousand of files which include this header (either directly on
> > > indirectly).
> > >
> > > Because it is a false positive, we just silence -Wtype-limits flag
> > > locally to remove the spam. clang does not warn about it, so we just
> > > apply the diag_ignore() directive to gcc.
> > >
> > > By doing so, the warnings for a W=2 build are reduced by
> > > 34%. Benchmark was done with gcc 11.2.1 on Linux v5.17 x86_64
> > > allyesconfig (except CONFIG_WERROR). Beforethe patch: 515496 warnings
> > > and after: 340097.
> > >
> > > For reference, past proposal to modify GENMASK_INPUT_CHECK() was
> > > rejected in:
> > > https://lore.kernel.org/all/20220304124416.1181029-1-mailhol.vinc...@wanadoo.fr/
> >
> > So, here is nothing wrong with the kernel code and we have an alternative
> > compiler (clang) that doesn't throw Wtype-limits. It sounds to me like an
> > internal GCC problem, and I don't understand how hiding broken Wtype-limits
> > on kernel side would help people to improve GCC.
> >
> > On the thread you mentioned above:
> >
> > > > > > Have you fixed W=1 warnings?
> > > > > > Without fixing W=1 (which makes much more sense, when used with
> > > > > > WERROR=y && COMPILE_TEST=y) this has no value.
> > > > >
> > > > > How is this connected?
> > > >
> > > > By priorities.
> > > > I don't see much value in fixing W=2 per se if the code doesn't compile
> > > > for W=1.
> > >
> > > *My code* compiles for W=1. For me, fixing this W=2 in the next in line
> > > if speaking of priorities.
> > >
> > > I do not understand why I should be forbidden to fix a W=2 in the
> > > file which I am maintaining on the grounds that some code to which
> > > I do not care still has some W=1.
> >
> > If you are concerned about a particular driver - why don't you silence
> > the warning in there? Or alternatively build it with clang?
>
> Sorry if my previous comments looked selfish. I used the first
> person to illustrate my point but because this W=2 appears in
> thousands of files my real intent is to fix it for everybody, not
> only for myself.
Globally, we have not yet fixed W=1, that's why W=2 isn't that important.
(If the above statement is wrong - can someone explain me the logic of
splitting warnings by levels?)
Locally you cleared W=1, which is great, and I understand that you'd
like to have clean W=2 too.
> > With all that, I think that the right way to go is to fix the root
> > cause of this churn - broken Wtype-limits in GCC, and after that move
> > Wtype-limits to W=1. Anything else looks hacky to me.
>
> Why is this use of __diag_ignore() hacky compared when compared
> to the other use of __diag_ignore() or the use of -Wno-something
> in local Makefiles?
__diag_ignore() is not hacky when it's well-justified. Globally
there's a single user of __diag_ignore() - SYSCALL_DEFINE, and
it looks well-justified to me:
The new warning seems reasonable in principle, but it doesn't
help us here, since we rely on the type mismatch to sanitize the
system call arguments. After I reported this as GCC PR82435, a new
-Wno-attribute-alias option was added that could be used to turn the
warning off globally on the command line, but I'd pre
