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