build_bug.h: add wrapper for _Static_assert
diff mbox series

Message ID 20190203192401.29170-1-linux@rasmusvillemoes.dk
State In Next
Commit c9068a141b35cbab2a9f0fabce0a076730cc7955
Headers show
Series
  • build_bug.h: add wrapper for _Static_assert
Related show

Commit Message

Rasmus Villemoes Feb. 3, 2019, 7:24 p.m. UTC
BUILD_BUG_ON() is a little annoying, since it cannot be used outside
function scope. So one cannot put assertions about the sizeof() a
struct next to the struct definition, but has to hide that in some
more or less arbitrary function.

Since gcc 4.6 (which is now also the required minimum), there is
support for the C11 _Static_assert in all C modes, including gnu89. So
add a simple wrapper for that.

_Static_assert() requires a message argument, which is usually quite
redundant (and I believe that bug got fixed at least in newer C++
standards), but we can easily work around that with a little macro
magic, making it optional.

For example, adding

  static_assert(sizeof(struct printf_spec) == 8);

in vsprintf.c and modifying that struct to violate it, one gets

./include/linux/build_bug.h:78:41: error: static assertion failed: "sizeof(struct printf_spec) == 8"
 #define __static_assert(expr, msg, ...) _Static_assert(expr, "" msg "")

godbolt.org suggests that _Static_assert() has been support by clang
since at least 3.0.0.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 include/linux/build_bug.h | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

Comments

Andrew Morton Feb. 4, 2019, 11:09 p.m. UTC | #1
On Sun,  3 Feb 2019 20:24:00 +0100 Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:

> BUILD_BUG_ON() is a little annoying, since it cannot be used outside
> function scope. So one cannot put assertions about the sizeof() a
> struct next to the struct definition, but has to hide that in some
> more or less arbitrary function.
> 
> Since gcc 4.6 (which is now also the required minimum), there is
> support for the C11 _Static_assert in all C modes, including gnu89. So
> add a simple wrapper for that.
> 
> _Static_assert() requires a message argument, which is usually quite
> redundant (and I believe that bug got fixed at least in newer C++
> standards), but we can easily work around that with a little macro
> magic, making it optional.
> 
> For example, adding
> 
>   static_assert(sizeof(struct printf_spec) == 8);
> 
> in vsprintf.c and modifying that struct to violate it, one gets
> 
> ./include/linux/build_bug.h:78:41: error: static assertion failed: "sizeof(struct printf_spec) == 8"
>  #define __static_assert(expr, msg, ...) _Static_assert(expr, "" msg "")
> 
> godbolt.org suggests that _Static_assert() has been support by clang
> since at least 3.0.0.
> 

It would be (very) nice to actually use this macro in a few places so
it gets its build testing while in -next.
Andrew Morton Feb. 4, 2019, 11:12 p.m. UTC | #2
On Mon, 4 Feb 2019 15:09:16 -0800 Andrew Morton <akpm@linux-foundation.org> wrote:

> On Sun,  3 Feb 2019 20:24:00 +0100 Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
> 
> > BUILD_BUG_ON() is a little annoying, since it cannot be used outside
> > function scope. So one cannot put assertions about the sizeof() a
> > struct next to the struct definition, but has to hide that in some
> > more or less arbitrary function.
> > 
> > Since gcc 4.6 (which is now also the required minimum), there is
> > support for the C11 _Static_assert in all C modes, including gnu89. So
> > add a simple wrapper for that.
> > 
> > _Static_assert() requires a message argument, which is usually quite
> > redundant (and I believe that bug got fixed at least in newer C++
> > standards), but we can easily work around that with a little macro
> > magic, making it optional.
> > 
> > For example, adding
> > 
> >   static_assert(sizeof(struct printf_spec) == 8);
> > 
> > in vsprintf.c and modifying that struct to violate it, one gets
> > 
> > ./include/linux/build_bug.h:78:41: error: static assertion failed: "sizeof(struct printf_spec) == 8"
> >  #define __static_assert(expr, msg, ...) _Static_assert(expr, "" msg "")
> > 
> > godbolt.org suggests that _Static_assert() has been support by clang
> > since at least 3.0.0.
> > 
> 
> It would be (very) nice to actually use this macro in a few places so
> it gets its build testing while in -next.

