Commit eaae7841ba83bb42 ("tools: sync lib/find_bit implementation") broke
build of 5.13-rc1 using gcc (GCC) 8.3.1 20190311 (Red Hat 8.3.1-3).
DESCEND objtool
CC /usr/src/linux/tools/objtool/exec-cmd.o
CC /usr/src/linux/tools/objtool/help.o
CC /usr/src/linux/tools/objtool/pager.o
CC /usr/src/linux/tools/objtool/parse-options.o
CC /usr/src/linux/tools/objtool/run-command.o
CC /usr/src/linux/tools/objtool/sigchain.o
CC /usr/src/linux/tools/objtool/subcmd-config.o
LD /usr/src/linux/tools/objtool/libsubcmd-in.o
AR /usr/src/linux/tools/objtool/libsubcmd.a
CC /usr/src/linux/tools/objtool/arch/x86/special.o
In file included from /usr/src/linux/tools/include/linux/kernel.h:8:0,
from /usr/src/linux/tools/include/linux/list.h:7,
from /usr/src/linux/tools/objtool/include/objtool/arch.h:10,
from /usr/src/linux/tools/objtool/include/objtool/check.h:11,
from /usr/src/linux/tools/objtool/include/objtool/special.h:10,
from arch/x86/special.c:4:
/usr/src/linux/tools/include/asm-generic/bitops/find.h: In function 'find_next_bit':
/usr/src/linux/tools/include/linux/bits.h:24:21: error: first argument to '__builtin_choose_expr' not a constant
(BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
^
/usr/src/linux/tools/include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
#define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
^
/usr/src/linux/tools/include/linux/bits.h:38:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
(GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
^
/usr/src/linux/tools/include/asm-generic/bitops/find.h:32:17: note: in expansion of macro 'GENMASK'
val = *addr & GENMASK(size - 1, offset);
^
/usr/src/linux/tools/include/linux/build_bug.h:16:51: error: bit-field '<anonymous>' width not an integer constant
#define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
^
/usr/src/linux/tools/include/linux/bits.h:24:3: note: in expansion of macro 'BUILD_BUG_ON_ZERO'
(BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
^
/usr/src/linux/tools/include/linux/bits.h:38:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
(GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
^
/usr/src/linux/tools/include/asm-generic/bitops/find.h:32:17: note: in expansion of macro 'GENMASK'
val = *addr & GENMASK(size - 1, offset);
^
/usr/src/linux/tools/include/asm-generic/bitops/find.h: In function 'find_next_and_bit':
/usr/src/linux/tools/include/linux/bits.h:24:21: error: first argument to '__builtin_choose_expr' not a constant
(BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
^
/usr/src/linux/tools/include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
#define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
^
/usr/src/linux/tools/include/linux/bits.h:38:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
(GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
^
/usr/src/linux/tools/include/asm-generic/bitops/find.h:62:27: note: in expansion of macro 'GENMASK'
val = *addr1 & *addr2 & GENMASK(size - 1, offset);
^
/usr/src/linux/tools/include/linux/build_bug.h:16:51: error: bit-field '<anonymous>' width not an integer constant
#define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
^
/usr/src/linux/tools/include/linux/bits.h:24:3: note: in expansion of macro 'BUILD_BUG_ON_ZERO'
(BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
^
/usr/src/linux/tools/include/linux/bits.h:38:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
(GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
^
/usr/src/linux/tools/include/asm-generic/bitops/find.h:62:27: note: in expansion of macro 'GENMASK'
val = *addr1 & *addr2 & GENMASK(size - 1, offset);
^
/usr/src/linux/tools/include/asm-generic/bitops/find.h: In function 'find_next_zero_bit':
/usr/src/linux/tools/include/linux/bits.h:24:21: error: first argument to '__builtin_choose_expr' not a constant
(BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
^
/usr/src/linux/tools/include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
#define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
^
/usr/src/linux/tools/include/linux/bits.h:38:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
(GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
^
/usr/src/linux/tools/include/asm-generic/bitops/find.h:90:18: note: in expansion of macro 'GENMASK'
val = *addr | ~GENMASK(size - 1, offset);
^
/usr/src/linux/tools/include/linux/build_bug.h:16:51: error: bit-field '<anonymous>' width not an integer constant
#define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
^
/usr/src/linux/tools/include/linux/bits.h:24:3: note: in expansion of macro 'BUILD_BUG_ON_ZERO'
(BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
^
/usr/src/linux/tools/include/linux/bits.h:38:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
(GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
^
/usr/src/linux/tools/include/asm-generic/bitops/find.h:90:18: note: in expansion of macro 'GENMASK'
val = *addr | ~GENMASK(size - 1, offset);
^
make[5]: *** [/usr/src/linux/tools/objtool/arch/x86/special.o] Error 1
make[4]: *** [arch/x86] Error 2
make[3]: *** [/usr/src/linux/tools/objtool/objtool-in.o] Error 2
make[2]: *** [objtool] Error 2
make[1]: *** [tools/objtool] Error 2
make: *** [__sub-make] Error 2
Applying below diff seems to solve the build failure.
Do we need to use BUILD_BUG_ON_ZERO() here?
diff --git a/tools/include/linux/bits.h b/tools/include/linux/bits.h
index 7f475d59a097..0aba9294f29d 100644
--- a/tools/include/linux/bits.h
+++ b/tools/include/linux/bits.h
@@ -21,8 +21,7 @@
#if !defined(__ASSEMBLY__)
#include <linux/build_bug.h>
#define GENMASK_INPUT_CHECK(h, l) \
- (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
- __builtin_constant_p((l) > (h)), (l) > (h), 0)))
+ ({ BUILD_BUG_ON(__builtin_constant_p((l) > (h)) && ((l) > (h))); 0; })
#else
/*
* BUILD_BUG_ON_ZERO is not available in h files included from asm files,
Also, why the fast path of find_*_bit() functions does not check
__builtin_constant_p(offset) as well as small_const_nbits(size), for the fast
path fails to catch BUILD_BUG_ON_ZERO() when offset argument is not a constant.
+Cc: Rikard
On Mon, May 10, 2021 at 6:31 PM Tetsuo Handa
<[email protected]> wrote:
>
> Commit eaae7841ba83bb42 ("tools: sync lib/find_bit implementation") broke
> build of 5.13-rc1 using gcc (GCC) 8.3.1 20190311 (Red Hat 8.3.1-3).
>
> DESCEND objtool
> CC /usr/src/linux/tools/objtool/exec-cmd.o
> CC /usr/src/linux/tools/objtool/help.o
> CC /usr/src/linux/tools/objtool/pager.o
> CC /usr/src/linux/tools/objtool/parse-options.o
> CC /usr/src/linux/tools/objtool/run-command.o
> CC /usr/src/linux/tools/objtool/sigchain.o
> CC /usr/src/linux/tools/objtool/subcmd-config.o
> LD /usr/src/linux/tools/objtool/libsubcmd-in.o
> AR /usr/src/linux/tools/objtool/libsubcmd.a
> CC /usr/src/linux/tools/objtool/arch/x86/special.o
> In file included from /usr/src/linux/tools/include/linux/kernel.h:8:0,
> from /usr/src/linux/tools/include/linux/list.h:7,
> from /usr/src/linux/tools/objtool/include/objtool/arch.h:10,
> from /usr/src/linux/tools/objtool/include/objtool/check.h:11,
> from /usr/src/linux/tools/objtool/include/objtool/special.h:10,
> from arch/x86/special.c:4:
> /usr/src/linux/tools/include/asm-generic/bitops/find.h: In function 'find_next_bit':
> /usr/src/linux/tools/include/linux/bits.h:24:21: error: first argument to '__builtin_choose_expr' not a constant
> (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
> ^
> /usr/src/linux/tools/include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
> #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
> ^
> /usr/src/linux/tools/include/linux/bits.h:38:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
> (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> ^
> /usr/src/linux/tools/include/asm-generic/bitops/find.h:32:17: note: in expansion of macro 'GENMASK'
> val = *addr & GENMASK(size - 1, offset);
> ^
> /usr/src/linux/tools/include/linux/build_bug.h:16:51: error: bit-field '<anonymous>' width not an integer constant
> #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
> ^
> /usr/src/linux/tools/include/linux/bits.h:24:3: note: in expansion of macro 'BUILD_BUG_ON_ZERO'
> (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
> ^
> /usr/src/linux/tools/include/linux/bits.h:38:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
> (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> ^
> /usr/src/linux/tools/include/asm-generic/bitops/find.h:32:17: note: in expansion of macro 'GENMASK'
> val = *addr & GENMASK(size - 1, offset);
> ^
> /usr/src/linux/tools/include/asm-generic/bitops/find.h: In function 'find_next_and_bit':
> /usr/src/linux/tools/include/linux/bits.h:24:21: error: first argument to '__builtin_choose_expr' not a constant
> (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
> ^
> /usr/src/linux/tools/include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
> #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
> ^
> /usr/src/linux/tools/include/linux/bits.h:38:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
> (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> ^
> /usr/src/linux/tools/include/asm-generic/bitops/find.h:62:27: note: in expansion of macro 'GENMASK'
> val = *addr1 & *addr2 & GENMASK(size - 1, offset);
> ^
> /usr/src/linux/tools/include/linux/build_bug.h:16:51: error: bit-field '<anonymous>' width not an integer constant
> #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
> ^
> /usr/src/linux/tools/include/linux/bits.h:24:3: note: in expansion of macro 'BUILD_BUG_ON_ZERO'
> (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
> ^
> /usr/src/linux/tools/include/linux/bits.h:38:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
> (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> ^
> /usr/src/linux/tools/include/asm-generic/bitops/find.h:62:27: note: in expansion of macro 'GENMASK'
> val = *addr1 & *addr2 & GENMASK(size - 1, offset);
> ^
> /usr/src/linux/tools/include/asm-generic/bitops/find.h: In function 'find_next_zero_bit':
> /usr/src/linux/tools/include/linux/bits.h:24:21: error: first argument to '__builtin_choose_expr' not a constant
> (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
> ^
> /usr/src/linux/tools/include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
> #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
> ^
> /usr/src/linux/tools/include/linux/bits.h:38:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
> (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> ^
> /usr/src/linux/tools/include/asm-generic/bitops/find.h:90:18: note: in expansion of macro 'GENMASK'
> val = *addr | ~GENMASK(size - 1, offset);
> ^
> /usr/src/linux/tools/include/linux/build_bug.h:16:51: error: bit-field '<anonymous>' width not an integer constant
> #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
> ^
> /usr/src/linux/tools/include/linux/bits.h:24:3: note: in expansion of macro 'BUILD_BUG_ON_ZERO'
> (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
> ^
> /usr/src/linux/tools/include/linux/bits.h:38:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
> (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> ^
> /usr/src/linux/tools/include/asm-generic/bitops/find.h:90:18: note: in expansion of macro 'GENMASK'
> val = *addr | ~GENMASK(size - 1, offset);
> ^
> make[5]: *** [/usr/src/linux/tools/objtool/arch/x86/special.o] Error 1
> make[4]: *** [arch/x86] Error 2
> make[3]: *** [/usr/src/linux/tools/objtool/objtool-in.o] Error 2
> make[2]: *** [objtool] Error 2
> make[1]: *** [tools/objtool] Error 2
> make: *** [__sub-make] Error 2
>
>
>
> Applying below diff seems to solve the build failure.
It will desynchronize this implementation with the mother's one (i.e.
in bits.h).
> Do we need to use BUILD_BUG_ON_ZERO() here?
Rikard?
>
> diff --git a/tools/include/linux/bits.h b/tools/include/linux/bits.h
> index 7f475d59a097..0aba9294f29d 100644
> --- a/tools/include/linux/bits.h
> +++ b/tools/include/linux/bits.h
> @@ -21,8 +21,7 @@
> #if !defined(__ASSEMBLY__)
> #include <linux/build_bug.h>
> #define GENMASK_INPUT_CHECK(h, l) \
> - (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
> - __builtin_constant_p((l) > (h)), (l) > (h), 0)))
> + ({ BUILD_BUG_ON(__builtin_constant_p((l) > (h)) && ((l) > (h))); 0; })
> #else
> /*
> * BUILD_BUG_ON_ZERO is not available in h files included from asm files,
> Also, why the fast path of find_*_bit() functions does not check
> __builtin_constant_p(offset) as well as small_const_nbits(size), for the fast
> path fails to catch BUILD_BUG_ON_ZERO() when offset argument is not a constant.
How would this help anything?
If you ask a bit from a bitmap behind the size, what do you expect to get?
And I'm a bit lost here, because I can't imagine the offset being
constant along with a size of bitmap. What do we want to achieve by
this? Any examples to better understand the case?
--
With Best Regards,
Andy Shevchenko
On Mon, May 10, 2021 at 06:44:44PM +0300, Andy Shevchenko wrote:
> +Cc: Rikard
>
> On Mon, May 10, 2021 at 6:31 PM Tetsuo Handa
> <[email protected]> wrote:
> >
> > Commit eaae7841ba83bb42 ("tools: sync lib/find_bit implementation") broke
> > build of 5.13-rc1 using gcc (GCC) 8.3.1 20190311 (Red Hat 8.3.1-3).
> >
> > DESCEND objtool
> > CC /usr/src/linux/tools/objtool/exec-cmd.o
> > CC /usr/src/linux/tools/objtool/help.o
> > CC /usr/src/linux/tools/objtool/pager.o
> > CC /usr/src/linux/tools/objtool/parse-options.o
> > CC /usr/src/linux/tools/objtool/run-command.o
> > CC /usr/src/linux/tools/objtool/sigchain.o
> > CC /usr/src/linux/tools/objtool/subcmd-config.o
> > LD /usr/src/linux/tools/objtool/libsubcmd-in.o
> > AR /usr/src/linux/tools/objtool/libsubcmd.a
> > CC /usr/src/linux/tools/objtool/arch/x86/special.o
> > In file included from /usr/src/linux/tools/include/linux/kernel.h:8:0,
> > from /usr/src/linux/tools/include/linux/list.h:7,
> > from /usr/src/linux/tools/objtool/include/objtool/arch.h:10,
> > from /usr/src/linux/tools/objtool/include/objtool/check.h:11,
> > from /usr/src/linux/tools/objtool/include/objtool/special.h:10,
> > from arch/x86/special.c:4:
> > /usr/src/linux/tools/include/asm-generic/bitops/find.h: In function 'find_next_bit':
> > /usr/src/linux/tools/include/linux/bits.h:24:21: error: first argument to '__builtin_choose_expr' not a constant
> > (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
> > ^
> > /usr/src/linux/tools/include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
> > #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
> > ^
> > /usr/src/linux/tools/include/linux/bits.h:38:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
> > (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> > ^
> > /usr/src/linux/tools/include/asm-generic/bitops/find.h:32:17: note: in expansion of macro 'GENMASK'
> > val = *addr & GENMASK(size - 1, offset);
> > ^
> > /usr/src/linux/tools/include/linux/build_bug.h:16:51: error: bit-field '<anonymous>' width not an integer constant
> > #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
> > ^
> > /usr/src/linux/tools/include/linux/bits.h:24:3: note: in expansion of macro 'BUILD_BUG_ON_ZERO'
> > (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
> > ^
> > /usr/src/linux/tools/include/linux/bits.h:38:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
> > (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> > ^
> > /usr/src/linux/tools/include/asm-generic/bitops/find.h:32:17: note: in expansion of macro 'GENMASK'
> > val = *addr & GENMASK(size - 1, offset);
> > ^
> > /usr/src/linux/tools/include/asm-generic/bitops/find.h: In function 'find_next_and_bit':
> > /usr/src/linux/tools/include/linux/bits.h:24:21: error: first argument to '__builtin_choose_expr' not a constant
> > (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
> > ^
> > /usr/src/linux/tools/include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
> > #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
> > ^
> > /usr/src/linux/tools/include/linux/bits.h:38:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
> > (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> > ^
> > /usr/src/linux/tools/include/asm-generic/bitops/find.h:62:27: note: in expansion of macro 'GENMASK'
> > val = *addr1 & *addr2 & GENMASK(size - 1, offset);
> > ^
> > /usr/src/linux/tools/include/linux/build_bug.h:16:51: error: bit-field '<anonymous>' width not an integer constant
> > #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
> > ^
> > /usr/src/linux/tools/include/linux/bits.h:24:3: note: in expansion of macro 'BUILD_BUG_ON_ZERO'
> > (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
> > ^
> > /usr/src/linux/tools/include/linux/bits.h:38:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
> > (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> > ^
> > /usr/src/linux/tools/include/asm-generic/bitops/find.h:62:27: note: in expansion of macro 'GENMASK'
> > val = *addr1 & *addr2 & GENMASK(size - 1, offset);
> > ^
> > /usr/src/linux/tools/include/asm-generic/bitops/find.h: In function 'find_next_zero_bit':
> > /usr/src/linux/tools/include/linux/bits.h:24:21: error: first argument to '__builtin_choose_expr' not a constant
> > (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
> > ^
> > /usr/src/linux/tools/include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
> > #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
> > ^
> > /usr/src/linux/tools/include/linux/bits.h:38:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
> > (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> > ^
> > /usr/src/linux/tools/include/asm-generic/bitops/find.h:90:18: note: in expansion of macro 'GENMASK'
> > val = *addr | ~GENMASK(size - 1, offset);
> > ^
> > /usr/src/linux/tools/include/linux/build_bug.h:16:51: error: bit-field '<anonymous>' width not an integer constant
> > #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
> > ^
> > /usr/src/linux/tools/include/linux/bits.h:24:3: note: in expansion of macro 'BUILD_BUG_ON_ZERO'
> > (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
> > ^
> > /usr/src/linux/tools/include/linux/bits.h:38:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
> > (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> > ^
> > /usr/src/linux/tools/include/asm-generic/bitops/find.h:90:18: note: in expansion of macro 'GENMASK'
> > val = *addr | ~GENMASK(size - 1, offset);
> > ^
> > make[5]: *** [/usr/src/linux/tools/objtool/arch/x86/special.o] Error 1
> > make[4]: *** [arch/x86] Error 2
> > make[3]: *** [/usr/src/linux/tools/objtool/objtool-in.o] Error 2
> > make[2]: *** [objtool] Error 2
> > make[1]: *** [tools/objtool] Error 2
> > make: *** [__sub-make] Error 2
> >
> >
> > Applying below diff seems to solve the build failure.
> >
> It will desynchronize this implementation with the mother's one (i.e.
> in bits.h).
>
> > Do we need to use BUILD_BUG_ON_ZERO() here?
>
> Rikard?
>
> >
> > diff --git a/tools/include/linux/bits.h b/tools/include/linux/bits.h
> > index 7f475d59a097..0aba9294f29d 100644
> > --- a/tools/include/linux/bits.h
> > +++ b/tools/include/linux/bits.h
> > @@ -21,8 +21,7 @@
> > #if !defined(__ASSEMBLY__)
> > #include <linux/build_bug.h>
> > #define GENMASK_INPUT_CHECK(h, l) \
> > - (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
> > - __builtin_constant_p((l) > (h)), (l) > (h), 0)))
> > + ({ BUILD_BUG_ON(__builtin_constant_p((l) > (h)) && ((l) > (h))); 0; })
> > #else
> > /*
> > * BUILD_BUG_ON_ZERO is not available in h files included from asm files,
As Andy said, we need to sync tools/GENMASK_INPUT_CHECK() with the
kernel version, and if I do this, I have many build failures:
https://pastebin.com/Fan1VLVF
Maybe in this case we should use __GENMASK() to bypass GENMASK_INPUT_CHECK()...
I'll check everything carefully this evening.
> > Also, why the fast path of find_*_bit() functions does not check
> > __builtin_constant_p(offset) as well as small_const_nbits(size), for the fast
> > path fails to catch BUILD_BUG_ON_ZERO() when offset argument is not a constant.
>
> How would this help anything?
>
> If you ask a bit from a bitmap behind the size, what do you expect to get?
>
> And I'm a bit lost here, because I can't imagine the offset being
> constant along with a size of bitmap. What do we want to achieve by
> this? Any examples to better understand the case?
If offset is constant, the existing fast path optimization would work
even better, without any modifications. (Not sure there's an example of
it in the existing codebase.) But even if the offset is not constant,
fast path works quite well - it saves ~1K of .text and improves on
performance:
https://lore.kernel.org/linux-m68k/[email protected]/
We don't need to disable the optimization for non-constant offsets,
for sure. If asserts in GENMASK() break build, we should use __GENMASK().
Thanks,
Yury
On Mon, May 10, 2021 at 06:44:44PM +0300, Andy Shevchenko wrote:
> +Cc: Rikard
>
> On Mon, May 10, 2021 at 6:31 PM Tetsuo Handa
> <[email protected]> wrote:
> >
> > Commit eaae7841ba83bb42 ("tools: sync lib/find_bit implementation") broke
> > build of 5.13-rc1 using gcc (GCC) 8.3.1 20190311 (Red Hat 8.3.1-3).
> >
> > DESCEND objtool
> > CC /usr/src/linux/tools/objtool/exec-cmd.o
> > CC /usr/src/linux/tools/objtool/help.o
> > CC /usr/src/linux/tools/objtool/pager.o
> > CC /usr/src/linux/tools/objtool/parse-options.o
> > CC /usr/src/linux/tools/objtool/run-command.o
> > CC /usr/src/linux/tools/objtool/sigchain.o
> > CC /usr/src/linux/tools/objtool/subcmd-config.o
> > LD /usr/src/linux/tools/objtool/libsubcmd-in.o
> > AR /usr/src/linux/tools/objtool/libsubcmd.a
> > CC /usr/src/linux/tools/objtool/arch/x86/special.o
> > In file included from /usr/src/linux/tools/include/linux/kernel.h:8:0,
> > from /usr/src/linux/tools/include/linux/list.h:7,
> > from /usr/src/linux/tools/objtool/include/objtool/arch.h:10,
> > from /usr/src/linux/tools/objtool/include/objtool/check.h:11,
> > from /usr/src/linux/tools/objtool/include/objtool/special.h:10,
> > from arch/x86/special.c:4:
> > /usr/src/linux/tools/include/asm-generic/bitops/find.h: In function 'find_next_bit':
> > /usr/src/linux/tools/include/linux/bits.h:24:21: error: first argument to '__builtin_choose_expr' not a constant
> > (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
> > ^
> > /usr/src/linux/tools/include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
> > #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
> > ^
> > /usr/src/linux/tools/include/linux/bits.h:38:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
> > (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> > ^
> > /usr/src/linux/tools/include/asm-generic/bitops/find.h:32:17: note: in expansion of macro 'GENMASK'
> > val = *addr & GENMASK(size - 1, offset);
> > ^
> > /usr/src/linux/tools/include/linux/build_bug.h:16:51: error: bit-field '<anonymous>' width not an integer constant
> > #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
> > ^
> > /usr/src/linux/tools/include/linux/bits.h:24:3: note: in expansion of macro 'BUILD_BUG_ON_ZERO'
> > (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
> > ^
> > /usr/src/linux/tools/include/linux/bits.h:38:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
> > (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> > ^
> > /usr/src/linux/tools/include/asm-generic/bitops/find.h:32:17: note: in expansion of macro 'GENMASK'
> > val = *addr & GENMASK(size - 1, offset);
> > ^
> > /usr/src/linux/tools/include/asm-generic/bitops/find.h: In function 'find_next_and_bit':
> > /usr/src/linux/tools/include/linux/bits.h:24:21: error: first argument to '__builtin_choose_expr' not a constant
> > (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
> > ^
> > /usr/src/linux/tools/include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
> > #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
> > ^
> > /usr/src/linux/tools/include/linux/bits.h:38:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
> > (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> > ^
> > /usr/src/linux/tools/include/asm-generic/bitops/find.h:62:27: note: in expansion of macro 'GENMASK'
> > val = *addr1 & *addr2 & GENMASK(size - 1, offset);
> > ^
> > /usr/src/linux/tools/include/linux/build_bug.h:16:51: error: bit-field '<anonymous>' width not an integer constant
> > #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
> > ^
> > /usr/src/linux/tools/include/linux/bits.h:24:3: note: in expansion of macro 'BUILD_BUG_ON_ZERO'
> > (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
> > ^
> > /usr/src/linux/tools/include/linux/bits.h:38:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
> > (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> > ^
> > /usr/src/linux/tools/include/asm-generic/bitops/find.h:62:27: note: in expansion of macro 'GENMASK'
> > val = *addr1 & *addr2 & GENMASK(size - 1, offset);
> > ^
> > /usr/src/linux/tools/include/asm-generic/bitops/find.h: In function 'find_next_zero_bit':
> > /usr/src/linux/tools/include/linux/bits.h:24:21: error: first argument to '__builtin_choose_expr' not a constant
> > (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
> > ^
> > /usr/src/linux/tools/include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
> > #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
> > ^
> > /usr/src/linux/tools/include/linux/bits.h:38:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
> > (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> > ^
> > /usr/src/linux/tools/include/asm-generic/bitops/find.h:90:18: note: in expansion of macro 'GENMASK'
> > val = *addr | ~GENMASK(size - 1, offset);
> > ^
> > /usr/src/linux/tools/include/linux/build_bug.h:16:51: error: bit-field '<anonymous>' width not an integer constant
> > #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
> > ^
> > /usr/src/linux/tools/include/linux/bits.h:24:3: note: in expansion of macro 'BUILD_BUG_ON_ZERO'
> > (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
> > ^
> > /usr/src/linux/tools/include/linux/bits.h:38:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
> > (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> > ^
> > /usr/src/linux/tools/include/asm-generic/bitops/find.h:90:18: note: in expansion of macro 'GENMASK'
> > val = *addr | ~GENMASK(size - 1, offset);
> > ^
> > make[5]: *** [/usr/src/linux/tools/objtool/arch/x86/special.o] Error 1
> > make[4]: *** [arch/x86] Error 2
> > make[3]: *** [/usr/src/linux/tools/objtool/objtool-in.o] Error 2
> > make[2]: *** [objtool] Error 2
> > make[1]: *** [tools/objtool] Error 2
> > make: *** [__sub-make] Error 2
> >
> >
Some background: when I added the input check to GENMASK there was
originally a check that disabled the input check if gcc versions earlier
than 4.9, since for old compilers, there was a bug [1] where for some
constructs, __builtin_constant_p() did not evaluate to a constant. This
was supposedly fixed in gcc 4.9, but apparently there are newer gcc
versions which suffer from this too.
[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=19449
> >
> > Applying below diff seems to solve the build failure.
>
> It will desynchronize this implementation with the mother's one (i.e.
> in bits.h).
>
> > Do we need to use BUILD_BUG_ON_ZERO() here?
>
> Rikard?
Yes, (as Yury pointed out), GENMASK is used in for example structure
initializers where BUIlD_BUG() can not be used.
>
> >
> > diff --git a/tools/include/linux/bits.h b/tools/include/linux/bits.h
> > index 7f475d59a097..0aba9294f29d 100644
> > --- a/tools/include/linux/bits.h
> > +++ b/tools/include/linux/bits.h
> > @@ -21,8 +21,7 @@
> > #if !defined(__ASSEMBLY__)
> > #include <linux/build_bug.h>
> > #define GENMASK_INPUT_CHECK(h, l) \
> > - (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
> > - __builtin_constant_p((l) > (h)), (l) > (h), 0)))
> > + ({ BUILD_BUG_ON(__builtin_constant_p((l) > (h)) && ((l) > (h))); 0; })
> > #else
> > /*
> > * BUILD_BUG_ON_ZERO is not available in h files included from asm files,
Does the following work for you? For simplicity, I copied__is_constexpr from
include/linux/minmax.h (which isn't available in tools/). A proper patch
would reuse __is_constexpr (possibly refactoring it to a separate
header since bits.h including minmax.h for that only seems smelly) and fix
bits.h in the kernel header as well, to keep the files in sync.
diff --git a/tools/include/linux/bits.h b/tools/include/linux/bits.h
index 7f475d59a097..7bc4c31a7df0 100644
--- a/tools/include/linux/bits.h
+++ b/tools/include/linux/bits.h
@@ -19,10 +19,13 @@
* GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
*/
#if !defined(__ASSEMBLY__)
+
+#define __is_constexpr(x) \
+ (sizeof(int) == sizeof(*(8 ? ((void *)((long)(x) * 0l)) : (int *)8)))
#include <linux/build_bug.h>
#define GENMASK_INPUT_CHECK(h, l) \
(BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
- __builtin_constant_p((l) > (h)), (l) > (h), 0)))
+ __is_constexpr((l) > (h)), (l) > (h), 0)))
#else
/*
* BUILD_BUG_ON_ZERO is not available in h files included from asm files,
I think any solution which results in using __GENMASK to avoid the input
check will just result in similar reports in the future.
Rikard
On Tue, May 11, 2021 at 12:51:58AM +0200, Rikard Falkeborn wrote:
> On Mon, May 10, 2021 at 06:44:44PM +0300, Andy Shevchenko wrote:
...
> Does the following work for you? For simplicity, I copied__is_constexpr from
> include/linux/minmax.h (which isn't available in tools/). A proper patch
> would reuse __is_constexpr (possibly refactoring it to a separate
> header since bits.h including minmax.h for that only seems smelly)
I think we need to have it in something like compiler.h (top level). Under
'top level' I meant something with the function as of compiler.h but with
Linuxisms rather than compiler attributes or so.
Separate header for the (single) macro is too much...
> and fix
> bits.h in the kernel header as well, to keep the files in sync.
Right.
--
With Best Regards,
Andy Shevchenko
On Tue, May 11, 2021 at 10:28:50AM +0300, Andy Shevchenko wrote:
> On Tue, May 11, 2021 at 12:51:58AM +0200, Rikard Falkeborn wrote:
> > On Mon, May 10, 2021 at 06:44:44PM +0300, Andy Shevchenko wrote:
>
> ...
>
> > Does the following work for you? For simplicity, I copied__is_constexpr from
> > include/linux/minmax.h (which isn't available in tools/). A proper patch
> > would reuse __is_constexpr (possibly refactoring it to a separate
> > header since bits.h including minmax.h for that only seems smelly)
>
> I think we need to have it in something like compiler.h (top level). Under
> 'top level' I meant something with the function as of compiler.h but with
> Linuxisms rather than compiler attributes or so.
Right. Will you send a patch, or do you want me to?
It would be good to get confirmation that __is_constexpr solves the
build failure.
>
> Separate header for the (single) macro is too much...
>
Agreed.
> > and fix
> > bits.h in the kernel header as well, to keep the files in sync.
>
> Right.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
With Best Regards,
Rikard
On 2021/05/11 0:44, Andy Shevchenko wrote:
> And I'm a bit lost here, because I can't imagine the offset being
> constant along with a size of bitmap. What do we want to achieve by
> this? Any examples to better understand the case?
Because I feel that the GENMASK() macro cannot be evaluated without
both arguments being a constant.
The usage is
unsigned long find_next_bit(const unsigned long *addr, unsigned long size,
unsigned long offset)
{
+ if (small_const_nbits(size)) {
+ unsigned long val;
+
+ if (unlikely(offset >= size))
+ return size;
+
+ val = *addr & GENMASK(size - 1, offset);
+ return val ? __ffs(val) : size;
+ }
+
return _find_next_bit(addr, NULL, size, offset, 0UL, 0);
}
where GENMASK() might be called even if "offset" is not a constant.
#define GENMASK(h, l) \
(GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
#define __GENMASK(h, l) \
(((~UL(0)) - (UL(1) << (l)) + 1) & \
(~UL(0) >> (BITS_PER_LONG - 1 - (h))))
#define GENMASK_INPUT_CHECK(h, l) \
(BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
__builtin_constant_p((l) > (h)), (l) > (h), 0)))
__GENMASK() does not need "h" and "l" being a constant.
Yes, small_const_nbits(size) in find_next_bit() can guarantee that "size" is a
constant and hence "h" argument in GENMASK_INPUT_CHECK() call is also a constant.
But nothing can guarantee that "offset" is a constant, and hence nothing can
guarantee that "l" argument in GENMASK_INPUT_CHECK() call is also a constant.
Then, how can (l) > (h) in __builtin_constant_p((l) > (h)) be evaluated at build time
if either l or h (i.e. "offset" and "size - 1" in find_next_bit()) lacks a guarantee of
being a constant?
But what a surprise,
On 2021/05/11 7:51, Rikard Falkeborn wrote:
> Does the following work for you? For simplicity, I copied__is_constexpr from
> include/linux/minmax.h (which isn't available in tools/). A proper patch
> would reuse __is_constexpr (possibly refactoring it to a separate
> header since bits.h including minmax.h for that only seems smelly) and fix
> bits.h in the kernel header as well, to keep the files in sync.
this works for me.
>
> diff --git a/tools/include/linux/bits.h b/tools/include/linux/bits.h
> index 7f475d59a097..7bc4c31a7df0 100644
> --- a/tools/include/linux/bits.h
> +++ b/tools/include/linux/bits.h
> @@ -19,10 +19,13 @@
> * GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
> */
> #if !defined(__ASSEMBLY__)
> +
> +#define __is_constexpr(x) \
> + (sizeof(int) == sizeof(*(8 ? ((void *)((long)(x) * 0l)) : (int *)8)))
> #include <linux/build_bug.h>
> #define GENMASK_INPUT_CHECK(h, l) \
> (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
> - __builtin_constant_p((l) > (h)), (l) > (h), 0)))
> + __is_constexpr((l) > (h)), (l) > (h), 0)))
> #else
> /*
> * BUILD_BUG_ON_ZERO is not available in h files included from asm files,
>
On 2021/05/11 7:52, Yury Norov wrote:
> I tested the objtool build with the 8.4.0 and 7.5.0 compilers from
> ubuntu 21 distro, and it looks working. Can you please share more
> details about your system?
Nothing special. A plain x86_64 CentOS 7.9 system with devtoolset-8.
$ /opt/rh/devtoolset-8/root/bin/gcc --version
gcc (GCC) 8.3.1 20190311 (Red Hat 8.3.1-3)
Copyright (C) 2018 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
$ rpm -qi devtoolset-8-gcc
Name : devtoolset-8-gcc
Version : 8.3.1
Release : 3.2.el7
Architecture: x86_64
Install Date: Wed Apr 22 07:58:16 2020
Group : Development/Languages
Size : 74838011
License : GPLv3+ and GPLv3+ with exceptions and GPLv2+ with exceptions and LGPLv2+ and BSD
Signature : RSA/SHA1, Thu Apr 16 19:44:43 2020, Key ID 4eb84e71f2ee9d55
Source RPM : devtoolset-8-gcc-8.3.1-3.2.el7.src.rpm
Build Date : Sat Mar 28 00:06:45 2020
Build Host : c1be.rdu2.centos.org
Relocations : (not relocatable)
Packager : CBS <[email protected]>
Vendor : CentOS
URL : http://gcc.gnu.org
Summary : GCC version 8
Description :
The devtoolset-8-gcc package contains the GNU Compiler Collection version 7.
On Tue, May 11, 2021 at 1:36 PM Rikard Falkeborn
<[email protected]> wrote:
>
> On Tue, May 11, 2021 at 10:28:50AM +0300, Andy Shevchenko wrote:
> > On Tue, May 11, 2021 at 12:51:58AM +0200, Rikard Falkeborn wrote:
> > > On Mon, May 10, 2021 at 06:44:44PM +0300, Andy Shevchenko wrote:
> >
> > ...
> >
> > > Does the following work for you? For simplicity, I copied__is_constexpr from
> > > include/linux/minmax.h (which isn't available in tools/). A proper patch
> > > would reuse __is_constexpr (possibly refactoring it to a separate
> > > header since bits.h including minmax.h for that only seems smelly)
> >
> > I think we need to have it in something like compiler.h (top level). Under
> > 'top level' I meant something with the function as of compiler.h but with
> > Linuxisms rather than compiler attributes or so.
>
> Right. Will you send a patch, or do you want me to?
Please, go ahead!
I'm in a vacation mood (tomorrow it will start)
> It would be good to get confirmation that __is_constexpr solves the
> build failure.
--
With Best Regards,
Andy Shevchenko
On Tue, May 11, 2021 at 08:53:53PM +0900, Tetsuo Handa wrote:
> On 2021/05/11 0:44, Andy Shevchenko wrote:
> > And I'm a bit lost here, because I can't imagine the offset being
> > constant along with a size of bitmap. What do we want to achieve by
> > this? Any examples to better understand the case?
>
> Because I feel that the GENMASK() macro cannot be evaluated without
> both arguments being a constant.
>
> The usage is
>
> unsigned long find_next_bit(const unsigned long *addr, unsigned long size,
> unsigned long offset)
> {
> + if (small_const_nbits(size)) {
> + unsigned long val;
> +
> + if (unlikely(offset >= size))
> + return size;
> +
> + val = *addr & GENMASK(size - 1, offset);
> + return val ? __ffs(val) : size;
> + }
> +
> return _find_next_bit(addr, NULL, size, offset, 0UL, 0);
> }
>
> where GENMASK() might be called even if "offset" is not a constant.
>
> #define GENMASK(h, l) \
> (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
>
> #define __GENMASK(h, l) \
> (((~UL(0)) - (UL(1) << (l)) + 1) & \
> (~UL(0) >> (BITS_PER_LONG - 1 - (h))))
>
> #define GENMASK_INPUT_CHECK(h, l) \
> (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
> __builtin_constant_p((l) > (h)), (l) > (h), 0)))
>
> __GENMASK() does not need "h" and "l" being a constant.
>
> Yes, small_const_nbits(size) in find_next_bit() can guarantee that "size" is a
> constant and hence "h" argument in GENMASK_INPUT_CHECK() call is also a constant.
> But nothing can guarantee that "offset" is a constant, and hence nothing can
> guarantee that "l" argument in GENMASK_INPUT_CHECK() call is also a constant.
>
> Then, how can (l) > (h) in __builtin_constant_p((l) > (h)) be evaluated at build time
> if either l or h (i.e. "offset" and "size - 1" in find_next_bit()) lacks a guarantee of
> being a constant?
>
So the idea is that if (l > h) is constant, __builtin_constant_p should
evaluate that, and if it is not it should use zero instead as input to
__builtin_chose_expr(). This works with non-const inputs in many other
places in the kernel, but apparently in this case with a certain
compiler, it doesn't so I guess we need to work around it.
> But what a surprise,
>
> On 2021/05/11 7:51, Rikard Falkeborn wrote:
> > Does the following work for you? For simplicity, I copied__is_constexpr from
> > include/linux/minmax.h (which isn't available in tools/). A proper patch
> > would reuse __is_constexpr (possibly refactoring it to a separate
> > header since bits.h including minmax.h for that only seems smelly) and fix
> > bits.h in the kernel header as well, to keep the files in sync.
>
> this works for me.
>
Great, thanks for testing!
I sent a patch for this here:
https://lore.kernel.org/lkml/[email protected]/
Rikard
> >
> > diff --git a/tools/include/linux/bits.h b/tools/include/linux/bits.h
> > index 7f475d59a097..7bc4c31a7df0 100644
> > --- a/tools/include/linux/bits.h
> > +++ b/tools/include/linux/bits.h
> > @@ -19,10 +19,13 @@
> > * GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
> > */
> > #if !defined(__ASSEMBLY__)
> > +
> > +#define __is_constexpr(x) \
> > + (sizeof(int) == sizeof(*(8 ? ((void *)((long)(x) * 0l)) : (int *)8)))
> > #include <linux/build_bug.h>
> > #define GENMASK_INPUT_CHECK(h, l) \
> > (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
> > - __builtin_constant_p((l) > (h)), (l) > (h), 0)))
> > + __is_constexpr((l) > (h)), (l) > (h), 0)))
> > #else
> > /*
> > * BUILD_BUG_ON_ZERO is not available in h files included from asm files,
> >
>
>
>
> On 2021/05/11 7:52, Yury Norov wrote:
> > I tested the objtool build with the 8.4.0 and 7.5.0 compilers from
> > ubuntu 21 distro, and it looks working. Can you please share more
> > details about your system?
>
> Nothing special. A plain x86_64 CentOS 7.9 system with devtoolset-8.
>
> $ /opt/rh/devtoolset-8/root/bin/gcc --version
> gcc (GCC) 8.3.1 20190311 (Red Hat 8.3.1-3)
> Copyright (C) 2018 Free Software Foundation, Inc.
> This is free software; see the source for copying conditions. There is NO
> warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
>
> $ rpm -qi devtoolset-8-gcc
> Name : devtoolset-8-gcc
> Version : 8.3.1
> Release : 3.2.el7
> Architecture: x86_64
> Install Date: Wed Apr 22 07:58:16 2020
> Group : Development/Languages
> Size : 74838011
> License : GPLv3+ and GPLv3+ with exceptions and GPLv2+ with exceptions and LGPLv2+ and BSD
> Signature : RSA/SHA1, Thu Apr 16 19:44:43 2020, Key ID 4eb84e71f2ee9d55
> Source RPM : devtoolset-8-gcc-8.3.1-3.2.el7.src.rpm
> Build Date : Sat Mar 28 00:06:45 2020
> Build Host : c1be.rdu2.centos.org
> Relocations : (not relocatable)
> Packager : CBS <[email protected]>
> Vendor : CentOS
> URL : http://gcc.gnu.org
> Summary : GCC version 8
> Description :
> The devtoolset-8-gcc package contains the GNU Compiler Collection version 7.
>
On Tue, May 11, 2021 at 10:39 PM Rikard Falkeborn
<[email protected]> wrote:
> On Tue, May 11, 2021 at 08:53:53PM +0900, Tetsuo Handa wrote:
> > #define GENMASK_INPUT_CHECK(h, l) \
> > (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
> > __builtin_constant_p((l) > (h)), (l) > (h), 0)))
> >
> > __GENMASK() does not need "h" and "l" being a constant.
> >
> > Yes, small_const_nbits(size) in find_next_bit() can guarantee that "size" is a
> > constant and hence "h" argument in GENMASK_INPUT_CHECK() call is also a constant.
> > But nothing can guarantee that "offset" is a constant, and hence nothing can
> > guarantee that "l" argument in GENMASK_INPUT_CHECK() call is also a constant.
> >
> > Then, how can (l) > (h) in __builtin_constant_p((l) > (h)) be evaluated at build time
> > if either l or h (i.e. "offset" and "size - 1" in find_next_bit()) lacks a guarantee of
> > being a constant?
> >
>
> So the idea is that if (l > h) is constant, __builtin_constant_p should
> evaluate that, and if it is not it should use zero instead as input to
> __builtin_chose_expr(). This works with non-const inputs in many other
> places in the kernel, but apparently in this case with a certain
> compiler, it doesn't so I guess we need to work around it.
I have a vague memory that __builtin_constant_p() inside of
__builtin_choose_expr()
always evaluates to false because of the order in which the compiler processes
those: If constant-folding only happens after __builtin_choose_expr(), then
__builtin_constant_p() has to be false.
Arnd
On 12/05/2021 09.48, Arnd Bergmann wrote:
> On Tue, May 11, 2021 at 10:39 PM Rikard Falkeborn
> <[email protected]> wrote:
>> On Tue, May 11, 2021 at 08:53:53PM +0900, Tetsuo Handa wrote:
>
>>> #define GENMASK_INPUT_CHECK(h, l) \
>>> (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
>>> __builtin_constant_p((l) > (h)), (l) > (h), 0)))
>>>
>>> __GENMASK() does not need "h" and "l" being a constant.
>>>
>>> Yes, small_const_nbits(size) in find_next_bit() can guarantee that "size" is a
>>> constant and hence "h" argument in GENMASK_INPUT_CHECK() call is also a constant.
>>> But nothing can guarantee that "offset" is a constant, and hence nothing can
>>> guarantee that "l" argument in GENMASK_INPUT_CHECK() call is also a constant.
>>>
>>> Then, how can (l) > (h) in __builtin_constant_p((l) > (h)) be evaluated at build time
>>> if either l or h (i.e. "offset" and "size - 1" in find_next_bit()) lacks a guarantee of
>>> being a constant?
>>>
>>
>> So the idea is that if (l > h) is constant, __builtin_constant_p should
>> evaluate that, and if it is not it should use zero instead as input to
>> __builtin_chose_expr(). This works with non-const inputs in many other
>> places in the kernel, but apparently in this case with a certain
>> compiler, it doesn't so I guess we need to work around it.
>
> I have a vague memory that __builtin_constant_p() inside of
> __builtin_choose_expr()
> always evaluates to false because of the order in which the compiler processes
> those: If constant-folding only happens after __builtin_choose_expr(), then
> __builtin_constant_p() has to be false.
It's more complicated than that. __builtin_constant_p on something which
is a bona-fide Integer Constant Expression (ICE) gets folded early to a
1. And then it turns out that such a __builtin_constant_p() that folds
early to a 1 can be "stronger" than a literal 1, in the sense that when
used as the controlling expression of a ?: with nonsense in the false
branch, the former is OK but the latter fails:
https://lore.kernel.org/lkml/[email protected]/
Now what happens when the argument to __builtin_constant_p is not an ICE
is a lot more complicated. The argument _may_ be so obviously
non-constant that it can be folded early to a 0, hence still be suitable
as first argument to __b_c_e. But it is also possible that the compiler
leaves it unevaluated, in the "hope" that a later optimization stage
could prove the argument constant. And that's the case where __b_c_e
will then break, because that can't be left unevaluated for very long -
the very _type_ of the result depends on which branch is chosen.
tl;dr: there's no "order in which the compiler processes those", __b_c_p
can get evaluated (folded) early, before __b_c_e inspects it, or be left
for later stages.
Rasmus
On Wed, May 12, 2021 at 10:16 AM Rasmus Villemoes
<[email protected]> wrote:
> It's more complicated than that. __builtin_constant_p on something which
> is a bona-fide Integer Constant Expression (ICE) gets folded early to a
> 1. And then it turns out that such a __builtin_constant_p() that folds
> early to a 1 can be "stronger" than a literal 1, in the sense that when
> used as the controlling expression of a ?: with nonsense in the false
> branch, the former is OK but the latter fails:
>
> https://lore.kernel.org/lkml/[email protected]/
>
> Now what happens when the argument to __builtin_constant_p is not an ICE
> is a lot more complicated. The argument _may_ be so obviously
> non-constant that it can be folded early to a 0, hence still be suitable
> as first argument to __b_c_e. But it is also possible that the compiler
> leaves it unevaluated, in the "hope" that a later optimization stage
> could prove the argument constant. And that's the case where __b_c_e
> will then break, because that can't be left unevaluated for very long -
> the very _type_ of the result depends on which branch is chosen.
>
> tl;dr: there's no "order in which the compiler processes those", __b_c_p
> can get evaluated (folded) early, before __b_c_e inspects it, or be left
> for later stages.
Thanks for the detailed explanation. Checking the actual behavior of
a trivial example, I find that
int f(void)
{
const int i = 1;
return __builtin_choose_expr(__builtin_constant_p(i), 1, 2);
}
used to return '2' with gcc-7, which is what I remembered.
With gcc-8 and up as well as any version of clang, it returns '1' now:
https://godbolt.org/z/7eKjbMocb
I have also seen a couple of cases where __builtin_constant_p()
without a __builtin_choose_expr() ended up unexpectedly
returning true when gcc found a code path that it would be constant
(e.g. conditionally initializing a variable to one of two possible
ICEs), but then later turning that back into a non-constant
expression in a later optimization stage. There is probably also
a much more detailed explanation behind those.
Arnd