Hi,
There is no point in taking this patch on any stable kernel as it's just improving a build error diagnostic message.
Vegard
On 4/15/20 1:33 PM, Sasha Levin wrote:
From: Vegard Nossum vegard.nossum@oracle.com
[ Upstream commit af9c5d2e3b355854ff0e4acfbfbfadcd5198a349 ]
compiletime_assert() uses __LINE__ to create a unique function name. This means that if you have more than one BUILD_BUG_ON() in the same source line (which can happen if they appear e.g. in a macro), then the error message from the compiler might output the wrong condition.
For this source file:
#include <linux/build_bug.h>
#define macro() \ BUILD_BUG_ON(1); \ BUILD_BUG_ON(0);
void foo() { macro(); }
gcc would output:
./include/linux/compiler.h:350:38: error: call to `__compiletime_assert_9' declared with attribute error: BUILD_BUG_ON failed: 0 _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
However, it was not the BUILD_BUG_ON(0) that failed, so it should say 1 instead of 0. With this patch, we use __COUNTER__ instead of __LINE__, so each BUILD_BUG_ON() gets a different function name and the correct condition is printed:
./include/linux/compiler.h:350:38: error: call to `__compiletime_assert_0' declared with attribute error: BUILD_BUG_ON failed: 1 _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
Signed-off-by: Vegard Nossum vegard.nossum@oracle.com Signed-off-by: Andrew Morton akpm@linux-foundation.org Reviewed-by: Masahiro Yamada yamada.masahiro@socionext.com Reviewed-by: Daniel Santos daniel.santos@pobox.com Cc: Rasmus Villemoes linux@rasmusvillemoes.dk Cc: Ian Abbott abbotti@mev.co.uk Cc: Joe Perches joe@perches.com Link: http://lkml.kernel.org/r/20200331112637.25047-1-vegard.nossum@oracle.com Signed-off-by: Linus Torvalds torvalds@linux-foundation.org Signed-off-by: Sasha Levin sashal@kernel.org
include/linux/compiler.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/compiler.h b/include/linux/compiler.h index 5e88e7e33abec..034b0a644efcc 100644 --- a/include/linux/compiler.h +++ b/include/linux/compiler.h @@ -347,7 +347,7 @@ static inline void *offset_to_ptr(const int *off)
- compiler has support to do so.
*/ #define compiletime_assert(condition, msg) \
- _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
- _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
#define compiletime_assert_atomic_type(t) \ compiletime_assert(__native_word(t), \
On Wed, Apr 22, 2020 at 10:21:23AM +0200, Vegard Nossum wrote:
Hi,
There is no point in taking this patch on any stable kernel as it's just improving a build error diagnostic message.
build error messages are nice to have be correct, don't you think?
Seems like a valid fix for me.
thanks,
greg k-h
On 4/22/20 10:34 AM, Greg KH wrote:
On Wed, Apr 22, 2020 at 10:21:23AM +0200, Vegard Nossum wrote:
Hi,
There is no point in taking this patch on any stable kernel as it's just improving a build error diagnostic message.
build error messages are nice to have be correct, don't you think?
Seems like a valid fix for me.
thanks,
greg k-h
The patch will break on gcc 4.2 and earlier, so if the older stable kernels support those then people might be surprised. The mainline patch was deemed fine since gcc 4.6 is required. More info here: https://lkml.org/lkml/2020/3/31/1477
If no stable kernel is built with gcc <= 4.2 then you can disregard this.
I think the patch also doesn't really satisfy the following criteria from stable_kernel_rules.txt:
- "It must fix a real bug that bothers people": It bothered me (and I ran into the bug) on mainline, but that was while writing brand new code that used BUILD_BUG_ON(). Presumably these things will not fire on existing kernel code.
- "It must fix a problem that causes a build error ...": It doesn't fix any of the things listed, not even a build error, just a _diagnostic_ for an incredibly rare case of a rare kind of build error.
In the end, I am not personally opposed to having the patch in stable, but it seems to go against everything I've ever heard about stable rules so I'm a bit surprised when you take it anyway. I think it might reduce people's trust in stable kernels if any random weird patch is taken uncritically when even the patch author points out that it doesn't fit the criteria!
Vegard
On Wed, Apr 22, 2020 at 10:53:33AM +0200, Vegard Nossum wrote:
On 4/22/20 10:34 AM, Greg KH wrote:
On Wed, Apr 22, 2020 at 10:21:23AM +0200, Vegard Nossum wrote:
Hi,
There is no point in taking this patch on any stable kernel as it's just improving a build error diagnostic message.
build error messages are nice to have be correct, don't you think?
Seems like a valid fix for me.
thanks,
greg k-h
The patch will break on gcc 4.2 and earlier, so if the older stable kernels support those then people might be surprised. The mainline patch was deemed fine since gcc 4.6 is required. More info here: https://lkml.org/lkml/2020/3/31/1477
If no stable kernel is built with gcc <= 4.2 then you can disregard this.
I think the patch also doesn't really satisfy the following criteria from stable_kernel_rules.txt:
- "It must fix a real bug that bothers people": It bothered me (and I
ran into the bug) on mainline, but that was while writing brand new code that used BUILD_BUG_ON(). Presumably these things will not fire on existing kernel code.
- "It must fix a problem that causes a build error ...": It doesn't fix
any of the things listed, not even a build error, just a _diagnostic_ for an incredibly rare case of a rare kind of build error.
In the end, I am not personally opposed to having the patch in stable, but it seems to go against everything I've ever heard about stable rules so I'm a bit surprised when you take it anyway. I think it might reduce people's trust in stable kernels if any random weird patch is taken uncritically when even the patch author points out that it doesn't fit the criteria!
Hey Vegard,
I'll drop it.
In general, patches that address build issues are easier to rationalize in the context of stable as the regressions they might cause will be limited to build time.
linux-stable-mirror@lists.linaro.org
-
Greg KH -
Sasha Levin -
Vegard Nossum