ie, just about every BUILD_BUG_ON in mm/ could use this.  Let's switch
a few?
Masahiro Yamada Feb. 5, 2019, 8:05 a.m. UTC | #3
On Mon, Feb 4, 2019 at 4:24 AM Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
>
> BUILD_BUG_ON() is a little annoying, since it cannot be used outside
> function scope. So one cannot put assertions about the sizeof() a
> struct next to the struct definition, but has to hide that in some
> more or less arbitrary function.
>
> Since gcc 4.6 (which is now also the required minimum), there is
> support for the C11 _Static_assert in all C modes, including gnu89. So
> add a simple wrapper for that.
>
> _Static_assert() requires a message argument, which is usually quite
> redundant (and I believe that bug got fixed at least in newer C++
> standards), but we can easily work around that with a little macro
> magic, making it optional.
>
> For example, adding
>
>   static_assert(sizeof(struct printf_spec) == 8);
>
> in vsprintf.c and modifying that struct to violate it, one gets
>
> ./include/linux/build_bug.h:78:41: error: static assertion failed: "sizeof(struct printf_spec) == 8"
>  #define __static_assert(expr, msg, ...) _Static_assert(expr, "" msg "")
>
> godbolt.org suggests that _Static_assert() has been support by clang
> since at least 3.0.0.
>
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
>  include/linux/build_bug.h | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>
> diff --git a/include/linux/build_bug.h b/include/linux/build_bug.h
> index faeec7433aab..4bf9ba847b44 100644
> --- a/include/linux/build_bug.h
> +++ b/include/linux/build_bug.h
> @@ -58,4 +58,23 @@
>   */
>  #define BUILD_BUG() BUILD_BUG_ON_MSG(1, "BUILD_BUG failed")
>
> +/**
> + * static_assert - check integer constant expression at build time
> + *
> + * static_assert() is a wrapper for the C11 _Static_assert, with a
> + * little macro magic to make the message optional (defaulting to the
> + * stringification of the tested expression).
> + *
> + * Contrary to BUILD_BUG_ON(), static_assert() can be used at global
> + * scope, but requires the expression to be an integer constant
> + * expression (i.e., it is not enough that __builtin_constant_p() is
> + * true for expr).
> + *
> + * Also note that BUILD_BUG_ON() fails the build if the condition is
> + * true, while static_assert() fails the build if the expression is
> + * false.
> + */
> +#define static_assert(expr, ...) __static_assert(expr, ##__VA_ARGS__, #expr)
> +#define __static_assert(expr, msg, ...) _Static_assert(expr, "" msg "")

What is the "" "" for?


Bikeshed:

There might be room for argument about
where this macro should go.

Another possible place is <linux/compiler.h>
where compiletime_assert() is defined.


