[linux-next:master 7012/7430] include/linux/compiler_types.h:328:38: error: call to '__compiletime_assert_183' declared with attribute error: unexpected size in kmalloc_index()
4:10 p.m.
tree: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master
head: ccc252d2e818f6a479441119ad453c3ce7c7c461
commit: a7ba988ff9de37f0961b4bf96d17aca73d0d2e25 [7012/7430] mm, slub: change run-time
assertion in kmalloc_index() to compile-time
config: parisc-randconfig-r014-20210604 (attached as .config)
compiler: hppa-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
#
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commi...
git remote add linux-next
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
git fetch --no-tags linux-next master
git checkout a7ba988ff9de37f0961b4bf96d17aca73d0d2e25
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=parisc
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp(a)intel.com>
All errors (new ones prefixed by >>):
In file included from <command-line>:
In function 'kmalloc_index',
inlined from 'kmalloc_node' at include/linux/slab.h:572:20,
inlined from 'bpf_map_kmalloc_node.isra.0.part.0' at
include/linux/bpf.h:1319:9:
> include/linux/compiler_types.h:328:38: error: call to
'__compiletime_assert_183' declared with attribute error: unexpected size in
kmalloc_index()
328 | _compiletime_assert(condition, msg,
__compiletime_assert_, __COUNTER__)
| ^
include/linux/compiler_types.h:309:4: note: in definition of macro
'__compiletime_assert'
309 | prefix ## suffix(); \
| ^~~~~~
include/linux/compiler_types.h:328:2: note: in expansion of macro
'_compiletime_assert'
328 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
| ^~~~~~~~~~~~~~~~~~~
include/linux/build_bug.h:39:37: note: in expansion of macro
'compiletime_assert'
39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
| ^~~~~~~~~~~~~~~~~~
include/linux/slab.h:389:2: note: in expansion of macro 'BUILD_BUG_ON_MSG'
389 | BUILD_BUG_ON_MSG(1, "unexpected size in kmalloc_index()");
| ^~~~~~~~~~~~~~~~
vim +/__compiletime_assert_183 +328 include/linux/compiler_types.h
eb5c2d4b45e3d2 Will Deacon 2020-07-21 314
eb5c2d4b45e3d2 Will Deacon 2020-07-21 315 #define _compiletime_assert(condition, msg,
prefix, suffix) \
eb5c2d4b45e3d2 Will Deacon 2020-07-21 316 __compiletime_assert(condition, msg, prefix,
suffix)
eb5c2d4b45e3d2 Will Deacon 2020-07-21 317
eb5c2d4b45e3d2 Will Deacon 2020-07-21 318 /**
eb5c2d4b45e3d2 Will Deacon 2020-07-21 319 * compiletime_assert - break build and emit
msg if condition is false
eb5c2d4b45e3d2 Will Deacon 2020-07-21 320 * @condition: a compile-time constant
condition to check
eb5c2d4b45e3d2 Will Deacon 2020-07-21 321 * @msg: a message to emit if condition
is false
eb5c2d4b45e3d2 Will Deacon 2020-07-21 322 *
eb5c2d4b45e3d2 Will Deacon 2020-07-21 323 * In tradition of POSIX assert, this macro
will break the build if the
eb5c2d4b45e3d2 Will Deacon 2020-07-21 324 * supplied condition is *false*, emitting the
supplied error message if the
eb5c2d4b45e3d2 Will Deacon 2020-07-21 325 * compiler has support to do so.
eb5c2d4b45e3d2 Will Deacon 2020-07-21 326 */
eb5c2d4b45e3d2 Will Deacon 2020-07-21 327 #define compiletime_assert(condition, msg) \
eb5c2d4b45e3d2 Will Deacon 2020-07-21 @328 _compiletime_assert(condition, msg,
__compiletime_assert_, __COUNTER__)
eb5c2d4b45e3d2 Will Deacon 2020-07-21 329
:::::: The code at line 328 was first introduced by commit
:::::: eb5c2d4b45e3d2d5d052ea6b8f1463976b1020d5 compiler.h: Move compiletime_assert()
macros into compiler_types.h
:::::: TO: Will Deacon <will(a)kernel.org>
:::::: CC: Will Deacon <will(a)kernel.org>
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
9:08 p.m.
On Sat, Jun 05, 2021 at 02:10:46PM +0800, kernel test robot wrote:
tree:
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master
head: ccc252d2e818f6a479441119ad453c3ce7c7c461
commit: a7ba988ff9de37f0961b4bf96d17aca73d0d2e25 [7012/7430] mm, slub: change run-time
assertion in kmalloc_index() to compile-time
config: parisc-randconfig-r014-20210604 (attached as .config)
compiler: hppa-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
#
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commi...
git remote add linux-next
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
git fetch --no-tags linux-next master
git checkout a7ba988ff9de37f0961b4bf96d17aca73d0d2e25
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=parisc
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp(a)intel.com>
All errors (new ones prefixed by >>):
In file included from <command-line>:
In function 'kmalloc_index',
inlined from 'kmalloc_node' at include/linux/slab.h:572:20,
inlined from 'bpf_map_kmalloc_node.isra.0.part.0' at
include/linux/bpf.h:1319:9:
>> include/linux/compiler_types.h:328:38: error: call to
'__compiletime_assert_183' declared with attribute error: unexpected size in
kmalloc_index()
328 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
| ^
include/linux/compiler_types.h:309:4: note: in definition of macro
'__compiletime_assert'
309 | prefix ## suffix(); \
| ^~~~~~
include/linux/compiler_types.h:328:2: note: in expansion of macro
'_compiletime_assert'
328 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
| ^~~~~~~~~~~~~~~~~~~
include/linux/build_bug.h:39:37: note: in expansion of macro
'compiletime_assert'
39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
| ^~~~~~~~~~~~~~~~~~
include/linux/slab.h:389:2: note: in expansion of macro 'BUILD_BUG_ON_MSG'
389 | BUILD_BUG_ON_MSG(1, "unexpected size in kmalloc_index()");
| ^~~~~~~~~~~~~~~~
Reproduce with attached config, and read the code.
It has no problem in clang (clang-10/clang-11). it is problem with gcc.
I found two ways to solve the error (maybe there would be better
solution) Any thoughts or opinions?
First ways is to change condition of kmalloc_index macro.
diff --git a/include/linux/slab.h b/include/linux/slab.h
index 70e46db766ca..be2c900cba4b 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -397,7 +397,7 @@ static __always_inline unsigned int __kmalloc_index(size_t size,
/* Will never be reached. Needed because the compiler may complain */
return -1;
}
-#define kmalloc_index(s) __kmalloc_index(s, true)
+#define kmalloc_index(s) __kmalloc_index(s, __builtin_constant_p(s) && true)
#endif /* !CONFIG_SLOB */
void *__kmalloc(size_t size, gfp_t flags) __assume_kmalloc_alignment __malloc;
Second way is making bpf_map_kmalloc_node always inline.
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 02b02cb29ce2..09379d705349 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1312,7 +1312,7 @@ void *bpf_map_kzalloc(const struct bpf_map *map, size_t size, gfp_t
flags);
void __percpu *bpf_map_alloc_percpu(const struct bpf_map *map, size_t size,
size_t align, gfp_t flags);
#else
-static inline void *
+static __always_inline void *
bpf_map_kmalloc_node(const struct bpf_map *map, size_t size, gfp_t flags,
int node)
{
9:40 p.m.
On 6/6/21 1:08 PM, Hyeonggon Yoo wrote:
On Sat, Jun 05, 2021 at 02:10:46PM +0800, kernel test robot wrote:
> tree: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master
> head: ccc252d2e818f6a479441119ad453c3ce7c7c461
> commit: a7ba988ff9de37f0961b4bf96d17aca73d0d2e25 [7012/7430] mm, slub: change
run-time assertion in kmalloc_index() to compile-time
> config: parisc-randconfig-r014-20210604 (attached as .config)
> compiler: hppa-linux-gcc (GCC) 9.3.0
> reproduce (this is a W=1 build):
> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross
-O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> #
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commi...
> git remote add linux-next
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
> git fetch --no-tags linux-next master
> git checkout a7ba988ff9de37f0961b4bf96d17aca73d0d2e25
> # save the attached .config to linux build tree
> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=parisc
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp(a)intel.com>
>
> All errors (new ones prefixed by >>):
> In file included from <command-line>:
> In function 'kmalloc_index',
> inlined from 'kmalloc_node' at include/linux/slab.h:572:20,
> inlined from 'bpf_map_kmalloc_node.isra.0.part.0' at
include/linux/bpf.h:1319:9:
> >> include/linux/compiler_types.h:328:38: error: call to
'__compiletime_assert_183' declared with attribute error: unexpected size in
kmalloc_index()
> 328 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
> | ^
> include/linux/compiler_types.h:309:4: note: in definition of macro
'__compiletime_assert'
> 309 | prefix ## suffix(); \
> | ^~~~~~
> include/linux/compiler_types.h:328:2: note: in expansion of macro
'_compiletime_assert'
> 328 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
> | ^~~~~~~~~~~~~~~~~~~
> include/linux/build_bug.h:39:37: note: in expansion of macro
'compiletime_assert'
> 39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
> | ^~~~~~~~~~~~~~~~~~
> include/linux/slab.h:389:2: note: in expansion of macro
'BUILD_BUG_ON_MSG'
> 389 | BUILD_BUG_ON_MSG(1, "unexpected size in kmalloc_index()");
> | ^~~~~~~~~~~~~~~~
Reproduce with attached config, and read the code.
It has no problem in clang (clang-10/clang-11). it is problem with gcc.
But what exactly is the gcc problem here?
Did you have to reproduce it with specific gcc version and/or architecture?
I found two ways to solve the error (maybe there would be better
solution) Any thoughts or opinions?
First ways is to change condition of kmalloc_index macro.
diff --git a/include/linux/slab.h b/include/linux/slab.h
index 70e46db766ca..be2c900cba4b 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -397,7 +397,7 @@ static __always_inline unsigned int __kmalloc_index(size_t size,
/* Will never be reached. Needed because the compiler may complain */
return -1;
}
-#define kmalloc_index(s) __kmalloc_index(s, true)
+#define kmalloc_index(s) __kmalloc_index(s, __builtin_constant_p(s) && true)
I wonder how this extra guard can possibly matter?
bpf_map_kmalloc_node()
kmalloc_node()
if (__builtin_constant_p(size) ...)
unsigned int i = kmalloc_index(size);
We shouldn't be even reaching kmalloc_index() unless __builtin_constant_p(size)
is already true.
#endif /* !CONFIG_SLOB */
void *__kmalloc(size_t size, gfp_t flags) __assume_kmalloc_alignment __malloc;
Second way is making bpf_map_kmalloc_node always inline.
If bpf_map_kmalloc_node() is not being compiled as inline, how can it possibly
evaluate __builtin_constant_p(size) as true when processing the inlined
kmalloc_node()?
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 02b02cb29ce2..09379d705349 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1312,7 +1312,7 @@ void *bpf_map_kzalloc(const struct bpf_map *map, size_t size, gfp_t
flags);
void __percpu *bpf_map_alloc_percpu(const struct bpf_map *map, size_t size,
size_t align, gfp_t flags);
#else
-static inline void *
+static __always_inline void *
bpf_map_kmalloc_node(const struct bpf_map *map, size_t size, gfp_t flags,
int node)
{
10:25 p.m.
On Mon, Jun 07, 2021 at 01:40:02PM +0200, Vlastimil Babka wrote:
On 6/6/21 1:08 PM, Hyeonggon Yoo wrote:
> On Sat, Jun 05, 2021 at 02:10:46PM +0800, kernel test robot wrote:
>> tree: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
master
>> head: ccc252d2e818f6a479441119ad453c3ce7c7c461
>> commit: a7ba988ff9de37f0961b4bf96d17aca73d0d2e25 [7012/7430] mm, slub: change
run-time assertion in kmalloc_index() to compile-time
>> config: parisc-randconfig-r014-20210604 (attached as .config)
>> compiler: hppa-linux-gcc (GCC) 9.3.0
>> reproduce (this is a W=1 build):
>> wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
>> chmod +x ~/bin/make.cross
>> #
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commi...
>> git remote add linux-next
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
>> git fetch --no-tags linux-next master
>> git checkout a7ba988ff9de37f0961b4bf96d17aca73d0d2e25
>> # save the attached .config to linux build tree
>> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross
ARCH=parisc
>>
>> If you fix the issue, kindly add following tag as appropriate
>> Reported-by: kernel test robot <lkp(a)intel.com>
>>
>> All errors (new ones prefixed by >>):
>> In file included from <command-line>:
>> In function 'kmalloc_index',
>> inlined from 'kmalloc_node' at include/linux/slab.h:572:20,
>> inlined from 'bpf_map_kmalloc_node.isra.0.part.0' at
include/linux/bpf.h:1319:9:
>> >> include/linux/compiler_types.h:328:38: error: call to
'__compiletime_assert_183' declared with attribute error: unexpected size in
kmalloc_index()
>> 328 | _compiletime_assert(condition, msg, __compiletime_assert_,
__COUNTER__)
>> | ^
>> include/linux/compiler_types.h:309:4: note: in definition of macro
'__compiletime_assert'
>> 309 | prefix ## suffix(); \
>> | ^~~~~~
>> include/linux/compiler_types.h:328:2: note: in expansion of macro
'_compiletime_assert'
>> 328 | _compiletime_assert(condition, msg, __compiletime_assert_,
__COUNTER__)
>> | ^~~~~~~~~~~~~~~~~~~
>> include/linux/build_bug.h:39:37: note: in expansion of macro
'compiletime_assert'
>> 39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
>> | ^~~~~~~~~~~~~~~~~~
>> include/linux/slab.h:389:2: note: in expansion of macro
'BUILD_BUG_ON_MSG'
>> 389 | BUILD_BUG_ON_MSG(1, "unexpected size in
kmalloc_index()");
>> | ^~~~~~~~~~~~~~~~
>
> Reproduce with attached config, and read the code.
> It has no problem in clang (clang-10/clang-11). it is problem with gcc.
But what exactly is the gcc problem here?
Did you have to reproduce it with specific gcc version and/or architecture?
Before replying, I should say that I'm not an expert on gcc.
I just tried some ways to fix the error, and it seemed to me that
gcc is doing something wrong.
I found the error was in kernel/bpf/local_storage.c
and I copied the bot's config in linux-next (20210607), and just entered all new
config.
running 'make kernel/bpf/local_storage.o CC=gcc' can reproduce the
error.
the bot says it is error with parisc, but I was able to reproduce
it in my x86 machine.
I tested on gcc-9.3.0 and gcc-10.2.0 both makes an error,
and clang 10.0.1, clang 11.0.0 didn't make an error.
> I found two ways to solve the error (maybe there would be
better
> solution) Any thoughts or opinions?
>
>
> First ways is to change condition of kmalloc_index macro.
>
> diff --git a/include/linux/slab.h b/include/linux/slab.h
> index 70e46db766ca..be2c900cba4b 100644
> --- a/include/linux/slab.h
> +++ b/include/linux/slab.h
> @@ -397,7 +397,7 @@ static __always_inline unsigned int __kmalloc_index(size_t
size,
> /* Will never be reached. Needed because the compiler may complain */
> return -1;
> }
> -#define kmalloc_index(s) __kmalloc_index(s, true)
> +#define kmalloc_index(s) __kmalloc_index(s, __builtin_constant_p(s) &&
true)
I wonder how this extra guard can possibly matter?
bpf_map_kmalloc_node()
kmalloc_node()
if (__builtin_constant_p(size) ...)
unsigned int i = kmalloc_index(size);
We shouldn't be even reaching kmalloc_index() unless __builtin_constant_p(size)
is already true.
Yes, I knew that when I wrote the code. That totally doesn't make sense.
why __builtin_constant_p(size) was true in kmalloc_node,
and false in the evalaution (__builtin_constant_p(s) && true)?
but it actually solves the error.
At this point, I thought gcc was doing something wrong....
Well, I know we need more evidence to conclude gcc is wrong.
(Or we made wrong code that makes compiler confusing.)
I want to hear you and some other people's opinion.
> #endif /* !CONFIG_SLOB */
>
> void *__kmalloc(size_t size, gfp_t flags) __assume_kmalloc_alignment __malloc;
>
>
> Second way is making bpf_map_kmalloc_node always inline.
If bpf_map_kmalloc_node() is not being compiled as inline, how can it possibly
evaluate __builtin_constant_p(size) as true when processing the inlined
kmalloc_node()?
As I said above - it doesn't make sense, but gcc is acting differently
on __inline and inline, in bpf_map_kmalloc_node function. just making
bpf_map_kmalloc_node always solves the error, but I have no clue WHY...
anyway, in summary:
- the diff I sent should not make sense, but it works for gcc.
- So I think gcc is doing something wrong (but more evidence needed)
I can be missing something. So If I said something wrong, or if you
can't reproduce the error, please tell me!
Thanks,
Hyeonggon Yoo
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index 02b02cb29ce2..09379d705349 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -1312,7 +1312,7 @@ void *bpf_map_kzalloc(const struct bpf_map *map, size_t
size, gfp_t flags);
> > void __percpu *bpf_map_alloc_percpu(const struct bpf_map *map, size_t size,
> > size_t align, gfp_t flags);
> > #else
> > -static inline void *
> > +static __always_inline void *
> > bpf_map_kmalloc_node(const struct bpf_map *map, size_t size, gfp_t flags,
> > int node)
> > {
> >
>
1:27 a.m.
On 6/7/21 2:25 PM, Hyeonggon Yoo wrote:
On Mon, Jun 07, 2021 at 01:40:02PM +0200, Vlastimil Babka wrote:
> On 6/6/21 1:08 PM, Hyeonggon Yoo wrote:
> > On Sat, Jun 05, 2021 at 02:10:46PM +0800, kernel test robot wrote:
> >> tree: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
master
> >> head: ccc252d2e818f6a479441119ad453c3ce7c7c461
> >> commit: a7ba988ff9de37f0961b4bf96d17aca73d0d2e25 [7012/7430] mm, slub:
change run-time assertion in kmalloc_index() to compile-time
> >> config: parisc-randconfig-r014-20210604 (attached as .config)
> >> compiler: hppa-linux-gcc (GCC) 9.3.0
> >> reproduce (this is a W=1 build):
> >> wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
> >> chmod +x ~/bin/make.cross
> >> #
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commi...
> >> git remote add linux-next
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
> >> git fetch --no-tags linux-next master
> >> git checkout a7ba988ff9de37f0961b4bf96d17aca73d0d2e25
> >> # save the attached .config to linux build tree
> >> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross
ARCH=parisc
> >>
> >> If you fix the issue, kindly add following tag as appropriate
> >> Reported-by: kernel test robot <lkp(a)intel.com>
> >>
> >> All errors (new ones prefixed by >>):
> >> In file included from <command-line>:
> >> In function 'kmalloc_index',
> >> inlined from 'kmalloc_node' at include/linux/slab.h:572:20,
> >> inlined from 'bpf_map_kmalloc_node.isra.0.part.0' at
include/linux/bpf.h:1319:9:
> >> >> include/linux/compiler_types.h:328:38: error: call to
'__compiletime_assert_183' declared with attribute error: unexpected size in
kmalloc_index()
> >> 328 | _compiletime_assert(condition, msg, __compiletime_assert_,
__COUNTER__)
> >> | ^
> >> include/linux/compiler_types.h:309:4: note: in definition of macro
'__compiletime_assert'
> >> 309 | prefix ## suffix(); \
> >> | ^~~~~~
> >> include/linux/compiler_types.h:328:2: note: in expansion of macro
'_compiletime_assert'
> >> 328 | _compiletime_assert(condition, msg, __compiletime_assert_,
__COUNTER__)
> >> | ^~~~~~~~~~~~~~~~~~~
> >> include/linux/build_bug.h:39:37: note: in expansion of macro
'compiletime_assert'
> >> 39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond),
msg)
> >> | ^~~~~~~~~~~~~~~~~~
> >> include/linux/slab.h:389:2: note: in expansion of macro
'BUILD_BUG_ON_MSG'
> >> 389 | BUILD_BUG_ON_MSG(1, "unexpected size in
kmalloc_index()");
> >> | ^~~~~~~~~~~~~~~~
> >
> > Reproduce with attached config, and read the code.
> > It has no problem in clang (clang-10/clang-11). it is problem with gcc.
>
> But what exactly is the gcc problem here?
> Did you have to reproduce it with specific gcc version and/or architecture?
>
Before replying, I should say that I'm not an expert on gcc.
I just tried some ways to fix the error, and it seemed to me that
gcc is doing something wrong.
I'm involving my gcc colleagues, will report results...
1:49 a.m.
On Mon, Jun 07, 2021 at 05:27:27PM +0200, Vlastimil Babka wrote:
On 6/7/21 2:25 PM, Hyeonggon Yoo wrote:
> On Mon, Jun 07, 2021 at 01:40:02PM +0200, Vlastimil Babka wrote:
>> On 6/6/21 1:08 PM, Hyeonggon Yoo wrote:
>> > On Sat, Jun 05, 2021 at 02:10:46PM +0800, kernel test robot wrote:
>> >> tree:
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master
>> >> head: ccc252d2e818f6a479441119ad453c3ce7c7c461
>> >> commit: a7ba988ff9de37f0961b4bf96d17aca73d0d2e25 [7012/7430] mm, slub:
change run-time assertion in kmalloc_index() to compile-time
>> >> config: parisc-randconfig-r014-20210604 (attached as .config)
>> >> compiler: hppa-linux-gcc (GCC) 9.3.0
>> >> reproduce (this is a W=1 build):
>> >> wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
>> >> chmod +x ~/bin/make.cross
>> >> #
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commi...
>> >> git remote add linux-next
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
>> >> git fetch --no-tags linux-next master
>> >> git checkout a7ba988ff9de37f0961b4bf96d17aca73d0d2e25
>> >> # save the attached .config to linux build tree
>> >> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross
ARCH=parisc
>> >>
>> >> If you fix the issue, kindly add following tag as appropriate
>> >> Reported-by: kernel test robot <lkp(a)intel.com>
>> >>
>> >> All errors (new ones prefixed by >>):
>> >> In file included from <command-line>:
>> >> In function 'kmalloc_index',
>> >> inlined from 'kmalloc_node' at
include/linux/slab.h:572:20,
>> >> inlined from 'bpf_map_kmalloc_node.isra.0.part.0' at
include/linux/bpf.h:1319:9:
>> >> >> include/linux/compiler_types.h:328:38: error: call to
'__compiletime_assert_183' declared with attribute error: unexpected size in
kmalloc_index()
>> >> 328 | _compiletime_assert(condition, msg, __compiletime_assert_,
__COUNTER__)
>> >> | ^
>> >> include/linux/compiler_types.h:309:4: note: in definition of macro
'__compiletime_assert'
>> >> 309 | prefix ## suffix(); \
>> >> | ^~~~~~
>> >> include/linux/compiler_types.h:328:2: note: in expansion of macro
'_compiletime_assert'
>> >> 328 | _compiletime_assert(condition, msg, __compiletime_assert_,
__COUNTER__)
>> >> | ^~~~~~~~~~~~~~~~~~~
>> >> include/linux/build_bug.h:39:37: note: in expansion of macro
'compiletime_assert'
>> >> 39 | #define BUILD_BUG_ON_MSG(cond, msg)
compiletime_assert(!(cond), msg)
>> >> | ^~~~~~~~~~~~~~~~~~
>> >> include/linux/slab.h:389:2: note: in expansion of macro
'BUILD_BUG_ON_MSG'
>> >> 389 | BUILD_BUG_ON_MSG(1, "unexpected size in
kmalloc_index()");
>> >> | ^~~~~~~~~~~~~~~~
>> >
>> > Reproduce with attached config, and read the code.
>> > It has no problem in clang (clang-10/clang-11). it is problem with gcc.
>>
>> But what exactly is the gcc problem here?
>> Did you have to reproduce it with specific gcc version and/or architecture?
>>
>
> Before replying, I should say that I'm not an expert on gcc.
> I just tried some ways to fix the error, and it seemed to me that
> gcc is doing something wrong.
I'm involving my gcc colleagues, will report results...
Thank you so much!
Hyeonggon
5:57 p.m.
On 6/7/21 5:49 PM, Hyeonggon Yoo wrote:
On Mon, Jun 07, 2021 at 05:27:27PM +0200, Vlastimil Babka wrote:
> On 6/7/21 2:25 PM, Hyeonggon Yoo wrote:
> > On Mon, Jun 07, 2021 at 01:40:02PM +0200, Vlastimil Babka wrote:
> >> On 6/6/21 1:08 PM, Hyeonggon Yoo wrote:
> >> > On Sat, Jun 05, 2021 at 02:10:46PM +0800, kernel test robot wrote:
> >>
> >> But what exactly is the gcc problem here?
> >> Did you have to reproduce it with specific gcc version and/or architecture?
> >>
> >
> > Before replying, I should say that I'm not an expert on gcc.
> > I just tried some ways to fix the error, and it seemed to me that
> > gcc is doing something wrong.
>
> I'm involving my gcc colleagues, will report results...
Well, it seems the bot's .config included CONFIG_PROFILE_ALL_BRANCHES as the
main factor to trigger the problem. And (according to my colleagues) it might
add too many instrumented if conditions to sustain the builtin-constant
tracking, which is not unlimited, or interact with optimizations in some other
corner case way.
I guess we could add IS_ENABLED(CONFIG_PROFILE_ALL_BRANCHES) to the list of
conditions that excludes using BUILD_BUG_ON_MSG().
Thank you so much!
Hyeonggon
3:05 a.m.
On Tue, Jun 08, 2021 at 09:57:18AM +0200, Vlastimil Babka wrote:
On 6/7/21 5:49 PM, Hyeonggon Yoo wrote:
> On Mon, Jun 07, 2021 at 05:27:27PM +0200, Vlastimil Babka wrote:
>> On 6/7/21 2:25 PM, Hyeonggon Yoo wrote:
>> > On Mon, Jun 07, 2021 at 01:40:02PM +0200, Vlastimil Babka wrote:
>> >> On 6/6/21 1:08 PM, Hyeonggon Yoo wrote:
>> >> > On Sat, Jun 05, 2021 at 02:10:46PM +0800, kernel test robot
wrote:
>> >>
>> >> But what exactly is the gcc problem here?
>> >> Did you have to reproduce it with specific gcc version and/or
architecture?
>> >>
>> >
>> > Before replying, I should say that I'm not an expert on gcc.
>> > I just tried some ways to fix the error, and it seemed to me that
>> > gcc is doing something wrong.
>>
>> I'm involving my gcc colleagues, will report results...
Well, it seems the bot's .config included CONFIG_PROFILE_ALL_BRANCHES as the
main factor to trigger the problem. And (according to my colleagues)
Wow, AWESOME! How did your colleague find it? that was a big hint for me.
when CONFIG_PROFILE_ALL_BRANCHES is not set, it doesn't make an error.
it might add too many instrumented if conditions to sustain the
builtin-constant
tracking, which is not unlimited, or interact with optimizations in some other
corner case way.
I guess we could add IS_ENABLED(CONFIG_PROFILE_ALL_BRANCHES) to the
list of
conditions that excludes using BUILD_BUG_ON_MSG().
Well I wanted to understand how CONFIG_PROFILE_ALL_BRANCHES excludes
BUILD_BUG_ON This is gcc's preprocessor output.
So let's start from what CONFIG_PROFILE_ALL_BRANCHES does.
#ifdef CONFIG_PROFILE_ALL_BRANCHES
/*
* "Define 'is'", Bill Clinton
* "Define 'if'", Steven Rostedt
*/
#define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
#define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) :
__trace_if_value(cond))
#define __trace_if_value(cond) ({ \
static struct ftrace_branch_data \
__aligned(4) \
__section("_ftrace_branch") \
__if_trace = { \
.func = __func__, \
.file = __FILE__, \
.line = __LINE__, \
}; \
(cond) ? \
(__if_trace.miss_hit[1]++,1) : \
(__if_trace.miss_hit[0]++,0); \
})
#endif /* CONFIG_PROFILE_ALL_BRANCHES */
it seems it records non-constant condition's hit or miss.
if cond is constant, do nothing. else, records its hit or miss at
runtime.
then let's look at kmalloc_node, which is called by bpf_map_kmalloc_node.
static inline __attribute__((__gnu_inline__)) __attribute__((__unused__))
__attribute__((no_instrument_function)) __attribute__((__always_inline__))
void *kmalloc_node(size_t size, gfp_t flags, int node){
if ( (__builtin_constant_p(
!!(__builtin_constant_p(size)
&& size <= (1UL << ((11 + 12 - 1) <= 25 ? (11 + 12 - 1) :
25))))
? (!!(__builtin_constant_p(size) && size <= (1UL << ((11 + 12
- 1) <= 25 ? (11 + 12 - 1) : 25))))
: ({
static struct ftrace_branch_data __attribute__ ((__aligned__(4)))
__attribute__((__section__("_ftrace_branch")))
__if_trace =
{ .func = __func__,
.file = "include/linux/slab.h",
.line = 601,
};
(!!(__builtin_constant_p(size) && size <= (1UL << ((11 + 12 - 1) <=
25 ? (11 + 12 - 1) : 25))))
? (__if_trace.miss_hit[1]++,1)
: (__if_trace.miss_hit[0]++,0); })) )
{
unsigned int i = __kmalloc_index(size, true);
It seems that gcc evaluates
__builtin_constant_p(
!!(builtin_constant_p(size)
&& size <= KMALLOC_MAX_CACHE_SIZE)
)
as true.
mm.. when the size passed to bpf_map_kmalloc_node is not constant,
(__builtin_constant_p(size) && size <= KMALLOC_MAX_CACHE_SIZE) is false.
then __builtin_constant_p(!!false) is true. So it calls kmalloc_index.
what change should be made?
just checking CONFIG_PROFILE_ALL_BRANCHES to kmalloc_index's if condition
doesn't make sense, because kmalloc_node is not working as expected.
Plus, BUILD_BUG_ON_MSG seems not working with clang.
Below is generated by clang 11.0.0 preprocessor, when compiling
mm/kfence/kfence_test.o
Well, I'm going to read more code on BUILD_BUG_XXX family,
but if it doens't work on clang, we should change condition that we
made.
if ( (__builtin_constant_p(!!((0 || 110000 >= 110000) && size_is_constant)) ?
(!!((0 || 110000 >= 110000) && size_is_constant)) : ({ static struct
ftrace_branch_data __attribute__((__aligned__(4)))
__attribute__((__section__("_ftrace_branch"))) __if_trace = { .func = __func__,
.file = "include/linux/slab.h", .line = 416, }; (!!((0 || 110000 >= 110000)
&& size_is_constant)) ? (__if_trace.miss_hit[1]++,1) :
(__if_trace.miss_hit[0]++,0); })) )
do {
extern void __compiletime_assert_131(void) ;
// here - compiletime_assert_131 does NOTHING
if ( (__builtin_constant_p(!!(!(!(1))))
? (!!(!(!(1))))
: ({ static struct ftrace_branch_data __attribute__((__aligned__(4)))
__attribute__((__section__("_ftrace_branch"))) __if_trace = { .func = __func__,
.file = "include/linux/slab.h", .line = 417, }; (!!(!(!(1)))) ?
(__if_trace.miss_hit[1]++,1) : (__if_trace.miss_hit[0]++,0); })) )
__compiletime_assert_131(); } while (0);
else
do { do { } while(0); do { asm __inline volatile("1:\t" ".byte 0x0f,
0x0b" "\n" ".pushsection __bug_table,\"aw\"\n"
"2:\t" ".long " "1b" " - 2b" "\t#
bug_entry::bug_addr\n" "\t.word %c0" "\t# bug_entry::flags\n"
"\t.org 2b+%c1\n" ".popsection" : : "i" (0), "i"
(sizeof(struct bug_entry))); } while (0); do { ({ asm volatile("132"
":\n\t" ".pushsection .discard.unreachable\n\t" ".long "
"132" "b - .\n\t" ".popsection\n\t"); });
__builtin_unreachable(); } while (0); } while (0);
return -1;
}
Maybe it's because of definition of __compiletime_assert.
#ifdef __OPTIMIZE__
# define __compiletime_assert(condition, msg, prefix, suffix) \
do { \
extern void prefix ## suffix(void) __compiletime_error(msg); \
if (!(condition)) \
prefix ## suffix(); \
} while (0)
#else
# define __compiletime_assert(condition, msg, prefix, suffix) do { } while (0)
#endif
There can be an logical error or misunderstanding on my words.
If so, please tell me!
Thanks,
Hyeonggon
3:27 a.m.
On 6/8/21 7:05 PM, Hyeonggon Yoo wrote:
On Tue, Jun 08, 2021 at 09:57:18AM +0200, Vlastimil Babka wrote:
> On 6/7/21 5:49 PM, Hyeonggon Yoo wrote:
> > On Mon, Jun 07, 2021 at 05:27:27PM +0200, Vlastimil Babka wrote:
> >> On 6/7/21 2:25 PM, Hyeonggon Yoo wrote:
> >> > On Mon, Jun 07, 2021 at 01:40:02PM +0200, Vlastimil Babka wrote:
> >> >> On 6/6/21 1:08 PM, Hyeonggon Yoo wrote:
> >> >> > On Sat, Jun 05, 2021 at 02:10:46PM +0800, kernel test robot
wrote:
> >> >>
> >> >> But what exactly is the gcc problem here?
> >> >> Did you have to reproduce it with specific gcc version and/or
architecture?
> >> >>
> >> >
> >> > Before replying, I should say that I'm not an expert on gcc.
> >> > I just tried some ways to fix the error, and it seemed to me that
> >> > gcc is doing something wrong.
> >>
> >> I'm involving my gcc colleagues, will report results...
>
> Well, it seems the bot's .config included CONFIG_PROFILE_ALL_BRANCHES as the
> main factor to trigger the problem. And (according to my colleagues)
Wow, AWESOME! How did your colleague find it? that was a big hint for me.
when CONFIG_PROFILE_ALL_BRANCHES is not set, it doesn't make an error.
Well, we started with me doing "make kernel/bpf/local_storage.i" to create a
preprocessed C source that can be copied out and debugged outside of the whole
kernel build context. I send that to my colleague and he reduced the testcase
using cvise:
https://gcc.gnu.org/wiki/A_guide_to_testcase_reduction
While the reduction wasn't successful in preserving enough of the testcase, from
the result it was clear that there was lots of ftrace_branch_data stuff and so
it was easy to find this is due to
CONFIG_PROFILE_ALL_BRANCHES.
> it might add too many instrumented if conditions to sustain the
builtin-constant
> tracking, which is not unlimited, or interact with optimizations in some other
> corner case way.
> I guess we could add IS_ENABLED(CONFIG_PROFILE_ALL_BRANCHES) to the list of
> conditions that excludes using BUILD_BUG_ON_MSG().
Well I wanted to understand how CONFIG_PROFILE_ALL_BRANCHES excludes
BUILD_BUG_ON This is gcc's preprocessor output.
So let's start from what CONFIG_PROFILE_ALL_BRANCHES does.
#ifdef CONFIG_PROFILE_ALL_BRANCHES
/*
* "Define 'is'", Bill Clinton
* "Define 'if'", Steven Rostedt
*/
#define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
#define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) :
__trace_if_value(cond))
#define __trace_if_value(cond) ({ \
static struct ftrace_branch_data \
__aligned(4) \
__section("_ftrace_branch") \
__if_trace = { \
.func = __func__, \
.file = __FILE__, \
.line = __LINE__, \
}; \
(cond) ? \
(__if_trace.miss_hit[1]++,1) : \
(__if_trace.miss_hit[0]++,0); \
})
#endif /* CONFIG_PROFILE_ALL_BRANCHES */
it seems it records non-constant condition's hit or miss.
if cond is constant, do nothing. else, records its hit or miss at
runtime.
then let's look at kmalloc_node, which is called by bpf_map_kmalloc_node.
static inline __attribute__((__gnu_inline__)) __attribute__((__unused__))
__attribute__((no_instrument_function)) __attribute__((__always_inline__))
void *kmalloc_node(size_t size, gfp_t flags, int node){
if ( (__builtin_constant_p(
!!(__builtin_constant_p(size)
&& size <= (1UL << ((11 + 12 - 1) <= 25 ? (11 + 12 - 1) :
25))))
? (!!(__builtin_constant_p(size) && size <= (1UL << ((11 +
12 - 1) <= 25 ? (11 + 12 - 1) : 25))))
: ({
static struct ftrace_branch_data __attribute__ ((__aligned__(4)))
__attribute__((__section__("_ftrace_branch")))
__if_trace =
{ .func = __func__,
.file = "include/linux/slab.h",
.line = 601,
};
(!!(__builtin_constant_p(size) && size <= (1UL << ((11 + 12 - 1)
<= 25 ? (11 + 12 - 1) : 25))))
? (__if_trace.miss_hit[1]++,1)
: (__if_trace.miss_hit[0]++,0); })) )
{
unsigned int i = __kmalloc_index(size, true);
It seems that gcc evaluates
__builtin_constant_p(
!!(builtin_constant_p(size)
&& size <= KMALLOC_MAX_CACHE_SIZE)
)
as true.
mm.. when the size passed to bpf_map_kmalloc_node is not constant,
(__builtin_constant_p(size) && size <= KMALLOC_MAX_CACHE_SIZE) is false.
then __builtin_constant_p(!!false) is true. So it calls kmalloc_index.
what change should be made?
just checking CONFIG_PROFILE_ALL_BRANCHES to kmalloc_index's if condition
doesn't make sense, because kmalloc_node is not working as expected.
If I understood my colleagues right, the problem is that, while kmalloc_index()
seems to contains a number of *independent* "if (size <= X) conditions in a
sequence, the machinery of CONFIG_PROFILE_ALL_BRANCHES turns it to a deeply
nested if-then-else { if-then-else { if-then-else {...}}} thing (in fact using
the ternary operators, not if-then-else). At some point of the deep nesting gcc
"forgets" that __builtin_constant_p() is true and starts behaving as if it
wasn't.
Without CONFIG_PROFILE_ALL_BRANCHES it's able to keep track of it fine.
Plus, BUILD_BUG_ON_MSG seems not working with clang.
Below is generated by clang 11.0.0 preprocessor, when compiling
mm/kfence/kfence_test.o
Well, I'm going to read more code on BUILD_BUG_XXX family,
but if it doens't work on clang, we should change condition that we
made.
if ( (__builtin_constant_p(!!((0 || 110000 >= 110000) && size_is_constant)) ?
(!!((0 || 110000 >= 110000) && size_is_constant)) : ({ static struct
ftrace_branch_data __attribute__((__aligned__(4)))
__attribute__((__section__("_ftrace_branch"))) __if_trace = { .func = __func__,
.file = "include/linux/slab.h", .line = 416, }; (!!((0 || 110000 >= 110000)
&& size_is_constant)) ? (__if_trace.miss_hit[1]++,1) :
(__if_trace.miss_hit[0]++,0); })) )
do {
extern void __compiletime_assert_131(void) ;
// here - compiletime_assert_131 does NOTHING
It doesn't seem to do nothing? see below
if ( (__builtin_constant_p(!!(!(!(1))))
? (!!(!(!(1))))
: ({ static struct ftrace_branch_data __attribute__((__aligned__(4)))
__attribute__((__section__("_ftrace_branch"))) __if_trace = { .func = __func__,
.file = "include/linux/slab.h", .line = 417, }; (!!(!(!(1)))) ?
(__if_trace.miss_hit[1]++,1) : (__if_trace.miss_hit[0]++,0); })) )
__compiletime_assert_131(); } while (0);
The thing above seems to be exactly the "if (!(condition))
prefix ## suffix(); } while (0)" as the definition you posted below.
Anyway, you can verify that clang works by commenting out the highest size
checks and passing a constant size that makes it reach the BUILD_BUG_ON_MSG() ?
else
do { do { } while(0); do { asm __inline volatile("1:\t" ".byte 0x0f,
0x0b" "\n" ".pushsection __bug_table,\"aw\"\n"
"2:\t" ".long " "1b" " - 2b" "\t#
bug_entry::bug_addr\n" "\t.word %c0" "\t# bug_entry::flags\n"
"\t.org 2b+%c1\n" ".popsection" : : "i" (0), "i"
(sizeof(struct bug_entry))); } while (0); do { ({ asm volatile("132"
":\n\t" ".pushsection .discard.unreachable\n\t" ".long "
"132" "b - .\n\t" ".popsection\n\t"); });
__builtin_unreachable(); } while (0); } while (0);
return -1;
}
Maybe it's because of definition of __compiletime_assert.
#ifdef __OPTIMIZE__
# define __compiletime_assert(condition, msg, prefix, suffix) \
do { \
extern void prefix ## suffix(void) __compiletime_error(msg); \
if (!(condition)) \
prefix ## suffix(); \
} while (0)
#else
# define __compiletime_assert(condition, msg, prefix, suffix) do { } while (0)
#endif
There can be an logical error or misunderstanding on my words.
If so, please tell me!
Thanks,
Hyeonggon
4:45 a.m.
On Tue, Jun 08, 2021 at 07:27:52PM +0200, Vlastimil Babka wrote:
On 6/8/21 7:05 PM, Hyeonggon Yoo wrote:
> On Tue, Jun 08, 2021 at 09:57:18AM +0200, Vlastimil Babka wrote:
>> On 6/7/21 5:49 PM, Hyeonggon Yoo wrote:
>> > On Mon, Jun 07, 2021 at 05:27:27PM +0200, Vlastimil Babka wrote:
>> >> On 6/7/21 2:25 PM, Hyeonggon Yoo wrote:
>> >> > On Mon, Jun 07, 2021 at 01:40:02PM +0200, Vlastimil Babka wrote:
>> >> >> On 6/6/21 1:08 PM, Hyeonggon Yoo wrote:
>> >> >> > On Sat, Jun 05, 2021 at 02:10:46PM +0800, kernel test
robot wrote:
>> >> >>
>> >> >> But what exactly is the gcc problem here?
>> >> >> Did you have to reproduce it with specific gcc version and/or
architecture?
>> >> >>
>> >> >
>> >> > Before replying, I should say that I'm not an expert on gcc.
>> >> > I just tried some ways to fix the error, and it seemed to me that
>> >> > gcc is doing something wrong.
>> >>
>> >> I'm involving my gcc colleagues, will report results...
>>
>> Well, it seems the bot's .config included CONFIG_PROFILE_ALL_BRANCHES as
the
>> main factor to trigger the problem. And (according to my colleagues)
>
> Wow, AWESOME! How did your colleague find it? that was a big hint for me.
> when CONFIG_PROFILE_ALL_BRANCHES is not set, it doesn't make an error.
Well, we started with me doing "make kernel/bpf/local_storage.i" to create a
preprocessed C source that can be copied out and debugged outside of the whole
kernel build context. I send that to my colleague and he reduced the testcase
using cvise:
https://gcc.gnu.org/wiki/A_guide_to_testcase_reduction
While the reduction wasn't successful in preserving enough of the testcase, from
the result it was clear that there was lots of ftrace_branch_data stuff and so
it was easy to find this is due to
CONFIG_PROFILE_ALL_BRANCHES.
First time to hear about testcase reduction and cvise!
Thank you for letting me know it :)
I'm going to read it. but I ask some questions before that (see below)
> mm.. when the size passed to bpf_map_kmalloc_node is not
constant,
> (__builtin_constant_p(size) && size <= KMALLOC_MAX_CACHE_SIZE) is false.
> then __builtin_constant_p(!!false) is true. So it calls kmalloc_index.
>
> what change should be made?
> just checking CONFIG_PROFILE_ALL_BRANCHES to kmalloc_index's if condition
> doesn't make sense, because kmalloc_node is not working as expected.
If I understood my colleagues right, the problem is that, while kmalloc_index()
seems to contains a number of *independent* "if (size <= X) conditions in a
sequence, the machinery of CONFIG_PROFILE_ALL_BRANCHES turns it to a deeply
nested if-then-else { if-then-else { if-then-else {...}}} thing (in fact using
the ternary operators, not if-then-else).
At some point of the deep nesting gcc
"forgets" that __builtin_constant_p() is true and starts behaving as if it
wasn't.
Without CONFIG_PROFILE_ALL_BRANCHES it's able to keep track of it fine.
I think you are talking about some if statements in kmalloc_index.
How do you know gcc "forgets" __builtin_constant_p() is true?
can it be debugged using cvise? (not offending, just because
I don't know about cvise yet).
Also, as I understand right, kmalloc_index doesn't have information
about the value of __builtin_constant_p(size). the caller of
kmalloc_index (kmalloc_node here) has that information.
If I understand right, what CONFIG_PROFILE_ALL_BRANCHES does is below.
replacing if(cond) { body } -> if (
__builtin_constant_p(cond) ?
then (cond)
else (cond, record something)
) { body }
I think it does not make deep nested statements.
the below is some part of preprocessor output.
CONFIG_PROFILE_ALL_BRANCHES makes some ugly ternary operators,
but there is no deep-nested if-then-else statements.
if ( (__builtin_constant_p(!!(size <= 8)) ? (!!(size <= 8)) : ({ static struct
ftrace_branch_data __attribute__((__aligned__(4)))
__attribute__((__section__("_ftrace_branch"))) __if_trace = { .func = __func__,
.file = "include/linux/slab.h", .line = 392, }; (!!(size <= 8)) ?
(__if_trace.miss_hit[1]++,1) : (__if_trace.miss_hit[0]++,0); })) ) return 3;
if ( (__builtin_constant_p(!!(size <= 16)) ? (!!(size <= 16)) : ({ static struct
ftrace_branch_data __attribute__((__aligned__(4)))
__attribute__((__section__("_ftrace_branch"))) __if_trace = { .func = __func__,
.file = "include/linux/slab.h", .line = 393, }; (!!(size <= 16)) ?
(__if_trace.miss_hit[1]++,1) : (__if_trace.miss_hit[0]++,0); })) ) return 4;
if ( (__builtin_constant_p(!!(size <= 32)) ? (!!(size <= 32)) : ({ static struct
ftrace_branch_data __attribute__((__aligned__(4)))
__attribute__((__section__("_ftrace_branch"))) __if_trace = { .func = __func__,
.file = "include/linux/slab.h", .line = 394, }; (!!(size <= 32)) ?
(__if_trace.miss_hit[1]++,1) : (__if_trace.miss_hit[0]++,0); })) ) return 5;
.... and some if statements ...
And I think, the problem is on kmalloc_node, not on kmalloc_index.
the original code of kmalloc_node is below:
static __always_inline void *kmalloc_node(size_t size, gfp_t flags, int node)
{
#ifndef CONFIG_SLOB
if (__builtin_constant_p(size) &&
size <= KMALLOC_MAX_CACHE_SIZE) {
unsigned int i = kmalloc_index(size);
and which is changed to below: (by CONFIG_PROFILE_ALL_BRANCHES)
static __always_inline void *kmalloc_node(size_t size, gfp_t flags, int node)
{
if ( (
__builtin_constant_p(
!!(__builtin_constant_p(size) && size <= (1UL << ((11 + 12 - 1) <=
25 ? (11 + 12 - 1) : 25)))
)
? (!!(__builtin_constant_p(size) && size <= (1UL << ((11 + 12 - 1)
<= 25 ? (11 + 12 - 1) : 25))))
: ({ static struct ftrace_branch_data __attribute__((__aligned__(4)))
__attribute__((__section__("_ftrace_branch"))) __if_trace = { .func = __func__,
.file = "include/linux/slab.h", .line = 601, }; (!!(__builtin_constant_p(size)
&& size <= (1UL << ((11 + 12 - 1) <= 25 ? (11 + 12 - 1) : 25)))) ?
(__if_trace.miss_hit[1]++,1) : (__if_trace.miss_hit[0]++,0); })) )
{
unsigned int i = __kmalloc_index(size, true);
they are so ugly but the point is:
> It seems that gcc evaluates
>
> __builtin_constant_p(
> !!(builtin_constant_p(size)
> && size <= KMALLOC_MAX_CACHE_SIZE)
> )
> as true.
>
> mm.. when the size passed to bpf_map_kmalloc_node is not constant,
> (__builtin_constant_p(size) && size <= KMALLOC_MAX_CACHE_SIZE) is false.
> then __builtin_constant_p(!!false) is true. So it calls kmalloc_index.
>
> what change should be made?
> just checking CONFIG_PROFILE_ALL_BRANCHES to kmalloc_index's if condition
> doesn't make sense, because kmalloc_node is not working as expected.
some evidence to add:
there are 4 calls of bpf_map_kmalloc_node.
if you comment out two calls of bpf_map_kmalloc_node with non-constant
(in line 168, 508), it doesn't make an error. So I thought
it was problem when non-constant size was passed.
And if "kmalloc_index makes problem with non-constant size" is
true, then it is caller's fault because it is not allowed (except
in __kmalloc_index)
kmalloc_node shouldn't call kmalloc_index if the size was not
constant.
> void *kmalloc_node(size_t size, gfp_t flags, int node){
>
> if ( (__builtin_constant_p(
> !!(__builtin_constant_p(size)
> && size <= (1UL << ((11 + 12 - 1) <= 25 ? (11 + 12 - 1) :
25))))
> ? (!!(__builtin_constant_p(size) && size <= (1UL <<
((11 + 12 - 1) <= 25 ? (11 + 12 - 1) : 25))))
> if ( (__builtin_constant_p(!!((0 || 110000 >= 110000)
&& size_is_constant)) ? (!!((0 || 110000 >= 110000) &&
size_is_constant)) : ({ static struct ftrace_branch_data __attribute__((__aligned__(4)))
__attribute__((__section__("_ftrace_branch"))) __if_trace = { .func = __func__,
.file = "include/linux/slab.h", .line = 416, }; (!!((0 || 110000 >= 110000)
&& size_is_constant)) ? (__if_trace.miss_hit[1]++,1) :
(__if_trace.miss_hit[0]++,0); })) )
> do {
>
>
> extern void __compiletime_assert_131(void) ;
> // here - compiletime_assert_131 does NOTHING
It doesn't seem to do nothing? see below
> if ( (__builtin_constant_p(!!(!(!(1))))
> ? (!!(!(!(1))))
> : ({ static struct ftrace_branch_data __attribute__((__aligned__(4)))
__attribute__((__section__("_ftrace_branch"))) __if_trace = { .func = __func__,
.file = "include/linux/slab.h", .line = 417, }; (!!(!(!(1)))) ?
(__if_trace.miss_hit[1]++,1) : (__if_trace.miss_hit[0]++,0); })) )
__compiletime_assert_131(); } while (0);
The thing above seems to be exactly the "if (!(condition))
prefix ## suffix(); } while (0)" as the definition you posted below.
Anyway, you can verify that clang works by commenting out the highest size
checks and passing a constant size that makes it reach the BUILD_BUG_ON_MSG() ?
I verified as below.
clang didn't make an error, gcc worked as expected.
then I can explain why there is no error with clang:
it just did nothing with BUILD_BUG_ON.
hyeyoo@hyeyoo:~/바탕화면/linux-next$ git diff
diff --git a/include/linux/slab.h b/include/linux/slab.h
index 8d8dd8571261..f2f9a6a7e663 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -379,6 +379,9 @@ static __always_inline enum kmalloc_cache_type kmalloc_type(gfp_t
flags)
static __always_inline unsigned int __kmalloc_index(size_t size,
bool size_is_constant)
{
+
+ BUILD_BUG_ON(1);
+
if (!size)
return 0;
hyeyoo@hyeyoo:~/바탕화면/linux-next$ make mm/kfence/kfence_test.o CC=clang-11
CALL scripts/checksyscalls.sh
... some headers omitted ...
CC mm/kfence/kfence_test.o
In file included from <command-line>:
In function ‘__kmalloc_index’,
inlined from ‘kmalloc_cache_alignment’ at mm/kfence/kfence_test.c:200:50:
././include/linux/compiler_types.h:328:38: error: call to ‘__compiletime_assert_131’
declared with attribute error: BUILD_BUG_ON failed: 1
328 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
| ^
././include/linux/compiler_types.h:309:4: note: in definition of macro
‘__compiletime_assert’
309 | prefix ## suffix(); \
| ^~~~~~
././include/linux/compiler_types.h:328:2: note: in expansion of macro
‘_compiletime_assert’
328 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
| ^~~~~~~~~~~~~~~~~~~
./include/linux/build_bug.h:39:37: note: in expansion of macro ‘compiletime_assert’
39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
| ^~~~~~~~~~~~~~~~~~
./include/linux/build_bug.h:50:2: note: in expansion of macro ‘BUILD_BUG_ON_MSG’
50 | BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)
| ^~~~~~~~~~~~~~~~
./include/linux/slab.h:383:2: note: in expansion of macro ‘BUILD_BUG_ON’
383 | BUILD_BUG_ON(1);
| ^~~~~~~~~~~~
In function ‘__kmalloc_index’,
inlined from ‘test_alloc’ at mm/kfence/kfence_test.c:271:47:
././include/linux/compiler_types.h:328:38: error: call to ‘__compiletime_assert_131’
declared with attribute error: BUILD_BUG_ON failed: 1
328 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
| ^
././include/linux/compiler_types.h:309:4: note: in definition of macro
‘__compiletime_assert’
309 | prefix ## suffix(); \
| ^~~~~~
././include/linux/compiler_types.h:328:2: note: in expansion of macro
‘_compiletime_assert’
328 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
| ^~~~~~~~~~~~~~~~~~~
./include/linux/build_bug.h:39:37: note: in expansion of macro ‘compiletime_assert’
39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
| ^~~~~~~~~~~~~~~~~~
./include/linux/build_bug.h:50:2: note: in expansion of macro ‘BUILD_BUG_ON_MSG’
50 | BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)
| ^~~~~~~~~~~~~~~~
./include/linux/slab.h:383:2: note: in expansion of macro ‘BUILD_BUG_ON’
383 | BUILD_BUG_ON(1);
| ^~~~~~~~~~~~
make[2]: *** [scripts/Makefile.build:272: mm/kfence/kfence_test.o] 오류 1
make[1]: *** [scripts/Makefile.build:533: mm/kfence] 오류 2
make: *** [Makefile:1948: mm] 오류 2
4:50 a.m.
On Wed, Jun 09, 2021 at 03:45:01AM +0900, Hyeonggon Yoo wrote:
I verified as below.
clang didn't make an error, gcc worked as expected.
then I can explain why there is no error with clang:
it just did nothing with BUILD_BUG_ON.
Well I deleted output something too much... (making it confusing)
I send output without deleting part of it
hyeyoo@hyeyoo:~/바탕화면/linux-next$ git diff
diff --git a/include/linux/slab.h b/include/linux/slab.h
index 8d8dd8571261..f2f9a6a7e663 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -379,6 +379,9 @@ static __always_inline enum kmalloc_cache_type kmalloc_type(gfp_t
flags)
static __always_inline unsigned int __kmalloc_index(size_t size,
bool size_is_constant)
{
+
+ BUILD_BUG_ON(1);
+
if (!size)
return 0;
hyeyoo@hyeyoo:~/바탕화면/linux-next$ make mm/kfence/kfence_test.o
CALL scripts/checksyscalls.sh
CALL scripts/atomic/check-atomics.sh
DESCEND objtool
CC mm/kfence/kfence_test.o
In file included from <command-line>:
In function ‘__kmalloc_index’,
inlined from ‘kmalloc_cache_alignment’ at mm/kfence/kfence_test.c:200:50:
././include/linux/compiler_types.h:328:38: error: call to ‘__compiletime_assert_131’
declared with attribute error: BUILD_BUG_ON failed: 1
328 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
| ^
././include/linux/compiler_types.h:309:4: note: in definition of macro
‘__compiletime_assert’
309 | prefix ## suffix(); \
| ^~~~~~
././include/linux/compiler_types.h:328:2: note: in expansion of macro
‘_compiletime_assert’
328 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
| ^~~~~~~~~~~~~~~~~~~
./include/linux/build_bug.h:39:37: note: in expansion of macro ‘compiletime_assert’
39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
| ^~~~~~~~~~~~~~~~~~
./include/linux/build_bug.h:50:2: note: in expansion of macro ‘BUILD_BUG_ON_MSG’
50 | BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)
| ^~~~~~~~~~~~~~~~
./include/linux/slab.h:383:2: note: in expansion of macro ‘BUILD_BUG_ON’
383 | BUILD_BUG_ON(1);
| ^~~~~~~~~~~~
In function ‘__kmalloc_index’,
inlined from ‘test_alloc’ at mm/kfence/kfence_test.c:271:47:
././include/linux/compiler_types.h:328:38: error: call to ‘__compiletime_assert_131’
declared with attribute error: BUILD_BUG_ON failed: 1
328 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
| ^
././include/linux/compiler_types.h:309:4: note: in definition of macro
‘__compiletime_assert’
309 | prefix ## suffix(); \
| ^~~~~~
././include/linux/compiler_types.h:328:2: note: in expansion of macro
‘_compiletime_assert’
328 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
| ^~~~~~~~~~~~~~~~~~~
./include/linux/build_bug.h:39:37: note: in expansion of macro ‘compiletime_assert’
39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
| ^~~~~~~~~~~~~~~~~~
./include/linux/build_bug.h:50:2: note: in expansion of macro ‘BUILD_BUG_ON_MSG’
50 | BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)
| ^~~~~~~~~~~~~~~~
./include/linux/slab.h:383:2: note: in expansion of macro ‘BUILD_BUG_ON’
383 | BUILD_BUG_ON(1);
| ^~~~~~~~~~~~
make[2]: *** [scripts/Makefile.build:272: mm/kfence/kfence_test.o] 오류 1
make[1]: *** [scripts/Makefile.build:533: mm/kfence] 오류 2
make: *** [Makefile:1948: mm] 오류 2
hyeyoo@hyeyoo:~/바탕화면/linux-next$ make mm/kfence/kfence_test.o CC=clang-11
SYNC include/config/auto.conf.cmd
*
* Restart config...
*
*
* Memory initialization
*
Initialize kernel stack variables at function entry
1. no automatic initialization (weakest) (INIT_STACK_NONE)
2.
0xAA-init everything on the stack (strongest) (INIT_STACK_ALL_PATTERN) (NEW)
3. zero-init everything on the stack (strongest and safest) (INIT_STACK_ALL_ZERO) (NEW)
choice[1-3?]:
Enable heap memory zeroing on allocation by default (INIT_ON_ALLOC_DEFAULT_ON) [Y/n/?] y
Enable heap memory zeroing on free by default (INIT_ON_FREE_DEFAULT_ON) [N/y/?] n
*
* KCSAN: dynamic data race detector
*
KCSAN: dynamic data race detector (KCSAN) [N/y/?] (NEW)
CC scripts/mod/empty.o
MKELF scripts/mod/elfconfig.h
HOSTCC scripts/mod/modpost.o
CC scripts/mod/devicetable-offsets.s
HOSTCC scripts/mod/file2alias.o
HOSTCC scripts/mod/sumversion.o
HOSTLD scripts/mod/modpost
CC kernel/bounds.s
CC arch/x86/kernel/asm-offsets.s
CALL scripts/checksyscalls.sh
CALL scripts/atomic/check-atomics.sh
DESCEND objtool
CC /home/hyeyoo/바탕화면/linux-next/tools/objtool/arch/x86/special.o
CC /home/hyeyoo/바탕화면/linux-next/tools/objtool/arch/x86/decode.o
LD /home/hyeyoo/바탕화면/linux-next/tools/objtool/arch/x86/objtool-in.o
CC /home/hyeyoo/바탕화면/linux-next/tools/objtool/weak.o
CC /home/hyeyoo/바탕화면/linux-next/tools/objtool/check.o
CC /home/hyeyoo/바탕화면/linux-next/tools/objtool/special.o
CC /home/hyeyoo/바탕화면/linux-next/tools/objtool/orc_gen.o
CC /home/hyeyoo/바탕화면/linux-next/tools/objtool/orc_dump.o
CC /home/hyeyoo/바탕화면/linux-next/tools/objtool/builtin-check.o
CC /home/hyeyoo/바탕화면/linux-next/tools/objtool/builtin-orc.o
CC /home/hyeyoo/바탕화면/linux-next/tools/objtool/elf.o
CC /home/hyeyoo/바탕화면/linux-next/tools/objtool/objtool.o
CC /home/hyeyoo/바탕화면/linux-next/tools/objtool/libstring.o
CC /home/hyeyoo/바탕화면/linux-next/tools/objtool/libctype.o
CC /home/hyeyoo/바탕화면/linux-next/tools/objtool/str_error_r.o
CC /home/hyeyoo/바탕화면/linux-next/tools/objtool/librbtree.o
LD /home/hyeyoo/바탕화면/linux-next/tools/objtool/objtool-in.o
LINK /home/hyeyoo/바탕화면/linux-next/tools/objtool/objtool
CC mm/kfence/kfence_test.o
hyeyoo@hyeyoo:~/바탕화면/linux-next$
3:17 p.m.
New subject: Some logical errors on my words, but I still wonder...
On Wed, Jun 09, 2021 at 03:45:01AM +0900, Hyeonggon Yoo wrote:
static __always_inline void *kmalloc_node(size_t size, gfp_t flags,
int node)
{
if ( (
__builtin_constant_p(
!!(__builtin_constant_p(size) && size <= (1UL << ((11 + 12 - 1) <=
25 ? (11 + 12 - 1) : 25)))
)
? (!!(__builtin_constant_p(size) && size <= (1UL << ((11 + 12 - 1)
<= 25 ? (11 + 12 - 1) : 25))))
: ({ static struct ftrace_branch_data __attribute__((__aligned__(4)))
__attribute__((__section__("_ftrace_branch"))) __if_trace = { .func = __func__,
.file = "include/linux/slab.h", .line = 601, }; (!!(__builtin_constant_p(size)
&& size <= (1UL << ((11 + 12 - 1) <= 25 ? (11 + 12 - 1) : 25)))) ?
(__if_trace.miss_hit[1]++,1) : (__if_trace.miss_hit[0]++,0); })) )
{
unsigned int i = __kmalloc_index(size, true);
they are so ugly but the point is:
> > It seems that gcc evaluates
> >
> > __builtin_constant_p(
> > !!(builtin_constant_p(size)
> > && size <= KMALLOC_MAX_CACHE_SIZE)
> > )
> > as true.
> >
> > mm.. when the size passed to bpf_map_kmalloc_node is not constant,
> > (__builtin_constant_p(size) && size <= KMALLOC_MAX_CACHE_SIZE) is
false.
> > then __builtin_constant_p(!!false) is true. So it calls kmalloc_index.
> >
There were some logical errors on my words. I still think the compiler
evaluate it as true, but that is not reason why kmalloc_node calls
__kmalloc_index for non-constant size.
I wonder why kmalloc_node called __kmalloc_index(size, size_is_constant=true)
for non-constant size.
Thanks,
Hyeonggon
> > what change should be made?
> > just checking CONFIG_PROFILE_ALL_BRANCHES to kmalloc_index's if condition
> > doesn't make sense, because kmalloc_node is not working as expected.
> some evidence to add:
>
> there are 4 calls of bpf_map_kmalloc_node.
> if you comment out two calls of bpf_map_kmalloc_node with non-constant
> (in line 168, 508), it doesn't make an error. So I thought
> it was problem when non-constant size was passed.
>
> And if "kmalloc_index makes problem with non-constant size" is
> true, then it is caller's fault because it is not allowed (except
> in __kmalloc_index)
>
> kmalloc_node shouldn't call kmalloc_index if the size was not
> constant.
9:43 p.m.
On 6/8/21 8:45 PM, Hyeonggon Yoo wrote:
> > mm.. when the size passed to bpf_map_kmalloc_node is not
constant,
> > (__builtin_constant_p(size) && size <= KMALLOC_MAX_CACHE_SIZE) is
false.
> > then __builtin_constant_p(!!false) is true. So it calls kmalloc_index.
> >
> > what change should be made?
> > just checking CONFIG_PROFILE_ALL_BRANCHES to kmalloc_index's if condition
> > doesn't make sense, because kmalloc_node is not working as expected.
>
> If I understood my colleagues right, the problem is that, while kmalloc_index()
> seems to contains a number of *independent* "if (size <= X) conditions in a
> sequence, the machinery of CONFIG_PROFILE_ALL_BRANCHES turns it to a deeply
> nested if-then-else { if-then-else { if-then-else {...}}} thing (in fact using
> the ternary operators, not if-then-else).
> At some point of the deep nesting gcc
> "forgets" that __builtin_constant_p() is true and starts behaving as if it
wasn't.
>
> Without CONFIG_PROFILE_ALL_BRANCHES it's able to keep track of it fine.
>
I think you are talking about some if statements in kmalloc_index.
How do you know gcc "forgets" __builtin_constant_p() is true?
can it be debugged using cvise? (not offending, just because
I don't know about cvise yet).
Also, as I understand right, kmalloc_index doesn't have information
about the value of __builtin_constant_p(size). the caller of
kmalloc_index (kmalloc_node here) has that information.
If I understand right, what CONFIG_PROFILE_ALL_BRANCHES does is below.
replacing if(cond) { body } -> if (
__builtin_constant_p(cond) ?
then (cond)
else (cond, record something)
) { body }
I think it does not make deep nested statements.
OK, might have been a misunderstanding of cvise output.
So I don't know why exactly there are false positives with
CONFIG_PROFILE_ALL_BRANCHES.
the below is some part of preprocessor output.
CONFIG_PROFILE_ALL_BRANCHES makes some ugly ternary operators,
but there is no deep-nested if-then-else statements.
if ( (__builtin_constant_p(!!(size <= 8)) ? (!!(size <= 8)) : ({ static struct
ftrace_branch_data __attribute__((__aligned__(4)))
__attribute__((__section__("_ftrace_branch"))) __if_trace = { .func = __func__,
.file = "include/linux/slab.h", .line = 392, }; (!!(size <= 8)) ?
(__if_trace.miss_hit[1]++,1) : (__if_trace.miss_hit[0]++,0); })) ) return 3;
if ( (__builtin_constant_p(!!(size <= 16)) ? (!!(size <= 16)) : ({ static struct
ftrace_branch_data __attribute__((__aligned__(4)))
__attribute__((__section__("_ftrace_branch"))) __if_trace = { .func = __func__,
.file = "include/linux/slab.h", .line = 393, }; (!!(size <= 16)) ?
(__if_trace.miss_hit[1]++,1) : (__if_trace.miss_hit[0]++,0); })) ) return 4;
if ( (__builtin_constant_p(!!(size <= 32)) ? (!!(size <= 32)) : ({ static struct
ftrace_branch_data __attribute__((__aligned__(4)))
__attribute__((__section__("_ftrace_branch"))) __if_trace = { .func = __func__,
.file = "include/linux/slab.h", .line = 394, }; (!!(size <= 32)) ?
(__if_trace.miss_hit[1]++,1) : (__if_trace.miss_hit[0]++,0); })) ) return 5;
.... and some if statements ...
And I think, the problem is on kmalloc_node, not on kmalloc_index.
the original code of kmalloc_node is below:
static __always_inline void *kmalloc_node(size_t size, gfp_t flags, int node)
{
#ifndef CONFIG_SLOB
if (__builtin_constant_p(size) &&
size <= KMALLOC_MAX_CACHE_SIZE) {
unsigned int i = kmalloc_index(size);
and which is changed to below: (by CONFIG_PROFILE_ALL_BRANCHES)
static __always_inline void *kmalloc_node(size_t size, gfp_t flags, int node)
{
if ( (
__builtin_constant_p(
!!(__builtin_constant_p(size) && size <= (1UL << ((11 + 12 - 1) <=
25 ? (11 + 12 - 1) : 25)))
)
? (!!(__builtin_constant_p(size) && size <= (1UL << ((11 + 12 - 1)
<= 25 ? (11 + 12 - 1) : 25))))
: ({ static struct ftrace_branch_data __attribute__((__aligned__(4)))
__attribute__((__section__("_ftrace_branch"))) __if_trace = { .func = __func__,
.file = "include/linux/slab.h", .line = 601, }; (!!(__builtin_constant_p(size)
&& size <= (1UL << ((11 + 12 - 1) <= 25 ? (11 + 12 - 1) : 25)))) ?
(__if_trace.miss_hit[1]++,1) : (__if_trace.miss_hit[0]++,0); })) )
{
unsigned int i = __kmalloc_index(size, true);
they are so ugly but the point is:
> > It seems that gcc evaluates
> >
> > __builtin_constant_p(
> > !!(builtin_constant_p(size)
> > && size <= KMALLOC_MAX_CACHE_SIZE)
> > )
> > as true.
> >
> > mm.. when the size passed to bpf_map_kmalloc_node is not constant,
> > (__builtin_constant_p(size) && size <= KMALLOC_MAX_CACHE_SIZE) is
false.
> > then __builtin_constant_p(!!false) is true. So it calls kmalloc_index.
> >
> > what change should be made?
> > just checking CONFIG_PROFILE_ALL_BRANCHES to kmalloc_index's if condition
> > doesn't make sense, because kmalloc_node is not working as expected.
some evidence to add:
there are 4 calls of bpf_map_kmalloc_node.
if you comment out two calls of bpf_map_kmalloc_node with non-constant
(in line 168, 508), it doesn't make an error. So I thought
it was problem when non-constant size was passed.
And if "kmalloc_index makes problem with non-constant size" is
true, then it is caller's fault because it is not allowed (except
in __kmalloc_index)
kmalloc_node shouldn't call kmalloc_index if the size was not
constant.
Yes. You could perhaps exclude CONFIG_PROFILE_ALL_BRANCHES for now, and fill
official gcc bugzilla with the preprocessed file.
Bonus points if you can use cvise in a way that reduces the testcase but does
not remove any of the important parts. All we could get were degenerate cases
like this one
https://paste.opensuse.org/9320186
> > void *kmalloc_node(size_t size, gfp_t flags, int node){
> >
> > if ( (__builtin_constant_p(
> > !!(__builtin_constant_p(size)
> > && size <= (1UL << ((11 + 12 - 1) <= 25 ? (11 + 12 -
1) : 25))))
> > ? (!!(__builtin_constant_p(size) && size <= (1UL <<
((11 + 12 - 1) <= 25 ? (11 + 12 - 1) : 25))))
> > if ( (__builtin_constant_p(!!((0 || 110000 >= 110000) &&
size_is_constant)) ? (!!((0 || 110000 >= 110000) && size_is_constant)) : ({
static struct ftrace_branch_data __attribute__((__aligned__(4)))
__attribute__((__section__("_ftrace_branch"))) __if_trace = { .func = __func__,
.file = "include/linux/slab.h", .line = 416, }; (!!((0 || 110000 >= 110000)
&& size_is_constant)) ? (__if_trace.miss_hit[1]++,1) :
(__if_trace.miss_hit[0]++,0); })) )
> > do {
> >
> >
> > extern void __compiletime_assert_131(void) ;
> > // here - compiletime_assert_131 does NOTHING
>
> It doesn't seem to do nothing? see below
>
> > if ( (__builtin_constant_p(!!(!(!(1))))
> > ? (!!(!(!(1))))
> > : ({ static struct ftrace_branch_data
__attribute__((__aligned__(4))) __attribute__((__section__("_ftrace_branch")))
__if_trace = { .func = __func__, .file = "include/linux/slab.h", .line = 417, };
(!!(!(!(1)))) ? (__if_trace.miss_hit[1]++,1) : (__if_trace.miss_hit[0]++,0); })) )
__compiletime_assert_131(); } while (0);
>
> The thing above seems to be exactly the "if (!(condition))
> prefix ## suffix(); } while (0)" as the definition you posted below.
>
> Anyway, you can verify that clang works by commenting out the highest size
> checks and passing a constant size that makes it reach the BUILD_BUG_ON_MSG() ?
>
I verified as below.
clang didn't make an error, gcc worked as expected.
then I can explain why there is no error with clang:
it just did nothing with BUILD_BUG_ON.
That might warrant a clang bug report too. We excluded clang 10 due to false
positives. If the BUILD_BUG_ON doesn't work at all in clang 11 we might have to
exclude clang completely.
hyeyoo@hyeyoo:~/바탕화면/linux-next$ git diff
diff --git a/include/linux/slab.h b/include/linux/slab.h
index 8d8dd8571261..f2f9a6a7e663 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -379,6 +379,9 @@ static __always_inline enum kmalloc_cache_type kmalloc_type(gfp_t
flags)
static __always_inline unsigned int __kmalloc_index(size_t size,
bool size_is_constant)
{
+
+ BUILD_BUG_ON(1);
+
if (!size)
return 0;
hyeyoo@hyeyoo:~/바탕화면/linux-next$ make mm/kfence/kfence_test.o CC=clang-11
CALL scripts/checksyscalls.sh
... some headers omitted ...
CC mm/kfence/kfence_test.o
In file included from <command-line>:
In function ‘__kmalloc_index’,
inlined from ‘kmalloc_cache_alignment’ at mm/kfence/kfence_test.c:200:50:
././include/linux/compiler_types.h:328:38: error: call to ‘__compiletime_assert_131’
declared with attribute error: BUILD_BUG_ON failed: 1
328 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
| ^
././include/linux/compiler_types.h:309:4: note: in definition of macro
‘__compiletime_assert’
309 | prefix ## suffix(); \
| ^~~~~~
././include/linux/compiler_types.h:328:2: note: in expansion of macro
‘_compiletime_assert’
328 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
| ^~~~~~~~~~~~~~~~~~~
./include/linux/build_bug.h:39:37: note: in expansion of macro ‘compiletime_assert’
39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
| ^~~~~~~~~~~~~~~~~~
./include/linux/build_bug.h:50:2: note: in expansion of macro ‘BUILD_BUG_ON_MSG’
50 | BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)
| ^~~~~~~~~~~~~~~~
./include/linux/slab.h:383:2: note: in expansion of macro ‘BUILD_BUG_ON’
383 | BUILD_BUG_ON(1);
| ^~~~~~~~~~~~
In function ‘__kmalloc_index’,
inlined from ‘test_alloc’ at mm/kfence/kfence_test.c:271:47:
././include/linux/compiler_types.h:328:38: error: call to ‘__compiletime_assert_131’
declared with attribute error: BUILD_BUG_ON failed: 1
328 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
| ^
././include/linux/compiler_types.h:309:4: note: in definition of macro
‘__compiletime_assert’
309 | prefix ## suffix(); \
| ^~~~~~
././include/linux/compiler_types.h:328:2: note: in expansion of macro
‘_compiletime_assert’
328 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
| ^~~~~~~~~~~~~~~~~~~
./include/linux/build_bug.h:39:37: note: in expansion of macro ‘compiletime_assert’
39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
| ^~~~~~~~~~~~~~~~~~
./include/linux/build_bug.h:50:2: note: in expansion of macro ‘BUILD_BUG_ON_MSG’
50 | BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)
| ^~~~~~~~~~~~~~~~
./include/linux/slab.h:383:2: note: in expansion of macro ‘BUILD_BUG_ON’
383 | BUILD_BUG_ON(1);
| ^~~~~~~~~~~~
make[2]: *** [scripts/Makefile.build:272: mm/kfence/kfence_test.o] 오류 1
make[1]: *** [scripts/Makefile.build:533: mm/kfence] 오류 2
make: *** [Makefile:1948: mm] 오류 2
6:58 p.m.
On Thu, Jun 10, 2021, 8:43 PM Vlastimil Babka <vbabka(a)suse.cz> wrote:
> > void *kmalloc_node(size_t size, gfp_t flags, int node){
> >
> > if ( (__builtin_constant_p(
> > !!(__builtin_constant_p(size)
> > && size <= (1UL << ((11 + 12 - 1) <= 25 ? (11 + 12 -
1) : 25))))
> > ? (!!(__builtin_constant_p(size) && size <= (1UL <<
((11 +
12 - 1) <= 25 ? (11 + 12 - 1) : 25))))
> > if ( (__builtin_constant_p(!!((0 || 110000 >= 110000) &&
size_is_constant)) ? (!!((0 || 110000 >= 110000) && size_is_constant)) :
({
static struct ftrace_branch_data __attribute__((__aligned__(4)))
__attribute__((__section__("_ftrace_branch"))) __if_trace = { .func =
__func__, .file = "include/linux/slab.h", .line = 416, }; (!!((0 || 110000
= 110000) && size_is_constant)) ? (__if_trace.miss_hit[1]++,1)
:
(__if_trace.miss_hit[0]++,0); })) )
> > do {
> >
> >
> > extern void __compiletime_assert_131(void) ;
> > // here - compiletime_assert_131 does NOTHING
>
> It doesn't seem to do nothing? see below
>
> > if ( (__builtin_constant_p(!!(!(!(1))))
> > ? (!!(!(!(1))))
> > : ({ static struct ftrace_branch_data
__attribute__((__aligned__(4)))
__attribute__((__section__("_ftrace_branch"))) __if_trace = { .func =
__func__, .file = "include/linux/slab.h", .line = 417, }; (!!(!(!(1)))) ?
(__if_trace.miss_hit[1]++,1) : (__if_trace.miss_hit[0]++,0); })) )
__compiletime_assert_131(); } while (0);
>
> The thing above seems to be exactly the "if (!(condition))
> prefix ## suffix(); } while (0)" as the definition you posted below.
>
> Anyway, you can verify that clang works by commenting out the highest
size
> checks and passing a constant size that makes it reach the
BUILD_BUG_ON_MSG() ?
>
I verified as below.
clang didn't make an error, gcc worked as expected.
then I can explain why there is no error with clang:
it just did nothing with BUILD_BUG_ON.
That might warrant a clang bug report too. We excluded clang 10 due to false
positives. If the BUILD_BUG_ON doesn't work at all in clang 11 we might
have to
exclude clang completely.
After playing with clang more a bit, I got to know that compiletime_assert
makes weird link error (undefined reference to compiletime_assert_XXX), Not
a compile error.
I think it's time to CC ClangBuiltLinux maintainers, who work on clang/llvm
build support.
[+CC Nathan and Nick]
I assumeed that compiletime_assert (in linux/compiler.h) will make compiler
error, but it makes no compile error, just makes weird link error.
I'm not sure it it works well with clang, or somewhat buggy status?
diff --git a/include/linux/slab.h b/include/linux/slab.h
index 8d8dd8571261..f2f9a6a7e663 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -379,6 +379,9 @@ static __always_inline enum kmalloc_cache_type
kmalloc_type(gfp_t flags)
static __always_inline unsigned int __kmalloc_index(size_t size,
bool size_is_constant)
{
+
+ BUILD_BUG_ON(1);
+
if (!size)
return 0;
On 6/8/21 8:45 PM, Hyeonggon Yoo wrote:
>> > mm.. when the size passed to bpf_map_kmalloc_node is
not constant,
>> > (__builtin_constant_p(size) && size <= KMALLOC_MAX_CACHE_SIZE)
is
false.
>> > then __builtin_constant_p(!!false) is true. So it calls kmalloc_index.
>> >
>> > what change should be made?
>> > just checking CONFIG_PROFILE_ALL_BRANCHES to kmalloc_index's if
condition
>> > doesn't make sense, because kmalloc_node is not working as expected.
>>
>> If I understood my colleagues right, the problem is that, while
kmalloc_index()
>> seems to contains a number of *independent* "if (size <= X) conditions
in a
>> sequence, the machinery of CONFIG_PROFILE_ALL_BRANCHES turns it to a
deeply
>> nested if-then-else { if-then-else { if-then-else {...}}} thing (in
fact using
>> the ternary operators, not if-then-else).
>> At some point of the deep nesting gcc
>> "forgets" that __builtin_constant_p() is true and starts behaving as
if
it wasn't.
>>
>> Without CONFIG_PROFILE_ALL_BRANCHES it's able to keep track of it fine.
>>
>
> I think you are talking about some if statements in kmalloc_index.
>
> How do you know gcc "forgets" __builtin_constant_p() is true?
> can it be debugged using cvise? (not offending, just because
> I don't know about cvise yet).
>
> Also, as I understand right, kmalloc_index doesn't have information
> about the value of __builtin_constant_p(size). the caller of
> kmalloc_index (kmalloc_node here) has that information.
>
> If I understand right, what CONFIG_PROFILE_ALL_BRANCHES does is below.
>
> replacing if(cond) { body } -> if (
> __builtin_constant_p(cond)
?
> then (cond)
> else (cond, record
something)
> ) { body }
>
> I think it does not make deep nested statements.
OK, might have been a misunderstanding of cvise output.
So I don't know why exactly there are false positives with
CONFIG_PROFILE_ALL_BRANCHES.
> the below is some part of preprocessor output.
> CONFIG_PROFILE_ALL_BRANCHES makes some ugly ternary operators,
> but there is no deep-nested if-then-else statements.
>
> if ( (__builtin_constant_p(!!(size <= 8)) ? (!!(size <= 8)) : ({ static
struct ftrace_branch_data __attribute__((__aligned__(4)))
__attribute__((__section__("_ftrace_branch"))) __if_trace = { .func =
__func__, .file = "include/linux/slab.h", .line = 392, }; (!!(size <= 8)) ?
(__if_trace.miss_hit[1]++,1) : (__if_trace.miss_hit[0]++,0); })) ) return 3;
>
> if ( (__builtin_constant_p(!!(size <= 16)) ? (!!(size <= 16)) : ({
static struct ftrace_branch_data __attribute__((__aligned__(4)))
__attribute__((__section__("_ftrace_branch"))) __if_trace = { .func =
__func__, .file = "include/linux/slab.h", .line = 393, }; (!!(size <= 16))
? (__if_trace.miss_hit[1]++,1) : (__if_trace.miss_hit[0]++,0); })) ) return
4;
>
> if ( (__builtin_constant_p(!!(size <= 32)) ? (!!(size <= 32)) : ({
static struct ftrace_branch_data __attribute__((__aligned__(4)))
__attribute__((__section__("_ftrace_branch"))) __if_trace = { .func =
__func__, .file = "include/linux/slab.h", .line = 394, }; (!!(size <= 32))
? (__if_trace.miss_hit[1]++,1) : (__if_trace.miss_hit[0]++,0); })) ) return
5;
>
> .... and some if statements ...
>
> And I think, the problem is on kmalloc_node, not on kmalloc_index.
> the original code of kmalloc_node is below:
>
>
> static __always_inline void *kmalloc_node(size_t size, gfp_t flags, int
node)
> {
> #ifndef CONFIG_SLOB
> if (__builtin_constant_p(size) &&
> size <= KMALLOC_MAX_CACHE_SIZE) {
> unsigned int i = kmalloc_index(size);
>
> and which is changed to below: (by CONFIG_PROFILE_ALL_BRANCHES)
>
> static __always_inline void *kmalloc_node(size_t size, gfp_t flags, int
node)
> {
>
> if ( (
> __builtin_constant_p(
> !!(__builtin_constant_p(size) && size <= (1UL << ((11
+ 12
- 1) <= 25 ? (11 + 12 - 1) : 25)))
> )
> ? (!!(__builtin_constant_p(size) && size <= (1UL
<< ((11 + 12 - 1) <= 25 ? (11 + 12 - 1) : 25))))
> : ({ static struct ftrace_branch_data
__attribute__((__aligned__(4)))
__attribute__((__section__("_ftrace_branch"))) __if_trace = { .func =
__func__, .file = "include/linux/slab.h", .line = 601, };
(!!(__builtin_constant_p(size) && size <= (1UL << ((11 + 12 - 1) <=
25 ?
(11 + 12 - 1) : 25)))) ? (__if_trace.miss_hit[1]++,1) :
(__if_trace.miss_hit[0]++,0); })) )
> {
> unsigned int i = __kmalloc_index(size, true);
>
>
> they are so ugly but the point is:
>
>> > It seems that gcc evaluates
>> >
>> > __builtin_constant_p(
>> > !!(builtin_constant_p(size)
>> > && size <= KMALLOC_MAX_CACHE_SIZE)
>> > )
>> > as true.
>> >
>> > mm.. when the size passed to bpf_map_kmalloc_node is not constant,
>> > (__builtin_constant_p(size) && size <= KMALLOC_MAX_CACHE_SIZE)
is
false.
>> > then __builtin_constant_p(!!false) is true. So it calls kmalloc_index.
>> >
>> > what change should be made?
>> > just checking CONFIG_PROFILE_ALL_BRANCHES to kmalloc_index's if
condition
>> > doesn't make sense, because kmalloc_node is not working as expected.
>
> some evidence to add:
>
> there are 4 calls of bpf_map_kmalloc_node.
> if you comment out two calls of bpf_map_kmalloc_node with
non-constant
> (in line 168, 508), it doesn't make an error. So I thought
> it was problem when non-constant size was passed.
>
> And if "kmalloc_index makes problem with non-constant size" is
> true, then it is caller's fault because it is not allowed (except
> in __kmalloc_index)
>
> kmalloc_node shouldn't call kmalloc_index if the size was not
> constant.
Yes. You could perhaps exclude CONFIG_PROFILE_ALL_BRANCHES for now,
You do you mean by "exclude CONFIG_PROFILE_ALL_BRANCHES for now"?
Did you mean we don't need to fix it for now? (Anyway I have no idea what
the fix patch would be for now...)
and fill
official gcc bugzilla with the preprocessed file.
Bonus points if you can use cvise in a way that reduces the testcase but
does
not remove any of the important parts. All we could get were degenerate
cases
like this one
https://paste.opensuse.org/9320186
I'll try that and report it to gcc bugzilla soon!
I think I should let you know that I'm going to korean army next week, So I
can't access internet for next two months... (Because they don't allow
internet in military training period)
I'll be back after 2 months, But can I ask you (Vlastimil) to write follow
up patch if we can fix it before I'm back?
Sorry I didn't know compiler will bother us at that time I wrote the patch
:(
8:27 p.m.
On 6/11/21 10:58 AM, Hyeonggon Yoo wrote:
On Thu, Jun 10, 2021, 8:43 PM Vlastimil Babka <vbabka(a)suse.cz
<mailto:vbabka@suse.cz>> wrote:
>> > void *kmalloc_node(size_t size, gfp_t flags, int node){
>> >
>> > if ( (__builtin_constant_p(
>> > !!(__builtin_constant_p(size)
>> > && size <= (1UL << ((11 + 12 - 1) <= 25 ?
(11 + 12 - 1) : 25))))
>> > ? (!!(__builtin_constant_p(size) && size <= (1UL
<< ((11 +
12 - 1) <= 25 ? (11 + 12 - 1) : 25))))
>
>
>> > if ( (__builtin_constant_p(!!((0 || 110000 >= 110000) &&
size_is_constant)) ? (!!((0 || 110000 >= 110000) && size_is_constant)) :
({
static struct ftrace_branch_data __attribute__((__aligned__(4)))
__attribute__((__section__("_ftrace_branch"))) __if_trace = { .func =
__func__, .file = "include/linux/slab.h", .line = 416, }; (!!((0 || 110000
>= 110000) && size_is_constant)) ? (__if_trace.miss_hit[1]++,1) :
(__if_trace.miss_hit[0]++,0); })) )
>> > do {
>> >
>> >
>> > extern void __compiletime_assert_131(void) ;
>> > // here - compiletime_assert_131 does NOTHING
>>
>> It doesn't seem to do nothing? see below
>>
>> > if ( (__builtin_constant_p(!!(!(!(1))))
>> > ? (!!(!(!(1))))
>> > : ({ static struct ftrace_branch_data
__attribute__((__aligned__(4)))
__attribute__((__section__("_ftrace_branch"))) __if_trace = { .func =
__func__, .file = "include/linux/slab.h", .line = 417, }; (!!(!(!(1)))) ?
(__if_trace.miss_hit[1]++,1) : (__if_trace.miss_hit[0]++,0); })) )
__compiletime_assert_131(); } while (0);
>>
>> The thing above seems to be exactly the "if (!(condition))
>> prefix ## suffix(); } while (0)" as the definition you posted below.
>>
>> Anyway, you can verify that clang works by commenting out the highest size
>> checks and passing a constant size that makes it reach the
BUILD_BUG_ON_MSG() ?
>>
>
> I verified as below.
> clang didn't make an error, gcc worked as expected.
>
> then I can explain why there is no error with clang:
> it just did nothing with BUILD_BUG_ON.
That might warrant a clang bug report too. We excluded clang 10 due to false
positives. If the BUILD_BUG_ON doesn't work at all in clang 11 we might have to
exclude clang completely.
After playing with clang more a bit, I got to know that compiletime_assert makes
weird link error (undefined reference to compiletime_assert_XXX), Not a compile
error.
Ah well that's much better than no error at all.
I think it's time to CC ClangBuiltLinux maintainers, who work on
clang/llvm
build support.
[+CC Nathan and Nick]
Good.
I assumeed that compiletime_assert (in linux/compiler.h) will make
compiler
error, but it makes no compile error, just makes weird link error.
I'm not sure it it works well with clang, or somewhat buggy status?
...
Yes. You could perhaps exclude CONFIG_PROFILE_ALL_BRANCHES for
now,
You do you mean by "exclude CONFIG_PROFILE_ALL_BRANCHES for now"?
Did you mean we don't need to fix it for now? (Anyway I have no idea what the
fix patch would be for now...)
I meant the the condition to use BUILD_BUG_ON instead of BUG_ON would include
!IS_ENABLED(CONFIG_PROFILE_ALL_BRANCHES)
and fill
official gcc bugzilla with the preprocessed file.
Bonus points if you can use cvise in a way that reduces the testcase but does
not remove any of the important parts. All we could get were degenerate cases
like this one
https://paste.opensuse.org/9320186 <https://paste.opensuse.org/9320186>
I'll try that and report it to gcc bugzilla soon!
I think I should let you know that I'm going to korean army next week, So I
can't access internet for next two months... (Because they don't allow internet
in military training period)
I'll be back after 2 months, But can I ask you (Vlastimil) to write follow up
patch if we can fix it before I'm back?
Sure, take care!
Sorry I didn't know compiler will bother us at that time I wrote
the patch :(
9:56 p.m.
On Fri, Jun 11, 2021, 7:27 PM Vlastimil Babka <vbabka(a)suse.cz> wrote:
On 6/11/21 10:58 AM, Hyeonggon Yoo wrote:
> On Thu, Jun 10, 2021, 8:43 PM Vlastimil Babka <vbabka(a)suse.cz
> <mailto:vbabka@suse.cz>> wrote:
>
>
> >> > void *kmalloc_node(size_t size, gfp_t flags, int node){
> >> >
> >> > if ( (__builtin_constant_p(
> >> > !!(__builtin_constant_p(size)
> >> > && size <= (1UL << ((11 + 12 - 1) <= 25
? (11 + 12 - 1)
: 25))))
> >> > ? (!!(__builtin_constant_p(size) && size <=
(1UL <<
((11 +
> 12 - 1) <= 25 ? (11 + 12 - 1) : 25))))
> >
> >
> >> > if ( (__builtin_constant_p(!!((0 || 110000 >= 110000)
&&
> size_is_constant)) ? (!!((0 || 110000 >= 110000) &&
size_is_constant)) : ({
> static struct ftrace_branch_data __attribute__((__aligned__(4)))
> __attribute__((__section__("_ftrace_branch"))) __if_trace = { .func =
> __func__, .file = "include/linux/slab.h", .line = 416, }; (!!((0 ||
110000
> >= 110000) && size_is_constant)) ? (__if_trace.miss_hit[1]++,1) :
> (__if_trace.miss_hit[0]++,0); })) )
> >> > do {
> >> >
> >> >
> >> > extern void __compiletime_assert_131(void) ;
> >> > // here - compiletime_assert_131 does NOTHING
> >>
> >> It doesn't seem to do nothing? see below
> >>
> >> > if ( (__builtin_constant_p(!!(!(!(1))))
> >> > ? (!!(!(!(1))))
> >> > : ({ static struct ftrace_branch_data
> __attribute__((__aligned__(4)))
> __attribute__((__section__("_ftrace_branch"))) __if_trace = { .func =
> __func__, .file = "include/linux/slab.h", .line = 417, };
(!!(!(!(1)))) ?
> (__if_trace.miss_hit[1]++,1) : (__if_trace.miss_hit[0]++,0); })) )
> __compiletime_assert_131(); } while (0);
> >>
> >> The thing above seems to be exactly the "if (!(condition))
> >> prefix ## suffix(); } while (0)" as the definition you posted
below.
> >>
> >> Anyway, you can verify that clang works by commenting out the
highest size
> >> checks and passing a constant size that makes it reach the
> BUILD_BUG_ON_MSG() ?
> >>
> >
> > I verified as below.
> > clang didn't make an error, gcc worked as expected.
> >
> > then I can explain why there is no error with clang:
> > it just did nothing with BUILD_BUG_ON.
>
> That might warrant a clang bug report too. We excluded clang 10 due
to false
> positives. If the BUILD_BUG_ON doesn't work at all in clang 11 we
might have to
> exclude clang completely.
>
>
> After playing with clang more a bit, I got to know that
compiletime_assert makes
> weird link error (undefined reference to compiletime_assert_XXX), Not a
compile
> error.
Ah well that's much better than no error at all.
> I think it's time to CC ClangBuiltLinux maintainers, who work on
clang/llvm
> build support.
>
> [+CC Nathan and Nick]
Good.
Forgot to link our previous work:
https://lore.kernel.org/lkml/20210515210950.GA52841@hyeyoo/
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org/thread/45UNT...
> I assumeed that compiletime_assert (in linux/compiler.h) will
make
compiler
> error, but it makes no compile error, just makes weird link error.
>
> I'm not sure it it works well with clang, or somewhat buggy status?
>
>
>
...
...?
> Yes. You could perhaps exclude CONFIG_PROFILE_ALL_BRANCHES for now,
>
>
> You do you mean by "exclude CONFIG_PROFILE_ALL_BRANCHES for now"?
>
> Did you mean we don't need to fix it for now? (Anyway I have no idea
what the
> fix patch would be for now...)
I meant the the condition to use BUILD_BUG_ON instead of BUG_ON would
include
!IS_ENABLED(CONFIG_PROFILE_ALL_BRANCHES)
You mean this? (This will make kmalloc_index return -1 without BUG())
if ((IS_ENABLED(CONFIG_CC_IS_GCC) || CONFIG_CLANG_VERSION >= 110000) &&
size_is_constant)
BUILD_BUG_ON_MSG(!IS_ENABLED(CONFIG_PROFILE_ALL_BRANCHES),
"unexpected size in kmalloc_index()");
else
BUG();
Or This?
if ((IS_ENABLED(CONFIG_CC_IS_GCC) || CONFIG_CLANG_VERSION >= 110000) &&
size_is_constant && !IS_ENABLED(CONFIG_PROFILE_ALL_BRANCHES))
BUILD_BUG_ON_MSG(1, "unexpected size in kmalloc_index()");
else
BUG();
Maybe this version seems better
But little bit worried :(
The code is getting too complicated...
How do you think?
> and fill
> official gcc bugzilla with the preprocessed file.
> Bonus points if you can use cvise in a way that reduces the testcase
but does
> not remove any of the important parts. All we could get were
degenerate cases
> like this one
> https://paste.opensuse.org/9320186 <
https://paste.opensuse.org/9320186>
>
>
> I'll try that and report it to gcc bugzilla soon!
>
> I think I should let you know that I'm going to korean army next week,
So I
> can't access internet for next two months... (Because they don't allow
internet
> in military training period)
>
> I'll be back after 2 months, But can I ask you (Vlastimil) to write
follow up
> patch if we can fix it before I'm back?
Sure, take care!
Thank you a lot!
> Sorry I didn't know compiler will bother us at that time I wrote the
patch :(
>
2:56 a.m.
On 6/11/2021 1:58 AM, Hyeonggon Yoo wrote:> After playing with clang
more a bit, I got to know that
compiletime_assert makes weird link error (undefined reference to
compiletime_assert_XXX), Not a compile error.
I think it's time to CC ClangBuiltLinux maintainers, who work on
clang/llvm build support.
[+CC Nathan and Nick]
I assumeed that compiletime_assert (in linux/compiler.h) will make
compiler error, but it makes no compile error, just makes weird link error.
I'm not sure it it works well with clang, or somewhat buggy status?
I am guessing this alone is why we were keyed into the thread so I am
just going to respond to this.
Unfortunately, this is a known issue with clang:
https://github.com/ClangBuiltLinux/linux/issues/1173
https://bugs.llvm.org/show_bug.cgi?id=16428
As you noticed, building the full kernel will result in a link error but
it would certainly be nicer if it were a compiler error. Something for
us to improve indeed but I am not sure when we will be able to allocate
resources for that. Until then, you can build a full kernel to get the
failing translation unit then use nm or readelf when building the single
translation unit to see if there are any "__compiletime_assert" symbols.
Cheers,
Nathan
9:59 a.m.
On 6/11/21 1:56 PM, Hyeonggon Yoo wrote:
On Fri, Jun 11, 2021, 7:27 PM Vlastimil Babka <vbabka(a)suse.cz
<mailto:vbabka@suse.cz>> wrote:
I meant the the condition to use BUILD_BUG_ON instead of BUG_ON would include
!IS_ENABLED(CONFIG_PROFILE_ALL_BRANCHES)
You mean this? (This will make kmalloc_index return -1 without BUG())
if ((IS_ENABLED(CONFIG_CC_IS_GCC) || CONFIG_CLANG_VERSION >= 110000) &&
size_is_constant)
BUILD_BUG_ON_MSG(!IS_ENABLED(CONFIG_PROFILE_ALL_BRANCHES), "unexpected
size in kmalloc_index()");
else
BUG();
No,
Or This?
if ((IS_ENABLED(CONFIG_CC_IS_GCC) || CONFIG_CLANG_VERSION >= 110000) &&
size_is_constant && !IS_ENABLED(CONFIG_PROFILE_ALL_BRANCHES))
BUILD_BUG_ON_MSG(1, "unexpected size in kmalloc_index()");
else
BUG();
Maybe this version seems better
Yeah, meant that.
But little bit worried :(
The code is getting too complicated...
How do you think?
Yeah, I expected that problems like this could occur as we're poking at some
rare corner cases of compiler implementations here. But if that leads to fixes
in compilers, good for everyone I'd say.
So I would try this, even if it becomes complicated.
10:19 a.m.
On Sat, Jun 12, 2021, 8:59 AM Vlastimil Babka <vbabka(a)suse.cz> wrote:
Okay, I see the code is okay, but one thing to suggest:
The problem already existed in kmalloc_node before the patch And we
found it by adding BUILD_BUG_ON in kmalloc_index.
So I suggest adding the condition in kmalloc_node. How about this?
On 6/11/21 1:56 PM, Hyeonggon Yoo wrote:
> On Fri, Jun 11, 2021, 7:27 PM Vlastimil Babka <vbabka(a)suse.cz
> <mailto:vbabka@suse.cz>> wrote:
> I meant the the condition to use BUILD_BUG_ON instead of BUG_ON
would include
> !IS_ENABLED(CONFIG_PROFILE_ALL_BRANCHES)
> You mean this? (This will make
kmalloc_index return -1 without BUG())
> if ((IS_ENABLED(CONFIG_CC_IS_GCC) || CONFIG_CLANG_VERSION
>= 110000) &&
> size_is_constant)
> BUILD_BUG_ON_MSG(!IS_ENABLED(CONFIG_PROFILE_ALL_BRANCHES),
"unexpected
> size in kmalloc_index()");
> else
> BUG();
No,
> Or This?
> if ((IS_ENABLED(CONFIG_CC_IS_GCC) || CONFIG_CLANG_VERSION
>= 110000) &&
> size_is_constant && !IS_ENABLED(CONFIG_PROFILE_ALL_BRANCHES))
> BUILD_BUG_ON_MSG(1, "unexpected size in kmalloc_index()");
> else
> BUG();
> Maybe this version seems better
Yeah, meant that.
Ah, okay!
> But little bit worried :(
> The code is getting too complicated...
> How do you think?
Yeah, I expected that problems like this could occur as we're poking at
some
rare corner cases of compiler implementations here. But if that leads to
fixes
in compilers, good for everyone I'd say.
So I would try this, even if it becomes complicated.
10:31 a.m.
On Sat, Jun 12, 2021, 1:56 AM Nathan Chancellor <nathan(a)kernel.org> wrote:
On 6/11/2021 1:58 AM, Hyeonggon Yoo wrote:> After playing with
clang
more a bit, I got to know that
> compiletime_assert makes weird link error (undefined reference to
> compiletime_assert_XXX), Not a compile error.
>
>
> I think it's time to CC ClangBuiltLinux maintainers, who work on
> clang/llvm build support.
>
> [+CC Nathan and Nick]
>
> I assumeed that compiletime_assert (in linux/compiler.h) will make
> compiler error, but it makes no compile error, just makes weird link
error.
>
> I'm not sure it it works well with clang, or somewhat buggy status?
I am guessing this alone is why we were keyed into the thread so I am
just going to respond to this.
Thank you for quick reply!
Unfortunately, this is a known issue with clang:
As you noticed, building the full kernel will result in a link error
but
it would certainly be nicer if it were a compiler error. Something for
us to improve indeed but I am not sure when we will be able to allocate
resources for that.
I wanted to be sure if we can use compiletime_assert for clang. Then there
is room for improvement, but it seems okay.
Until then, you can build a full kernel to get the
failing translation unit then use nm or readelf when building the
single
translation unit to see if there are any "__compiletime_assert" symbols.
Okay, then we should find symbol like that until improved.
It may confuse developer, but it seems okay for our code to support clang
as it meets minimal condition - build failure.
And I hope it become improved in future!
Thanks,
Hyeonggon
Cheers,
Nathan
7:26 p.m.
New subject: [PATCH FIX -next] Re: [linux-next:master 7012/7430] include/linux/compiler_types.h:328:38: error: call to '__compiletime_assert_183' declared with attribute error: unexpected size in kmalloc_index()
On 6/12/21 2:19 AM, Hyeonggon Yoo wrote:
On Sat, Jun 12, 2021, 8:59 AM Vlastimil Babka <vbabka(a)suse.cz
<mailto:vbabka@suse.cz>> wrote:
> But little bit worried :(
> The code is getting too complicated...
> How do you think?
Yeah, I expected that problems like this could occur as we're poking at some
rare corner cases of compiler implementations here. But if that leads to fixes
in compilers, good for everyone I'd say.
So I would try this, even if it becomes complicated.
Okay, I see the code is okay, but one thing to suggest:
The problem already existed in kmalloc_node before the patch And we found it
by adding BUILD_BUG_ON in kmalloc_index.
So I suggest adding the condition in kmalloc_node. How about this?
If we add it to kmalloc_node() it means with CONFIG_PROFILE_ALL_BRANCHES we will
not be using the compile-time optimized kmalloc_index anywhere. But we get the
error only with one file, so in all the other cases the gcc optimizer seems to
work fine even with this config enabled. Placing the condition to
kmalloc_index() thus limits it only to cases where it doesn't work.
As you indicated you would be away starting this week, let me just sent the fix
right now so we stop getting the -next reports:
----8<----
From 56c164dc5c0485c2e133190c0f56e4069844ad62 Mon Sep 17 00:00:00 2001
From: Vlastimil Babka <vbabka(a)suse.cz>
Date: Mon, 14 Jun 2021 11:16:03 +0200
Subject: [PATCH]
mm-slub-change-run-time-assertion-in-kmalloc_index-to-compile-time-fix-3
Kernel test robot reports a false-positive compile-time assert while compiling
kernel/bpf/local_storage.c
In file included from <command-line>:
In function 'kmalloc_index',
inlined from 'kmalloc_node' at include/linux/slab.h:572:20,
inlined from 'bpf_map_kmalloc_node.isra.0.part.0' at
include/linux/bpf.h:1319:9:
> >> include/linux/compiler_types.h:328:38: error: call to
'__compiletime_assert_183' declared with attribute error: unexpected size in
kmalloc_index()
328 | _compiletime_assert(condition, msg,
__compiletime_assert_, __COUNTER__)
| ^
include/linux/compiler_types.h:309:4: note: in definition of macro
'__compiletime_assert'
309 | prefix ## suffix(); \
| ^~~~~~
include/linux/compiler_types.h:328:2: note: in expansion of macro
'_compiletime_assert'
328 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
| ^~~~~~~~~~~~~~~~~~~
include/linux/build_bug.h:39:37: note: in expansion of macro
'compiletime_assert'
39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
| ^~~~~~~~~~~~~~~~~~
include/linux/slab.h:389:2: note: in expansion of macro 'BUILD_BUG_ON_MSG'
389 | BUILD_BUG_ON_MSG(1, "unexpected size in kmalloc_index()");
| ^~~~~~~~~~~~~~~~
This only happens with CONFIG_PROFILE_ALL_BRANCHES enabled. Informal
conversation with gcc developers suggest that this config pushes the optimizer
to a corner case where in kmalloc_node() the 'size' is considered a
builtin-constant and thus kmalloc_index() is chosen, but later in
kmalloc_index() the notion of compile-time constant is lost and thus the
compiletime assert becomes reachable.
While there's plan to submit a proper gcc bug report with reduced testcase,
for now add CONFIG_PROFILE_ALL_BRANCHES to the list of situations where we
keep using the runtime BUG_ON() instead of compile-time assert.
This is a 3rd fix for mmotm patch
mm-slub-change-run-time-assertion-in-kmalloc_index-to-compile-time.patch
[1] https://lore.kernel.org/linux-mm/202106051442.G1VJubTz-lkp@intel.com/
Reported-by: kernel test robot <lkp(a)intel.com>
Signed-off-by: Vlastimil Babka <vbabka(a)suse.cz>
---
include/linux/slab.h | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/include/linux/slab.h b/include/linux/slab.h
index 8d8dd8571261..083f3ce550bc 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -413,7 +413,8 @@ static __always_inline unsigned int __kmalloc_index(size_t size,
if (size <= 16 * 1024 * 1024) return 24;
if (size <= 32 * 1024 * 1024) return 25;
- if ((IS_ENABLED(CONFIG_CC_IS_GCC) || CONFIG_CLANG_VERSION >= 110000) &&
size_is_constant)
+ if ((IS_ENABLED(CONFIG_CC_IS_GCC) || CONFIG_CLANG_VERSION >= 110000)
+ && !IS_ENABLED(CONFIG_PROFILE_ALL_BRANCHES) && size_is_constant)
BUILD_BUG_ON_MSG(1, "unexpected size in kmalloc_index()");
else
BUG();
--
2.32.0
463
days inactive
472
days old
20 comments
4 participants
participants (4)
-
Hyeonggon Yoo -
kernel test robot -
Nathan Chancellor -
Vlastimil Babka