[PATCH] compiler.h: fix error in BUILD_BUG_ON() reporting

[PATCH] compiler.h: fix error in BUILD_BUG_ON() reporting

[Vegard Nossum]

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

Cc: Daniel Santos <daniel.santos@pobox.com>
Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
Cc: Ian Abbott <abbotti@mev.co.uk>
Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
---
 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),				\
-- 
2.16.1.72.g5be1f00a9.dirty

Re: [PATCH] compiler.h: fix error in BUILD_BUG_ON() reporting

[Masahiro Yamada]

On Tue, Mar 31, 2020 at 8:28 PM Vegard Nossum <vegard.nossum@oracle.com> wrote:
>
> 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__)
>
> Cc: Daniel Santos <daniel.santos@pobox.com>
> Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>


Reviewed-by: Masahiro Yamada <yamada.masahiro@socionext.com>




> Cc: Ian Abbott <abbotti@mev.co.uk>
> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
> ---
>  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),                            \
> --
> 2.16.1.72.g5be1f00a9.dirty
>


-- 
Best Regards
Masahiro Yamada

Re: [PATCH] compiler.h: fix error in BUILD_BUG_ON() reporting

[Joe Perches]

On Tue, 2020-03-31 at 13:26 +0200, Vegard Nossum wrote:
> 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__)
[]
> diff --git 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__)

This might be better using something like __LINE__ ## _ ## __COUNTER__

as line # is somewhat useful to identify the specific assert in a file.

Re: [PATCH] compiler.h: fix error in BUILD_BUG_ON() reporting

[Rasmus Villemoes]

On 31/03/2020 20.20, Joe Perches wrote:
> On Tue, 2020-03-31 at 13:26 +0200, Vegard Nossum wrote:

>>  #define compiletime_assert(condition, msg) \
>> -	_compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
>> +	_compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
> 
> This might be better using something like __LINE__ ## _ ## __COUNTER__
> 
> as line # is somewhat useful to identify the specific assert in a file.
> 

Eh, if the assert fires, doesn't the compiler's diagnostics already
contain all kinds of location information?

Rasmus

Re: [PATCH] compiler.h: fix error in BUILD_BUG_ON() reporting

[Joe Perches]

On Tue, 2020-03-31 at 20:56 +0200, Rasmus Villemoes wrote:
> On 31/03/2020 20.20, Joe Perches wrote:
> > On Tue, 2020-03-31 at 13:26 +0200, Vegard Nossum wrote:
> > >  #define compiletime_assert(condition, msg) \
> > > -	_compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
> > > +	_compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
> > 
> > This might be better using something like __LINE__ ## _ ## __COUNTER__
> > 
> > as line # is somewhat useful to identify the specific assert in a file.
> > 
> 
> Eh, if the assert fires, doesn't the compiler's diagnostics already
> contain all kinds of location information?

I presume if that were enough,
neither __LINE__ nor __COUNTER__
would be useful.

Re: [PATCH] compiler.h: fix error in BUILD_BUG_ON() reporting

[Rasmus Villemoes]

On 31/03/2020 21.00, Joe Perches wrote:
> On Tue, 2020-03-31 at 20:56 +0200, Rasmus Villemoes wrote:
>> On 31/03/2020 20.20, Joe Perches wrote:
>>> On Tue, 2020-03-31 at 13:26 +0200, Vegard Nossum wrote:
>>>>  #define compiletime_assert(condition, msg) \
>>>> -	_compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
>>>> +	_compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
>>>
>>> This might be better using something like __LINE__ ## _ ## __COUNTER__
>>>
>>> as line # is somewhat useful to identify the specific assert in a file.
>>>
>>
>> Eh, if the assert fires, doesn't the compiler's diagnostics already
>> contain all kinds of location information?
> 
> I presume if that were enough,
> neither __LINE__ nor __COUNTER__
> would be useful.

Not only useful, necessary: They are used to create a unique identifier.
Which turns out to not be unique when one uses __LINE__, causing the
problem Vegard saw and fixes.

Rasmus

Re: [PATCH] compiler.h: fix error in BUILD_BUG_ON() reporting