Thanks.
Rasmus Villemoes Feb. 5, 2019, 9:38 a.m. UTC | #4
On 05/02/2019 09.05, Masahiro Yamada wrote:
> On Mon, Feb 4, 2019 at 4:24 AM Rasmus Villemoes
> <linux@rasmusvillemoes.dk> wrote:
>>
>> BUILD_BUG_ON() is a little annoying, since it cannot be used outside
>> function scope. So one cannot put assertions about the sizeof() a
>> struct next to the struct definition, but has to hide that in some
>> more or less arbitrary function.
>>
>> Since gcc 4.6 (which is now also the required minimum), there is
>> support for the C11 _Static_assert in all C modes, including gnu89. So
>> add a simple wrapper for that.
>>
>> _Static_assert() requires a message argument, which is usually quite
>> redundant (and I believe that bug got fixed at least in newer C++
>> standards), but we can easily work around that with a little macro
>> magic, making it optional.
>>
>> For example, adding
>>
>>   static_assert(sizeof(struct printf_spec) == 8);
>>
>> in vsprintf.c and modifying that struct to violate it, one gets
>>
>> ./include/linux/build_bug.h:78:41: error: static assertion failed: "sizeof(struct printf_spec) == 8"
>>  #define __static_assert(expr, msg, ...) _Static_assert(expr, "" msg "")
>>
>> godbolt.org suggests that _Static_assert() has been support by clang
>> since at least 3.0.0.
>>
>> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
>> ---
>>  include/linux/build_bug.h | 19 +++++++++++++++++++
>>  1 file changed, 19 insertions(+)
>>
>> diff --git a/include/linux/build_bug.h b/include/linux/build_bug.h
>> index faeec7433aab..4bf9ba847b44 100644
>> --- a/include/linux/build_bug.h
>> +++ b/include/linux/build_bug.h
>> @@ -58,4 +58,23 @@
>>   */
>>  #define BUILD_BUG() BUILD_BUG_ON_MSG(1, "BUILD_BUG failed")
>>
>> +/**
>> + * static_assert - check integer constant expression at build time
>> + *
>> + * static_assert() is a wrapper for the C11 _Static_assert, with a
>> + * little macro magic to make the message optional (defaulting to the
>> + * stringification of the tested expression).
>> + *
>> + * Contrary to BUILD_BUG_ON(), static_assert() can be used at global
>> + * scope, but requires the expression to be an integer constant
>> + * expression (i.e., it is not enough that __builtin_constant_p() is
>> + * true for expr).
>> + *
>> + * Also note that BUILD_BUG_ON() fails the build if the condition is
>> + * true, while static_assert() fails the build if the expression is
>> + * false.
>> + */
>> +#define static_assert(expr, ...) __static_assert(expr, ##__VA_ARGS__, #expr)
>> +#define __static_assert(expr, msg, ...) _Static_assert(expr, "" msg "")
> 
> What is the "" "" for?

Good point. It's a leftover from when I had a fallback-implementation of
_Static_assert for gcc < 4.6, where I wanted to ensure that the second
argument was a string literal, even if my fallback implementation
ignored that argument. Now it's actually a little harmful, because

foobar.c:5:34: error: expected string literal before ‘expected’
 static_assert(sizeof(long) == 8, expected 64 bit machine);

is better than

foobar.c:4:34: error: expected ‘)’ before ‘expected’
 static_assert(sizeof(long) == 8, expected 64 bit machine);

> Bikeshed:
> 
> There might be room for argument about
> where this macro should go.
> 
> Another possible place is <linux/compiler.h>
> where compiletime_assert() is defined.

I'd rather move compiletime_assert to build_bug.h, and rename it so that
it becomes an implementation detail of BUILD_BUG. There are not that
many direct users of compiletime_assert(), and I think we should
standardize on fewer ways of achieving the same thing. static_assert()
for checking ICEs, usable at any scope, and BUILD_BUG_* for checking
that the optimizer is sufficiently smart.

This would also be a step towards another cleanup I'd like to do: make
build_bug.h not depend on compiler.h, because we already have a
dependency in the other direction (ARRAY_SIZE using BUILD_BUG_ON_ZERO).

Rasmus
Rasmus Villemoes Feb. 5, 2019, 9:53 a.m. UTC | #5
On 05/02/2019 00.12, Andrew Morton wrote:
>>
>> It would be (very) nice to actually use this macro in a few places so
>> it gets its build testing while in -next.
> 
> ie, just about every BUILD_BUG_ON in mm/ could use this.  Let's switch
> a few?
> 

Perhaps, but some make sense where they are, e.g. when iterating over
two arrays using one iterator variable, it's fine to assert they have
the same size just above the loop. And, unfortunately, static_assert()
is not quite a drop-in replacement for BUILD_BUG_ON even when the
argument is an ICE due to -Wdeclaration-after-statement.

I'll fix the two BUILD_BUGs I know I've added over the years (one in
vsprintf.c, one in fs/namei.c).

