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()

[Vlastimil Babka <vbabka@suse.cz>]

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
>