[Vegard Nossum]

On 3/31/20 9:00 PM, Joe Perches wrote:
> On Tue, 2020-03-31 at 20:56 +0200, Rasmus Villemoes wrote:
>> On 31/03/2020 20.20, Joe Perches wrote:
>>> On Tue, 2020-03-31 at 13:26 +0200, Vegard Nossum wrote:
>>>>   #define compiletime_assert(condition, msg) \
>>>> -	_compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
>>>> +	_compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
>>>
>>> This might be better using something like __LINE__ ## _ ## __COUNTER__
>>>
>>> as line # is somewhat useful to identify the specific assert in a file.
>>>
>>
>> Eh, if the assert fires, doesn't the compiler's diagnostics already
>> contain all kinds of location information?
> 
> I presume if that were enough,
> neither __LINE__ nor __COUNTER__
> would be useful.
> 

__LINE__ is only used currently for creating a unique identifier, as far
as I can tell.

The way it works is that it creates a function declaration with the
attribute __attribute__((error(message))), which makes gcc throw an
error if the function is ever used (i.e. calls are not compiled out).

The number does appear in the output, but it's not even really obvious
that it's a line number. And the compiler's diagnostics are pretty good
at showing the whole "stack trace" of where the call came from
(including the proper line numbers).


Vegard

Re: [PATCH] compiler.h: fix error in BUILD_BUG_ON() reporting

[Daniel Santos]

On 3/31/20 2:08 PM, Vegard Nossum wrote:
>
> __LINE__ is only used currently for creating a unique identifier, as far
> as I can tell.
>
> The way it works is that it creates a function declaration with the
> attribute __attribute__((error(message))), which makes gcc throw an
> error if the function is ever used (i.e. calls are not compiled out).

Back before __attribute__((error())), these macros used to just declare
a function that isn't defined and you only got an error at link-time --
the line number did matter then.  Then there was the negative array
index thing.

>
> The number does appear in the output, but it's not even really obvious
> that it's a line number. And the compiler's diagnostics are pretty good
> at showing the whole "stack trace" of where the call came from
> (including the proper line numbers).
>
>
> Vegard

And the stack trace used to be useless without -g or -g3, but I believe
gcc gives the macro expansion back trace without it now.  But imo, the
macro expansion back trace is a lot of noise that we can eliminate with
a direct gcc mechanism to break the build on some __builtin_constant_p()
expression.

Daniel

Re: [PATCH] compiler.h: fix error in BUILD_BUG_ON() reporting

[Daniel Santos]

On 3/31/20 1:56 PM, Rasmus Villemoes wrote:
> On 31/03/2020 20.20, Joe Perches wrote:
>> On Tue, 2020-03-31 at 13:26 +0200, Vegard Nossum wrote:
>>>  #define compiletime_assert(condition, msg) \
>>> -	_compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
>>> +	_compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
>> This might be better using something like __LINE__ ## _ ## __COUNTER__
>>
>> as line # is somewhat useful to identify the specific assert in a file.
>>
> Eh, if the assert fires, doesn't the compiler's diagnostics already
> contain all kinds of location information?
>
> Rasmus

Yes, the diagnostic contains the file name and line in a far more useful
format that every IDE knows how to read.  __LINE__ is only used for
uniqueness and was chosen when __COUNTER__ (introduced in gcc 4.3) was
still somewhat new.

Daniel

Re: [PATCH] compiler.h: fix error in BUILD_BUG_ON() reporting

[Daniel Santos]

On 3/31/20 6:26 AM, Vegard Nossum wrote:
> 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__)
>
> Cc: Daniel Santos <daniel.santos@pobox.com>
> Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
> Cc: Ian Abbott <abbotti@mev.co.uk>
> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
> ---
>  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),				\

This will break builds using gcc 4.2 and earlier and the expectation was
that you don't put two of them on the same line -- not helpful in macros
where it all must be on the same line.  Is gcc 4.2 still supported?  If
so, I recommend using another macro for the unique number that uses
__COUNTER__ if available and __LINE__ otherwise.  This was the decision
for using __LINE__ when I wrote the original anyway.

