Hello Vlastimil, recently kbuild-all test bot reported compile error on clang 10.0.1, with defconfig.
Nathan Chancellor wrote:
I think this happens because arch_prepare_optimized_kprobe() calls kzalloc() with a size of MAX_OPTINSN_SIZE, which is
#define MAX_OPTINSN_SIZE \ (((unsigned long)optprobe_template_end - \ (unsigned long)optprobe_template_entry) + \ MAX_OPTIMIZED_LENGTH + JMP32_INSN_SIZE)
and the optprobe_template_{end,entry} are not evaluated as constants.
I am not sure what the solution is. There seem to be a growing list of issues with LLVM 10 that were fixed in LLVM 11, which might necessitate requiring LLVM 11 and newer to build the kernel, given this affects a defconfig. Cheers, Nathan
I think it's because kmalloc compiles successfully when size is constant, and kmalloc_index isn't. so I think compiler seems to be confused.
currently if size is non-constant, kmalloc calls dummy function __kmalloc, which always returns NULL.
so what about changing kmalloc to do compile-time assertion too, and track all callers that are calling kmalloc with non-constant argument.
How do you think? If you think it is the solution, I'll do that work.
On 5/15/21 11:09 PM, Hyeonggon Yoo wrote:
Hello Vlastimil, recently kbuild-all test bot reported compile error on clang 10.0.1, with defconfig.
Hm yes, catching some compiler bug was something that was noted to be possible to happen.
Nathan Chancellor wrote:
I think this happens because arch_prepare_optimized_kprobe() calls kzalloc() with a size of MAX_OPTINSN_SIZE, which is
#define MAX_OPTINSN_SIZE \ (((unsigned long)optprobe_template_end - \ (unsigned long)optprobe_template_entry) + \ MAX_OPTIMIZED_LENGTH + JMP32_INSN_SIZE)
and the optprobe_template_{end,entry} are not evaluated as constants.
I am not sure what the solution is. There seem to be a growing list of issues with LLVM 10 that were fixed in LLVM 11, which might necessitate requiring LLVM 11 and newer to build the kernel, given this affects a defconfig. Cheers, Nathan
I think it's because kmalloc compiles successfully when size is constant, and kmalloc_index isn't. so I think compiler seems to be confused.
currently if size is non-constant, kmalloc calls dummy function __kmalloc, which always returns NULL.
That's a misunderstanding. __kmalloc() is not a dummy function, you probably found only the header declaration.
so what about changing kmalloc to do compile-time assertion too, and track all callers that are calling kmalloc with non-constant argument.
kmalloc() is expected to be called with both constant and non-constant size. __builtin_constant_p() is used to determine which implementation to use. One based on kmalloc_index(), other on __kmalloc().
It appears clang 10.0.1 is mistakenly evaluating __builtin_constant_p() as true. Probably something to do with LTO, because MAX_OPTINSN_SIZE seems it could be a "link-time constant".
Maybe we could extend Marco Elver's followup patch that uses BUILD_BUG_ON vs BUG() depending on size_is_constant parameter. It could use BUG() also if the compiler is LLVM < 11 or something. What would be the proper code for this condition?
How do you think? If you think it is the solution, I'll do that work.
On Sat, May 15, 2021 at 11:24:25PM +0200, Vlastimil Babka wrote:
That's a misunderstanding. __kmalloc() is not a dummy function, you probably found only the header declaration.
Sorry, that was totally my misunderstanding. I was reading dummy function in arch/alpha/boot/bootpz.c:415. I wrongly configured the tool.
It appears clang 10.0.1 is mistakenly evaluating __builtin_constant_p() as true. Probably something to do with LTO, because MAX_OPTINSN_SIZE seems it could be a "link-time constant".
That is what I was missing. Thank you for kindly explaining it.
Maybe we could extend Marco Elver's followup patch that uses BUILD_BUG_ON vs BUG() depending on size_is_constant parameter. It could use BUG() also if the compiler is LLVM < 11 or something. What would be the proper code for this condition?
Fixing clang's bug in linux kernel doesn't seem to be a solution. So now I understand why Nathan said we might require LLVM > 11.
I thought I should do something to fix it because I sent the patch. but I was misunderstanding a lot. Thank you sincerely for letting me know.
Thanks,
Hyeonggon
On Sat, May 15, 2021 at 11:24:25PM +0200, Vlastimil Babka wrote:
On 5/15/21 11:09 PM, Hyeonggon Yoo wrote:
Hello Vlastimil, recently kbuild-all test bot reported compile error on clang 10.0.1, with defconfig.
Hm yes, catching some compiler bug was something that was noted to be possible to happen.
Nathan Chancellor wrote:
I think this happens because arch_prepare_optimized_kprobe() calls kzalloc() with a size of MAX_OPTINSN_SIZE, which is
#define MAX_OPTINSN_SIZE \ (((unsigned long)optprobe_template_end - \ (unsigned long)optprobe_template_entry) + \ MAX_OPTIMIZED_LENGTH + JMP32_INSN_SIZE)
and the optprobe_template_{end,entry} are not evaluated as constants.
I am not sure what the solution is. There seem to be a growing list of issues with LLVM 10 that were fixed in LLVM 11, which might necessitate requiring LLVM 11 and newer to build the kernel, given this affects a defconfig. Cheers, Nathan
I think it's because kmalloc compiles successfully when size is constant, and kmalloc_index isn't. so I think compiler seems to be confused.
currently if size is non-constant, kmalloc calls dummy function __kmalloc, which always returns NULL.
That's a misunderstanding. __kmalloc() is not a dummy function, you probably found only the header declaration.
so what about changing kmalloc to do compile-time assertion too, and track all callers that are calling kmalloc with non-constant argument.
kmalloc() is expected to be called with both constant and non-constant size. __builtin_constant_p() is used to determine which implementation to use. One based on kmalloc_index(), other on __kmalloc().
It appears clang 10.0.1 is mistakenly evaluating __builtin_constant_p() as true. Probably something to do with LTO, because MAX_OPTINSN_SIZE seems it could be a "link-time constant".
This happens with x86_64 defconfig so LTO is not involved.
However, the explanation makes sense, given that the LLVM change I landed on changes the sparse conditional constant propagation pass, which I believe can influence how LLVM handles __builtin_constant_p().
Maybe we could extend Marco Elver's followup patch that uses BUILD_BUG_ON vs BUG() depending on size_is_constant parameter. It could use BUG() also if the compiler is LLVM < 11 or something. What would be the proper code for this condition?
This should work I think:
diff --git a/include/linux/slab.h b/include/linux/slab.h index 9d316aac0aba..1b653266f2aa 100644 --- a/include/linux/slab.h +++ b/include/linux/slab.h @@ -413,7 +413,7 @@ static __always_inline unsigned int __kmalloc_index(size_t size, if (size <= 16 * 1024 * 1024) return 24; if (size <= 32 * 1024 * 1024) return 25;
- if (size_is_constant) + if ((IS_ENABLED(CONFIG_CC_IS_GCC) || CONFIG_CLANG_VERSION > 110000) && size_is_constant) BUILD_BUG_ON_MSG(1, "unexpected size in kmalloc_index()"); else BUG();
On Sat, May 15, 2021 at 11:34:49PM -0700, Nathan Chancellor wrote:
This should work I think:
compiled well with clang-10.0.1, clang-11.0.0, and gcc-10.2.0 with x86_64 default config.
is the condition CONFIG_CLANG_VERSION > 110000, not including 110000 it self?
diff --git a/include/linux/slab.h b/include/linux/slab.h index 9d316aac0aba..1b653266f2aa 100644 --- a/include/linux/slab.h +++ b/include/linux/slab.h @@ -413,7 +413,7 @@ static __always_inline unsigned int __kmalloc_index(size_t size, if (size <= 16 * 1024 * 1024) return 24; if (size <= 32 * 1024 * 1024) return 25;
- if (size_is_constant)
- if ((IS_ENABLED(CONFIG_CC_IS_GCC) || CONFIG_CLANG_VERSION > 110000) && size_is_constant) BUILD_BUG_ON_MSG(1, "unexpected size in kmalloc_index()"); else BUG();
On 5/17/2021 5:38 PM, Hyeonggon Yoo wrote:
On Sat, May 15, 2021 at 11:34:49PM -0700, Nathan Chancellor wrote:
This should work I think:
compiled well with clang-10.0.1, clang-11.0.0, and gcc-10.2.0 with x86_64 default config.
is the condition CONFIG_CLANG_VERSION > 110000, not including 110000 it self?
Ah sorry, that should definitely be >= :(
That is what I get for writing an email that late... in reality, it probably won't matter due to the availability of 11.0.1 and 11.1.0 but it should absolutely be changed.
I have not given Nick's patch a go yet but would something like this be acceptable? If so, did you want me to send a formal fixup patch or did you want to send a v4? I have no personal preference.
diff --git a/include/linux/slab.h b/include/linux/slab.h index 9d316aac0aba..1b653266f2aa 100644 --- a/include/linux/slab.h +++ b/include/linux/slab.h @@ -413,7 +413,7 @@ static __always_inline unsigned int __kmalloc_index(size_t size, if (size <= 16 * 1024 * 1024) return 24; if (size <= 32 * 1024 * 1024) return 25;
- if (size_is_constant)
- if ((IS_ENABLED(CONFIG_CC_IS_GCC) || CONFIG_CLANG_VERSION > 110000) && size_is_constant) BUILD_BUG_ON_MSG(1, "unexpected size in kmalloc_index()"); else BUG();
On Mon, May 17, 2021 at 05:43:22PM -0700, Nathan Chancellor wrote:
Ah sorry, that should definitely be >= :(
That is what I get for writing an email that late... in reality, it probably won't matter due to the availability of 11.0.1 and 11.1.0 but it should absolutely be changed.
I have not given Nick's patch a go yet but would something like this be acceptable? If so, did you want me to send a formal fixup patch or did you want to send a v4? I have no personal preference.
I think fixup patch patch will be better as we can undo it later. I don't think Nick's patch is needed because that code is not related with clang version, and we don't need that code even in clang 10.
then is there something I can help for now?
thanks, Hyeonggon
On 5/18/21 2:43 AM, Nathan Chancellor wrote:
On 5/17/2021 5:38 PM, Hyeonggon Yoo wrote:
On Sat, May 15, 2021 at 11:34:49PM -0700, Nathan Chancellor wrote:
This should work I think:
compiled well with clang-10.0.1, clang-11.0.0, and gcc-10.2.0 with x86_64 default config.
is the condition CONFIG_CLANG_VERSION > 110000, not including 110000 it self?
Good spot.
Ah sorry, that should definitely be >= :(
That is what I get for writing an email that late... in reality, it probably won't matter due to the availability of 11.0.1 and 11.1.0 but it should absolutely be changed.
I have not given Nick's patch a go yet but would something like this be acceptable?
Yes.
If so, did you want me to send a formal fixup patch or did you want to send a v4? I have no personal preference.
At this point a fixup is the usual way. Andrew might squash it to the original patch (also with Marco's fixup) before sending to Linus.
diff --git a/include/linux/slab.h b/include/linux/slab.h index 9d316aac0aba..1b653266f2aa 100644 --- a/include/linux/slab.h +++ b/include/linux/slab.h @@ -413,7 +413,7 @@ static __always_inline unsigned int __kmalloc_index(size_t size, if (size <= 16 * 1024 * 1024) return 24; if (size <= 32 * 1024 * 1024) return 25; - if (size_is_constant) + if ((IS_ENABLED(CONFIG_CC_IS_GCC) || CONFIG_CLANG_VERSION > 110000) && size_is_constant) BUILD_BUG_ON_MSG(1, "unexpected size in kmalloc_index()"); else BUG();
On Tue, May 18, 2021 at 11:28:17AM +0200, Vlastimil Babka wrote:
On 5/18/21 2:43 AM, Nathan Chancellor wrote:
On 5/17/2021 5:38 PM, Hyeonggon Yoo wrote:
On Sat, May 15, 2021 at 11:34:49PM -0700, Nathan Chancellor wrote:
This should work I think:
compiled well with clang-10.0.1, clang-11.0.0, and gcc-10.2.0 with x86_64 default config.
is the condition CONFIG_CLANG_VERSION > 110000, not including 110000 it self?
Good spot.
Thanks!
Ah sorry, that should definitely be >= :(
That is what I get for writing an email that late... in reality, it probably won't matter due to the availability of 11.0.1 and 11.1.0 but it should absolutely be changed.
I have not given Nick's patch a go yet but would something like this be acceptable?
Yes.
You mean Nick's patch to added with Nathan's code? I'm not sure we need this, but will add it if you can accept it.
I'll send fixup patch soon. tell me if I can improve anything on it.
Thanks, Hyeonggon
On 5/18/21 1:18 PM, Hyeonggon Yoo wrote:
On Tue, May 18, 2021 at 11:28:17AM +0200, Vlastimil Babka wrote:
On 5/18/21 2:43 AM, Nathan Chancellor wrote:
On 5/17/2021 5:38 PM, Hyeonggon Yoo wrote:
On Sat, May 15, 2021 at 11:34:49PM -0700, Nathan Chancellor wrote:
This should work I think:
compiled well with clang-10.0.1, clang-11.0.0, and gcc-10.2.0 with x86_64 default config.
is the condition CONFIG_CLANG_VERSION > 110000, not including 110000 it self?
Good spot.
Thanks!
Ah sorry, that should definitely be >= :(
That is what I get for writing an email that late... in reality, it probably won't matter due to the availability of 11.0.1 and 11.1.0 but it should absolutely be changed.
I have not given Nick's patch a go yet but would something like this be acceptable?
Yes.
You mean Nick's patch to added with Nathan's code?
No, I thought Nathan was asking about his own proposal. I don't think Nick's patch that adds 26 index solves the issue. Nathan's proposal fixed with '>=' is OK.
I'm not sure we need this, but will add it if you can accept it.
I'll send fixup patch soon. tell me if I can improve anything on it.
Thanks, Hyeonggon
On Tue, May 18, 2021 at 01:34:07PM +0200, Vlastimil Babka wrote:
On 5/18/21 1:18 PM, Hyeonggon Yoo wrote:
On Tue, May 18, 2021 at 11:28:17AM +0200, Vlastimil Babka wrote:
On 5/18/21 2:43 AM, Nathan Chancellor wrote:
On 5/17/2021 5:38 PM, Hyeonggon Yoo wrote:
On Sat, May 15, 2021 at 11:34:49PM -0700, Nathan Chancellor wrote:
This should work I think:
compiled well with clang-10.0.1, clang-11.0.0, and gcc-10.2.0 with x86_64 default config.
is the condition CONFIG_CLANG_VERSION > 110000, not including 110000 it self?
Good spot.
Thanks!
Ah sorry, that should definitely be >= :(
That is what I get for writing an email that late... in reality, it probably won't matter due to the availability of 11.0.1 and 11.1.0 but it should absolutely be changed.
I have not given Nick's patch a go yet but would something like this be acceptable?
Yes.
You mean Nick's patch to added with Nathan's code?
No, I thought Nathan was asking about his own proposal. I don't think Nick's patch that adds 26 index solves the issue. Nathan's proposal fixed with '>=' is OK.
Ah, Okay! I sent the patch.
Thanks, Hyeonggon
-
Hyeonggon Yoo -
Nathan Chancellor -
Vlastimil Babka