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 >
