On Fri, Jun 11, 2021, 7:27 PM Vlastimil Babka <vbabka@suse.cz> wrote:
On 6/11/21 10:58 AM, Hyeonggon Yoo wrote:
> On Thu, Jun 10, 2021, 8:43 PM Vlastimil Babka <vbabka@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/45UNTPJX7VUKLB6TUYPJOS7DMREVW5US/


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