Rasmus
Masahiro Yamada Feb. 5, 2019, 2:06 p.m. UTC | #6
On Tue, Feb 5, 2019 at 6:39 PM Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
>
> On 05/02/2019 09.05, Masahiro Yamada wrote:
> > On Mon, Feb 4, 2019 at 4:24 AM Rasmus Villemoes
> > <linux@rasmusvillemoes.dk> wrote:
> >>
> >> BUILD_BUG_ON() is a little annoying, since it cannot be used outside
> >> function scope. So one cannot put assertions about the sizeof() a
> >> struct next to the struct definition, but has to hide that in some
> >> more or less arbitrary function.
> >>
> >> Since gcc 4.6 (which is now also the required minimum), there is
> >> support for the C11 _Static_assert in all C modes, including gnu89. So
> >> add a simple wrapper for that.
> >>
> >> _Static_assert() requires a message argument, which is usually quite
> >> redundant (and I believe that bug got fixed at least in newer C++
> >> standards), but we can easily work around that with a little macro
> >> magic, making it optional.
> >>
> >> For example, adding
> >>
> >>   static_assert(sizeof(struct printf_spec) == 8);
> >>
> >> in vsprintf.c and modifying that struct to violate it, one gets
> >>
> >> ./include/linux/build_bug.h:78:41: error: static assertion failed: "sizeof(struct printf_spec) == 8"
> >>  #define __static_assert(expr, msg, ...) _Static_assert(expr, "" msg "")
> >>
> >> godbolt.org suggests that _Static_assert() has been support by clang
> >> since at least 3.0.0.
> >>
> >> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> >> ---
> >>  include/linux/build_bug.h | 19 +++++++++++++++++++
> >>  1 file changed, 19 insertions(+)
> >>
> >> diff --git a/include/linux/build_bug.h b/include/linux/build_bug.h
> >> index faeec7433aab..4bf9ba847b44 100644
> >> --- a/include/linux/build_bug.h
> >> +++ b/include/linux/build_bug.h
> >> @@ -58,4 +58,23 @@
> >>   */
> >>  #define BUILD_BUG() BUILD_BUG_ON_MSG(1, "BUILD_BUG failed")
> >>
> >> +/**
> >> + * static_assert - check integer constant expression at build time
> >> + *
> >> + * static_assert() is a wrapper for the C11 _Static_assert, with a
> >> + * little macro magic to make the message optional (defaulting to the
> >> + * stringification of the tested expression).
> >> + *
> >> + * Contrary to BUILD_BUG_ON(), static_assert() can be used at global
> >> + * scope, but requires the expression to be an integer constant
> >> + * expression (i.e., it is not enough that __builtin_constant_p() is
> >> + * true for expr).
> >> + *
> >> + * Also note that BUILD_BUG_ON() fails the build if the condition is
> >> + * true, while static_assert() fails the build if the expression is
> >> + * false.
> >> + */
> >> +#define static_assert(expr, ...) __static_assert(expr, ##__VA_ARGS__, #expr)
> >> +#define __static_assert(expr, msg, ...) _Static_assert(expr, "" msg "")
> >
> > What is the "" "" for?
>
> Good point. It's a leftover from when I had a fallback-implementation of
> _Static_assert for gcc < 4.6, where I wanted to ensure that the second
> argument was a string literal, even if my fallback implementation
> ignored that argument. Now it's actually a little harmful, because
>
> foobar.c:5:34: error: expected string literal before ‘expected’
>  static_assert(sizeof(long) == 8, expected 64 bit machine);
>
> is better than
>
> foobar.c:4:34: error: expected ‘)’ before ‘expected’
>  static_assert(sizeof(long) == 8, expected 64 bit machine);
>
> > Bikeshed:
> >
> > There might be room for argument about
> > where this macro should go.
> >
> > Another possible place is <linux/compiler.h>
> > where compiletime_assert() is defined.
>
> I'd rather move compiletime_assert to build_bug.h, and rename it so that
> it becomes an implementation detail of BUILD_BUG. There are not that
> many direct users of compiletime_assert(), and I think we should
> standardize on fewer ways of achieving the same thing. static_assert()
> for checking ICEs, usable at any scope, and BUILD_BUG_* for checking
> that the optimizer is sufficiently smart.
>
> This would also be a step towards another cleanup I'd like to do: make
> build_bug.h not depend on compiler.h,


