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

From: Vlastimil Babka <vbabka@suse.cz>
To: Hyeonggon Yoo <42.hyeyoo@gmail.com>
Cc: kernel test robot <lkp@intel.com>,
	kbuild-all@lists.01.org,
	Linux Memory Management List <linux-mm@kvack.org>,
	Andrew Morton <akpm@linux-foundation.org>
Subject: 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()
Date: Tue, 8 Jun 2021 19:27:52 +0200	[thread overview]
Message-ID: <2d2d792e-e189-99a4-36cb-f1473a4df9ad@suse.cz> (raw)
In-Reply-To: <20210608170528.GA28015@hyeyoo>

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
> 