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

[Hyeonggon Yoo <42.hyeyoo@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 10569 bytes --]

On Thu, Jun 10, 2021, 8:43 PM Vlastimil Babka <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.


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

[-- Attachment #2: attachment.htm --]
[-- Type: text/html, Size: 14412 bytes --]