Probably, you cannot do this.
compiletime_assert relies on __attribute__((__error__(...)))
that is only supported by GCC.


> because we already have a
> dependency in the other direction (ARRAY_SIZE using BUILD_BUG_ON_ZERO).
>
> Rasmus
Luc Van Oostenryck Feb. 5, 2019, 8:27 p.m. UTC | #7
On Tue, Feb 05, 2019 at 10:53:31AM +0100, Rasmus Villemoes wrote:
> On 05/02/2019 00.12, Andrew Morton wrote:
> >>
> >> It would be (very) nice to actually use this macro in a few places so
> >> it gets its build testing while in -next.
> > 
> > ie, just about every BUILD_BUG_ON in mm/ could use this.  Let's switch
> > a few?
> > 
> 
> Perhaps, but some make sense where they are, e.g. when iterating over
> two arrays using one iterator variable, it's fine to assert they have
> the same size just above the loop. And, unfortunately, static_assert()
> is not quite a drop-in replacement for BUILD_BUG_ON even when the
> argument is an ICE due to -Wdeclaration-after-statement.

Yes, unfortunately.

-- Luc
Nick Desaulniers Feb. 7, 2019, 12:14 a.m. UTC | #8
On Tue, Feb 5, 2019 at 1:38 AM Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
>
> On 05/02/2019 09.05, Masahiro Yamada wrote:
> > On Mon, Feb 4, 2019 at 4:24 AM Rasmus Villemoes
> > <linux@rasmusvillemoes.dk> wrote:
> >> +#define static_assert(expr, ...) __static_assert(expr, ##__VA_ARGS__, #expr)
> >> +#define __static_assert(expr, msg, ...) _Static_assert(expr, "" msg "")
> >
> > What is the "" "" for?
>
> Good point. It's a leftover from when I had a fallback-implementation of
> _Static_assert for gcc < 4.6, where I wanted to ensure that the second
> argument was a string literal, even if my fallback implementation
> ignored that argument. Now it's actually a little harmful, because

I had assumed it was for the "optional" part of the error message,
since whether you pass a string error message or not, the C
preprocessor should join them all together?

>
> foobar.c:5:34: error: expected string literal before ‘expected’
>  static_assert(sizeof(long) == 8, expected 64 bit machine);

Hopefully you'd put `expected` in double quotes?

Note: I'm _very_ happy to see this being added.  Once it's landed, I
too can think of some places that it would work better than
BUILD_BUG_ON().

Patch
diff mbox series

diff --git a/include/linux/build_bug.h b/include/linux/build_bug.h
index faeec7433aab..4bf9ba847b44 100644
--- a/include/linux/build_bug.h
+++ b/include/linux/build_bug.h
@@ -58,4 +58,23 @@ 
  */
 #define BUILD_BUG() BUILD_BUG_ON_MSG(1, "BUILD_BUG failed")
 
+/**
+ * static_assert - check integer constant expression at build time
+ *
+ * static_assert() is a wrapper for the C11 _Static_assert, with a
+ * little macro magic to make the message optional (defaulting to the
+ * stringification of the tested expression).
+ *
+ * Contrary to BUILD_BUG_ON(), static_assert() can be used at global
+ * scope, but requires the expression to be an integer constant
+ * expression (i.e., it is not enough that __builtin_constant_p() is
+ * true for expr).
+ *
+ * Also note that BUILD_BUG_ON() fails the build if the condition is
+ * true, while static_assert() fails the build if the expression is
+ * false.
+ */
+#define static_assert(expr, ...) __static_assert(expr, ##__VA_ARGS__, #expr)
+#define __static_assert(expr, msg, ...) _Static_assert(expr, "" msg "")
+
 #endif	/* _LINUX_BUILD_BUG_H */