Also note that this construct:

BUILD_BUG_ON_MSG(0, "I like chicken"); BUILD_BUG_ON_MSG(1, "I don't like
chicken");

will incorrectly claim that I like chicken.  This is because of how
__attribute__((error)) works -- gcc will use the first declaration to
define the error message.

I couple of years ago, I almost wrote a gcc extension to get rid of this
whole mess and just __builtin_const_assert(cond, msg).  Maybe I'll
finish that this year.

Daniel

Re: [PATCH] compiler.h: fix error in BUILD_BUG_ON() reporting

[Nathan Chancellor]

On Tue, Mar 31, 2020 at 05:25:38PM -0500, Daniel Santos wrote:
> On 3/31/20 6:26 AM, Vegard Nossum wrote:
> > 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__)
> >
> > Cc: Daniel Santos <daniel.santos@pobox.com>
> > Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> > Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
> > Cc: Ian Abbott <abbotti@mev.co.uk>
> > Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
> > ---
> >  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),				\
> 
> This will break builds using gcc 4.2 and earlier and the expectation was
> that you don't put two of them on the same line -- not helpful in macros
> where it all must be on the same line.  Is gcc 4.2 still supported?  If
> so, I recommend using another macro for the unique number that uses
> __COUNTER__ if available and __LINE__ otherwise.  This was the decision
> for using __LINE__ when I wrote the original anyway.
> 
> Also note that this construct:
> 
> BUILD_BUG_ON_MSG(0, "I like chicken"); BUILD_BUG_ON_MSG(1, "I don't like
> chicken");
> 
> will incorrectly claim that I like chicken.  This is because of how
> __attribute__((error)) works -- gcc will use the first declaration to
> define the error message.
> 
> I couple of years ago, I almost wrote a gcc extension to get rid of this
> whole mess and just __builtin_const_assert(cond, msg).  Maybe I'll
> finish that this year.
> 
> Daniel

No, GCC 4.6 is the minimum required version and it is highly likely that
the minimum version of GCC will be raised to 4.8 soon:

https://lore.kernel.org/lkml/20200123153341.19947-10-will@kernel.org/
https://git.kernel.org/peterz/queue/c/0e75b883b400ac4b1dafafe3cbd2e0a39b714232

Cheers,
Nathan

Re: [PATCH] compiler.h: fix error in BUILD_BUG_ON() reporting

[Daniel Santos]

On 3/31/20 5:36 PM, Nathan Chancellor wrote:
> On Tue, Mar 31, 2020 at 05:25:38PM -0500, Daniel Santos wrote:
>>
>> This will break builds using gcc 4.2 and earlier and the expectation was
>> that you don't put two of them on the same line -- not helpful in macros
>> where it all must be on the same line.  Is gcc 4.2 still supported?  If
>> so, I recommend using another macro for the unique number that uses
>> __COUNTER__ if available and __LINE__ otherwise.  This was the decision
>> for using __LINE__ when I wrote the original anyway.
>>
>> Also note that this construct:
>>
>> BUILD_BUG_ON_MSG(0, "I like chicken"); BUILD_BUG_ON_MSG(1, "I don't like
>> chicken");
>>
>> will incorrectly claim that I like chicken.  This is because of how
>> __attribute__((error)) works -- gcc will use the first declaration to
>> define the error message.
>>
>> I couple of years ago, I almost wrote a gcc extension to get rid of this
>> whole mess and just __builtin_const_assert(cond, msg).  Maybe I'll
>> finish that this year.
>>
>> Daniel
> No, GCC 4.6 is the minimum required version and it is highly likely that
> the minimum version of GCC will be raised to 4.8 soon:
>
> https://lore.kernel.org/lkml/20200123153341.19947-10-will@kernel.org/
> https://git.kernel.org/peterz/queue/c/0e75b883b400ac4b1dafafe3cbd2e0a39b714232
>
> Cheers,
> Nathan

Thank you Nathan.  In that case this is definitely what we want now.

Reviewed-by: Daniel Santos <daniel.santos@pobox.com>

Cheers,
Daniel