[PATCH] utilize _Static_assert() for BUILD_BUG_ON() when the compiler supports it

[PATCH] utilize _Static_assert() for BUILD_BUG_ON() when the compiler supports it

[Jan Beulich]

This makes the resulting diagnostics quite a bit more useful.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

---
 include/linux/bug.h |   16 ++++++++++++++++
 1 file changed, 16 insertions(+)

--- 3.7-rc3/include/linux/bug.h
+++ 3.7-rc3-static-assert/include/linux/bug.h
@@ -27,8 +27,15 @@ struct pt_regs;
    result (of value 0 and type size_t), so the expression can be used
    e.g. in a structure initializer (or where-ever else comma expressions
    aren't permitted). */
+#if __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 6)
+#define BUILD_BUG_ON_ZERO(e) \
+	sizeof(struct { _Static_assert(!(e), "!(" #e ")"); })
+#define BUILD_BUG_ON_NULL(e) \
+	((void *)sizeof(struct { _Static_assert(!(e), "!(" #e ")"); }))
+#else
 #define BUILD_BUG_ON_ZERO(e) (sizeof(struct { int:-!!(e); }))
 #define BUILD_BUG_ON_NULL(e) ((void *)sizeof(struct { int:-!!(e); }))
+#endif
 
 /*
  * BUILD_BUG_ON_INVALID() permits the compiler to check the validity of the
@@ -54,6 +61,15 @@ struct pt_regs;
  */
 #ifndef __OPTIMIZE__
 #define BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
+#elif __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 3)
+#define __build_bug_on_failed(n) __build_bug_on_##n##_failed
+#define _BUILD_BUG_ON(n, condition)				\
+	do {							\
+		extern void __compiletime_error(#condition)	\
+		__build_bug_on_failed(n)(void);			\
+		if (condition) __build_bug_on_failed(n)();	\
+	} while(0)
+#define BUILD_BUG_ON(condition...) _BUILD_BUG_ON(__COUNTER__, ##condition)
 #else
 extern int __build_bug_on_failed;
 #define BUILD_BUG_ON(condition)					\

Re: [PATCH] utilize _Static_assert() for BUILD_BUG_ON() when the compiler supports it

[Rusty Russell]

Jan Beulich <JBeulich@suse.com> writes:
> This makes the resulting diagnostics quite a bit more useful.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Nice.  I'm a bit disappointed we can't just treat _Static_assert() as
void, like:

#define BUILD_BUG_ON_ZERO(e) (_Static_assert(!(e), "!(" #e ")"), 0)

> @@ -54,6 +61,15 @@ struct pt_regs;
>   */
>  #ifndef __OPTIMIZE__
>  #define BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
> +#elif __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 3)
> +#define __build_bug_on_failed(n) __build_bug_on_##n##_failed
> +#define _BUILD_BUG_ON(n, condition)				\
> +	do {							\
> +		extern void __compiletime_error(#condition)	\
> +		__build_bug_on_failed(n)(void);			\
> +		if (condition) __build_bug_on_failed(n)();	\
> +	} while(0)
> +#define BUILD_BUG_ON(condition...) _BUILD_BUG_ON(__COUNTER__, ##condition)
>  #else
>  extern int __build_bug_on_failed;
>  #define BUILD_BUG_ON(condition)					\

Why does this depend on gcc version?  Looks like residue from an attempt
to use _Static_assert here?

Thanks,
Rusty.

Re: [PATCH] utilize _Static_assert() for BUILD_BUG_ON() when the compiler supports it

[Jan Beulich]

>>> On 05.11.12 at 03:19, Rusty Russell <rusty@rustcorp.com.au> wrote:
> Jan Beulich <JBeulich@suse.com> writes:
>> @@ -54,6 +61,15 @@ struct pt_regs;
>>   */
>>  #ifndef __OPTIMIZE__
>>  #define BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
>> +#elif __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 3)
>> +#define __build_bug_on_failed(n) __build_bug_on_##n##_failed
>> +#define _BUILD_BUG_ON(n, condition)				\
>> +	do {							\
>> +		extern void __compiletime_error(#condition)	\
>> +		__build_bug_on_failed(n)(void);			\
>> +		if (condition) __build_bug_on_failed(n)();	\
>> +	} while(0)
>> +#define BUILD_BUG_ON(condition...) _BUILD_BUG_ON(__COUNTER__, ##condition)
>>  #else
>>  extern int __build_bug_on_failed;
>>  #define BUILD_BUG_ON(condition)					\
> 
> Why does this depend on gcc version?  Looks like residue from an attempt
> to use _Static_assert here?

That was the original hope, yes, but didn't work because of
certain uses BUILD_BUG_ON() in inline functions. Hence the
next best solution was to use __compiletime_error(), which
still gives a better diagnostic than what we have so far, but is
- see include/linux/compiler-gcc4.h - available only with gcc 4.3
and newer (simply using the construct even when expanding to
nothing - in include/linux/compile.h - wouldn't have the intended
effect of producing a compiler diagnostic at all, it would get
deferred to link time in _all_ cases, even if the compiler can tell
it's a constant).

Jan

Re: [PATCH] utilize _Static_assert() for BUILD_BUG_ON() when the compiler supports it

[Andrew Morton]

On Fri, 02 Nov 2012 14:47:40 +0000
"Jan Beulich" <JBeulich@suse.com> wrote:

> This makes the resulting diagnostics quite a bit more useful.

So asserts Jan, but to confirm it I would need to download, configure,
build and install a different gcc version, which sounds rather a hassle.

So, please show us an exmple of these diagnostics in the changelog.

> --- 3.7-rc3/include/linux/bug.h
> +++ 3.7-rc3-static-assert/include/linux/bug.h
> @@ -27,8 +27,15 @@ struct pt_regs;
>     result (of value 0 and type size_t), so the expression can be used
>     e.g. in a structure initializer (or where-ever else comma expressions
>     aren't permitted). */
> +#if __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 6)

This sort of logic is normally performed via the
include/linux/compiler*.h system.  And

	grep __GNUC include/linux/*.h

indicates that we've been pretty successful.  Can we do that here too?

(eg: suppose the Intel compiler supports _Static_assert?)

Re: [PATCH] utilize _Static_assert() for BUILD_BUG_ON() when the compiler supports it

[Rusty Russell]

Andrew Morton <akpm@linux-foundation.org> writes:

> On Fri, 02 Nov 2012 14:47:40 +0000
> "Jan Beulich" <JBeulich@suse.com> wrote:
>
>> This makes the resulting diagnostics quite a bit more useful.
>
> So asserts Jan, but to confirm it I would need to download, configure,
> build and install a different gcc version, which sounds rather a hassle.
>
> So, please show us an exmple of these diagnostics in the changelog.
>
>> --- 3.7-rc3/include/linux/bug.h
>> +++ 3.7-rc3-static-assert/include/linux/bug.h
>> @@ -27,8 +27,15 @@ struct pt_regs;
>>     result (of value 0 and type size_t), so the expression can be used
>>     e.g. in a structure initializer (or where-ever else comma expressions
>>     aren't permitted). */
>> +#if __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 6)
>
> This sort of logic is normally performed via the
> include/linux/compiler*.h system.  And
>
> 	grep __GNUC include/linux/*.h
>
> indicates that we've been pretty successful.  Can we do that here too?
>
> (eg: suppose the Intel compiler supports _Static_assert?)

Yeah, there are a lot of goodies here:

_Static_assert:
        We could define __ASSERT_STRUCT_FIELD(e) for this:
        #define BUILD_BUG_ON_ZERO(e) \
                sizeof(struct { __ASSERT_STRUCT_FIELD(e); })

__COUNTER__:
        Used to make a unique id.  Let's define __UNIQUE_ID(prefix) for
        this (using __COUNTER__ or __LINE__). 4.3 and above.

__compiletime_error():
        I blame Arjan for this.  It disappears if not implemented, which
        is just lazy.  BUILD_BUG_ON() does this right, and he didn't fix
        that at the time :(

I'd say we have three patches here, really:

1) Add __ASSERT_STRUCT_FIELD(e) to compiler.h
2) Add __UNIQUE_ID().
3) Use them (I can think of at least one other place for __UNIQUE_ID()).

Jan, do you want the glory?  :) If not, I'll respin.

Thanks,
Rusty.

Re: [PATCH] utilize _Static_assert() for BUILD_BUG_ON() when the compiler supports it

[Jan Beulich]

>>> On 06.11.12 at 02:51, Rusty Russell <rusty@rustcorp.com.au> wrote:
> Andrew Morton <akpm@linux-foundation.org> writes:
> 
>> On Fri, 02 Nov 2012 14:47:40 +0000
>> "Jan Beulich" <JBeulich@suse.com> wrote:
>>
>>> This makes the resulting diagnostics quite a bit more useful.
>>
>> So asserts Jan, but to confirm it I would need to download, configure,
>> build and install a different gcc version, which sounds rather a hassle.
>>
>> So, please show us an exmple of these diagnostics in the changelog.
>>
>>> --- 3.7-rc3/include/linux/bug.h
>>> +++ 3.7-rc3-static-assert/include/linux/bug.h
>>> @@ -27,8 +27,15 @@ struct pt_regs;
>>>     result (of value 0 and type size_t), so the expression can be used
>>>     e.g. in a structure initializer (or where-ever else comma expressions
>>>     aren't permitted). */
>>> +#if __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 6)
>>
>> This sort of logic is normally performed via the
>> include/linux/compiler*.h system.  And
>>
>> 	grep __GNUC include/linux/*.h
>>
>> indicates that we've been pretty successful.  Can we do that here too?
>>
>> (eg: suppose the Intel compiler supports _Static_assert?)
> 
> Yeah, there are a lot of goodies here:
> 
> _Static_assert:
>         We could define __ASSERT_STRUCT_FIELD(e) for this:
>         #define BUILD_BUG_ON_ZERO(e) \
>                 sizeof(struct { __ASSERT_STRUCT_FIELD(e); })

I considered something like this too, but it wouldn't work well: The
diagnostic issued from a failed _Static_assert() would preferably
refer to the original source expression, not an already macro
expanded variant of it. That, however, precludes passing it
through multiple levels of macro expansion. Which would leave
passing e and #e to __ASSERT_STRUCT_FIELD(), but that's again
ugly as it opens the door for someone adding a use where the two
arguments don't match up.

> __COUNTER__:
>         Used to make a unique id.  Let's define __UNIQUE_ID(prefix) for
>         this (using __COUNTER__ or __LINE__). 4.3 and above.

The fallback to __LINE__ is not necessarily correct in all cases,
namely when deep macro expansions result in two instances used
on the same source line, or a use in a header matches line number
wise a second use in another header or the source file.

I considered to option of a fallback (and hence an abstraction in
compiler*.h) when doing this, but I didn't want to do something
that would break in perhaps vary obscure ways.

> __compiletime_error():
>         I blame Arjan for this.  It disappears if not implemented, which
>         is just lazy.  BUILD_BUG_ON() does this right, and he didn't fix
>         that at the time :(

Again, the name of the macro made me not use it, as the obvious
fallback is a link time error. The only thing I would be agreeable to
is something like __buildtime_error().

> I'd say we have three patches here, really:
> 
> 1) Add __ASSERT_STRUCT_FIELD(e) to compiler.h
> 2) Add __UNIQUE_ID().
> 3) Use them (I can think of at least one other place for __UNIQUE_ID()).
> 
> Jan, do you want the glory?  :) If not, I'll respin.

Depending on the answers to the above (i.e. how much of it
actually would need re-doing and re-testing), it may take me
some time to get to this, but beyond that I'm fine with trying
to improve this to everyone's satisfaction.

Jan

Re: [PATCH] utilize _Static_assert() for BUILD_BUG_ON() when the compiler supports it

[Rusty Russell]

Jan Beulich <JBeulich@suse.com> writes:
>>>> On 06.11.12 at 02:51, Rusty Russell <rusty@rustcorp.com.au> wrote:
>> Andrew Morton <akpm@linux-foundation.org> writes:
>> 
>>> On Fri, 02 Nov 2012 14:47:40 +0000
>>> "Jan Beulich" <JBeulich@suse.com> wrote:
>>>
>>>> This makes the resulting diagnostics quite a bit more useful.
>>>
>>> So asserts Jan, but to confirm it I would need to download, configure,
>>> build and install a different gcc version, which sounds rather a hassle.
>>>
>>> So, please show us an exmple of these diagnostics in the changelog.
>>>
>>>> --- 3.7-rc3/include/linux/bug.h
>>>> +++ 3.7-rc3-static-assert/include/linux/bug.h
>>>> @@ -27,8 +27,15 @@ struct pt_regs;
>>>>     result (of value 0 and type size_t), so the expression can be used
>>>>     e.g. in a structure initializer (or where-ever else comma expressions
>>>>     aren't permitted). */
>>>> +#if __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 6)
>>>
>>> This sort of logic is normally performed via the
>>> include/linux/compiler*.h system.  And
>>>
>>> 	grep __GNUC include/linux/*.h
>>>
>>> indicates that we've been pretty successful.  Can we do that here too?
>>>
>>> (eg: suppose the Intel compiler supports _Static_assert?)
>> 
>> Yeah, there are a lot of goodies here:
>> 
>> _Static_assert:
>>         We could define __ASSERT_STRUCT_FIELD(e) for this:
>>         #define BUILD_BUG_ON_ZERO(e) \
>>                 sizeof(struct { __ASSERT_STRUCT_FIELD(e); })
>
> I considered something like this too, but it wouldn't work well: The
> diagnostic issued from a failed _Static_assert() would preferably
> refer to the original source expression, not an already macro
> expanded variant of it. That, however, precludes passing it
> through multiple levels of macro expansion. Which would leave
> passing e and #e to __ASSERT_STRUCT_FIELD(), but that's again
> ugly as it opens the door for someone adding a use where the two
> arguments don't match up.

Good point, but this is going to be pretty damn obscure.  In fact, I
don't think it'll see more than the use here.

>> __COUNTER__:
>>         Used to make a unique id.  Let's define __UNIQUE_ID(prefix) for
>>         this (using __COUNTER__ or __LINE__). 4.3 and above.
>
> The fallback to __LINE__ is not necessarily correct in all cases,
> namely when deep macro expansions result in two instances used
> on the same source line, or a use in a header matches line number
> wise a second use in another header or the source file.
>
> I considered to option of a fallback (and hence an abstraction in
> compiler*.h) when doing this, but I didn't want to do something
> that would break in perhaps vary obscure ways.

I was thinking of my own code in moduleparam.h, but elfnote.h does the
same trick.  In both cases, it'll simply fail compile if we fallback and
__LINE__ isn't unique.

So again, I don't think we're going to see many uses, and no existing
use will bite us.

>> __compiletime_error():
>>         I blame Arjan for this.  It disappears if not implemented, which
>>         is just lazy.  BUILD_BUG_ON() does this right, and he didn't fix
>>         that at the time :(
>
> Again, the name of the macro made me not use it, as the obvious
> fallback is a link time error. The only thing I would be agreeable to
> is something like __buildtime_error().

Yes, it's awkward to use.  Your own usage here is the only correct way
to do it, AFAICT:

> #define __build_bug_on_failed(n) __build_bug_on_##n##_failed
> #define _BUILD_BUG_ON(n, condition)				\
> 	do {							\
> 		extern void __compiletime_error(#condition)	\
> 		__build_bug_on_failed(n)(void);			\
> 		if (condition) __build_bug_on_failed(n)();	\
> 	} while(0)
> #define BUILD_BUG_ON(condition...) _BUILD_BUG_ON(__COUNTER__, ##condition)

The ... is overkill here: sure it's general, but we don't allow that
currently, nor in the other BUILD_BUG_ON() definitions.

So I think this becomes:

#define _BUILD_BUG_ON(undefname, condition)                                    \
        do {                                                                   \
                extern void __compiletime_error(#condition) undefname(void);   \
                if (condition) undefname();                                    \
        } while(0)
#define BUILD_BUG_ON(condition) \
        _BUILD_BUG_ON(__UNIQUE_ID(__build_bug_fail), (condition))

>> I'd say we have three patches here, really:
>> 
>> 1) Add __ASSERT_STRUCT_FIELD(e) to compiler.h
>> 2) Add __UNIQUE_ID().
>> 3) Use them (I can think of at least one other place for __UNIQUE_ID()).
>> 
>> Jan, do you want the glory?  :) If not, I'll respin.
>
> Depending on the answers to the above (i.e. how much of it
> actually would need re-doing and re-testing), it may take me
> some time to get to this, but beyond that I'm fine with trying
> to improve this to everyone's satisfaction.

Yeah, it is kind of a lot of work for a little bikeshed.  But putting
compiler tests in bug.h is pretty icky too.

Below is a __UNIQUE_ID() patch, which I'd like anyway to clean up
moduleparam.h.

Cheers,
Rusty.

Subject: __UNIQUE_ID()

Jan Beulich points out __COUNTER__ (gcc 4.3 and above), so let's use
that to create unique ids.  This is better than __LINE__ which we use
today, so provide a wrapper.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

diff --git a/include/linux/compiler-gcc4.h b/include/linux/compiler-gcc4.h
index 412bc6c..8908821 100644
--- a/include/linux/compiler-gcc4.h
+++ b/include/linux/compiler-gcc4.h
@@ -31,6 +31,8 @@
 
 #define __linktime_error(message) __attribute__((__error__(message)))
 
+#define __UNIQUE_ID(prefix) __PASTE(prefix, __PASTE(__, __COUNTER__))
+
 #if __GNUC_MINOR__ >= 5
 /*
  * Mark a position in code as unreachable.  This can be used to
diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index f430e41..c55ae87 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -42,6 +42,10 @@ extern void __chk_io_ptr(const volatile void __iomem *);
 # define __rcu
 #endif
 
+/* Indirect macros required for expanded argument pasting, eg. __LINE__. */
+#define ___PASTE(a,b) a##b
+#define __PASTE(a,b) ___PASTE(a,b)
+
 #ifdef __KERNEL__
 
 #ifdef __GNUC__
@@ -164,6 +168,11 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
     (typeof(ptr)) (__ptr + (off)); })
 #endif
 
+/* Not-quite-unique ID. */
+#ifndef __UNIQUE_ID
+# define __UNIQUE_ID(prefix) __PASTE(__PASTE(prefix, __), __LINE__)
+#endif
+
 #endif /* __KERNEL__ */
 
 #endif /* __ASSEMBLY__ */

Re: [PATCH] utilize _Static_assert() for BUILD_BUG_ON() when the compiler supports it

[Jan Beulich]

>>> On 07.11.12 at 02:03, Rusty Russell <rusty@rustcorp.com.au> wrote:
> Jan Beulich <JBeulich@suse.com> writes:
>>>>> On 06.11.12 at 02:51, Rusty Russell <rusty@rustcorp.com.au> wrote:
>>> Yeah, there are a lot of goodies here:
>>> 
>>> _Static_assert:
>>>         We could define __ASSERT_STRUCT_FIELD(e) for this:
>>>         #define BUILD_BUG_ON_ZERO(e) \
>>>                 sizeof(struct { __ASSERT_STRUCT_FIELD(e); })
>>
>> I considered something like this too, but it wouldn't work well: The
>> diagnostic issued from a failed _Static_assert() would preferably
>> refer to the original source expression, not an already macro
>> expanded variant of it. That, however, precludes passing it
>> through multiple levels of macro expansion. Which would leave
>> passing e and #e to __ASSERT_STRUCT_FIELD(), but that's again
>> ugly as it opens the door for someone adding a use where the two
>> arguments don't match up.
> 
> Good point, but this is going to be pretty damn obscure.  In fact, I
> don't think it'll see more than the use here.

Okay, so I'll go with something like that then.

>>> __COUNTER__:
>>>         Used to make a unique id.  Let's define __UNIQUE_ID(prefix) for
>>>         this (using __COUNTER__ or __LINE__). 4.3 and above.
>>
>> The fallback to __LINE__ is not necessarily correct in all cases,
>> namely when deep macro expansions result in two instances used
>> on the same source line, or a use in a header matches line number
>> wise a second use in another header or the source file.
>>
>> I considered to option of a fallback (and hence an abstraction in
>> compiler*.h) when doing this, but I didn't want to do something
>> that would break in perhaps vary obscure ways.
> 
> I was thinking of my own code in moduleparam.h, but elfnote.h does the
> same trick.  In both cases, it'll simply fail compile if we fallback and
> __LINE__ isn't unique.
> 
> So again, I don't think we're going to see many uses, and no existing
> use will bite us.

That's exactly the point - we can't know (because there's no
guarantee there aren't - or won't be by the time it might get
merged - any two conflicting uses of BUILD_BUG_ON...().

>>> __compiletime_error():
>>>         I blame Arjan for this.  It disappears if not implemented, which
>>>         is just lazy.  BUILD_BUG_ON() does this right, and he didn't fix
>>>         that at the time :(
>>
>> Again, the name of the macro made me not use it, as the obvious
>> fallback is a link time error. The only thing I would be agreeable to
>> is something like __buildtime_error().
> 
> Yes, it's awkward to use.  Your own usage here is the only correct way
> to do it, AFAICT:
> 
>> #define __build_bug_on_failed(n) __build_bug_on_##n##_failed
>> #define _BUILD_BUG_ON(n, condition)				\
>> 	do {							\
>> 		extern void __compiletime_error(#condition)	\
>> 		__build_bug_on_failed(n)(void);			\
>> 		if (condition) __build_bug_on_failed(n)();	\
>> 	} while(0)

Actually, I just noticed that __linktime_error() really is a compile
time error when gcc supports __attribute__(__error__()), so
probably using that one instead of __compiletime_error() here
would be the cleaner approach.

The one thing puzzling me here is that __linktime_error() gets
defined even if __CHECKER__ isn't defined, while
__compiletime_error() doesn't - an apparent inconsistency
between 746a2a838deec3ef86ef6b7c3edd4207b9a351aa
(Kosaki?) and 1399ff86f2a2bbacbbe68fa00c5f8c752b344723
(David?), with apparently the latter being too lax, as it only
introduced a use inside a !__CHECKER__ section.

>> #define BUILD_BUG_ON(condition...) _BUILD_BUG_ON(__COUNTER__, ##condition)
> 
> The ... is overkill here: sure it's general, but we don't allow that
> currently, nor in the other BUILD_BUG_ON() definitions.

This may be a leftover from previous failed attempts; I don't
see a reason why it should stay.

> So I think this becomes:
> 
> #define _BUILD_BUG_ON(undefname, condition)                                    \
>         do {                                                                   \
>                 extern void __compiletime_error(#condition) undefname(void);   \
>                 if (condition) undefname();                                    \
>         } while(0)
> #define BUILD_BUG_ON(condition) \
>         _BUILD_BUG_ON(__UNIQUE_ID(__build_bug_fail), (condition))

Yes, subject to the fallback issue.

> Subject: __UNIQUE_ID()
> 
> Jan Beulich points out __COUNTER__ (gcc 4.3 and above), so let's use
> that to create unique ids.  This is better than __LINE__ which we use
> today, so provide a wrapper.
> 
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Acked-by: Jan Beulich <jbeulich@suse.com>

> diff --git a/include/linux/compiler-gcc4.h b/include/linux/compiler-gcc4.h
> index 412bc6c..8908821 100644
> --- a/include/linux/compiler-gcc4.h
> +++ b/include/linux/compiler-gcc4.h
> @@ -31,6 +31,8 @@
>  
>  #define __linktime_error(message) __attribute__((__error__(message)))
>  
> +#define __UNIQUE_ID(prefix) __PASTE(prefix, __PASTE(__, __COUNTER__))
> +
>  #if __GNUC_MINOR__ >= 5
>  /*
>   * Mark a position in code as unreachable.  This can be used to
> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> index f430e41..c55ae87 100644
> --- a/include/linux/compiler.h
> +++ b/include/linux/compiler.h
> @@ -42,6 +42,10 @@ extern void __chk_io_ptr(const volatile void __iomem *);
>  # define __rcu
>  #endif
>  
> +/* Indirect macros required for expanded argument pasting, eg. __LINE__. */
> +#define ___PASTE(a,b) a##b
> +#define __PASTE(a,b) ___PASTE(a,b)
> +
>  #ifdef __KERNEL__
>  
>  #ifdef __GNUC__
> @@ -164,6 +168,11 @@ void ftrace_likely_update(struct ftrace_branch_data *f, 
> int val, int expect);
>      (typeof(ptr)) (__ptr + (off)); })
>  #endif
>  
> +/* Not-quite-unique ID. */
> +#ifndef __UNIQUE_ID
> +# define __UNIQUE_ID(prefix) __PASTE(__PASTE(prefix, __), __LINE__)
> +#endif
> +
>  #endif /* __KERNEL__ */
>  
>  #endif /* __ASSEMBLY__ */

Re: [PATCH] utilize _Static_assert() for BUILD_BUG_ON() when the compiler supports it

[Rusty Russell]

Jan Beulich <JBeulich@suse.com> writes:
>>>> On 07.11.12 at 02:03, Rusty Russell <rusty@rustcorp.com.au> wrote:
>>>> __COUNTER__:
>>>>         Used to make a unique id.  Let's define __UNIQUE_ID(prefix) for
>>>>         this (using __COUNTER__ or __LINE__). 4.3 and above.
>>>
>>> The fallback to __LINE__ is not necessarily correct in all cases,
>>> namely when deep macro expansions result in two instances used
>>> on the same source line, or a use in a header matches line number
>>> wise a second use in another header or the source file.
>>>
>>> I considered to option of a fallback (and hence an abstraction in
>>> compiler*.h) when doing this, but I didn't want to do something
>>> that would break in perhaps vary obscure ways.
>> 
>> I was thinking of my own code in moduleparam.h, but elfnote.h does the
>> same trick.  In both cases, it'll simply fail compile if we fallback and
>> __LINE__ isn't unique.
>> 
>> So again, I don't think we're going to see many uses, and no existing
>> use will bite us.
>
> That's exactly the point - we can't know (because there's no
> guarantee there aren't - or won't be by the time it might get
> merged - any two conflicting uses of BUILD_BUG_ON...().

Conflicting BUILD_BUG_ON() is completely harmless: two identical
undefines are OK.  The other cases cause a compile error, and one which
is trivial to fix, and I'm happy to fix it if it does.

I've never seen a construction in the kernel which would silently break
if __UNIQUE_ID() isn't actually unique.  That makes sense, because you
if the symbol wasn't exposed you wouldn't need a __UNIQUE_ID.  And if
it's exposed, the compiler will give an error on multiple definition.

(Obviously you can't have non-static __UNIQUE_ID() because it's only
ever unique across a single gcc compile).
>>> #define __build_bug_on_failed(n) __build_bug_on_##n##_failed
>>> #define _BUILD_BUG_ON(n, condition)				\
>>> 	do {							\
>>> 		extern void __compiletime_error(#condition)	\
>>> 		__build_bug_on_failed(n)(void);			\
>>> 		if (condition) __build_bug_on_failed(n)();	\
>>> 	} while(0)
>
> Actually, I just noticed that __linktime_error() really is a compile
> time error when gcc supports __attribute__(__error__()), so
> probably using that one instead of __compiletime_error() here
> would be the cleaner approach.
>
> The one thing puzzling me here is that __linktime_error() gets
> defined even if __CHECKER__ isn't defined, while
> __compiletime_error() doesn't - an apparent inconsistency
> between 746a2a838deec3ef86ef6b7c3edd4207b9a351aa
> (Kosaki?) and 1399ff86f2a2bbacbbe68fa00c5f8c752b344723
> (David?), with apparently the latter being too lax, as it only
> introduced a use inside a !__CHECKER__ section.

Yes, __linktime_error() makes no sense, since it's identical to
__compiletime_error().  It's also a terrible name.  Let's kill it.

I hadn't noticed BUILD_BUG.  We should fix that as discussed here, and
simply make BUILD_BUG_ON() call it.

>> Subject: __UNIQUE_ID()
>> 
>> Jan Beulich points out __COUNTER__ (gcc 4.3 and above), so let's use
>> that to create unique ids.  This is better than __LINE__ which we use
>> today, so provide a wrapper.
>> 
>> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
>
> Acked-by: Jan Beulich <jbeulich@suse.com>

Thanks,
Rusty.

Re: [PATCH] utilize _Static_assert() for BUILD_BUG_ON() when the compiler supports it

[Daniel Santos]

Wow, it's really easy to miss parallel development on the same issue.  
Sorry for my late response to this thread.  I started another thread 
addressing these issues (as well as a few others) back in September 
(https://lkml.org/lkml/2012/9/28/1136).  I've finally gotten ACKs from 
maintainers with v6 of the patches (here 
https://lkml.org/lkml/2012/11/20/621) and I'm just waiting for 3.8-rc1 
to re-submit them.  I actually submitted these patches back in June as 
part of a larger patch set, but broke it apart in September (I had way 
to many changes for one patch set)


On 11/07/2012 05:24 PM, Rusty Russell wrote:
> Jan Beulich<JBeulich@suse.com>  writes:
>>>>> On 07.11.12 at 02:03, Rusty Russell<rusty@rustcorp.com.au>  wrote:
>>>>> __COUNTER__:
>>>>>          Used to make a unique id.  Let's define __UNIQUE_ID(prefix) for
>>>>>          this (using __COUNTER__ or __LINE__). 4.3 and above.
>>>> The fallback to __LINE__ is not necessarily correct in all cases,
>>>> namely when deep macro expansions result in two instances used
>>>> on the same source line, or a use in a header matches line number
>>>> wise a second use in another header or the source file.
>>>>
>>>> I considered to option of a fallback (and hence an abstraction in
>>>> compiler*.h) when doing this, but I didn't want to do something
>>>> that would break in perhaps vary obscure ways.
>>> I was thinking of my own code in moduleparam.h, but elfnote.h does the
>>> same trick.  In both cases, it'll simply fail compile if we fallback and
>>> __LINE__ isn't unique.
>>>
>>> So again, I don't think we're going to see many uses, and no existing
>>> use will bite us.
>> That's exactly the point - we can't know (because there's no
>> guarantee there aren't - or won't be by the time it might get
>> merged - any two conflicting uses of BUILD_BUG_ON...().
> Conflicting BUILD_BUG_ON() is completely harmless: two identical
> undefines are OK.  The other cases cause a compile error, and one which
> is trivial to fix, and I'm happy to fix it if it does.
>
> I've never seen a construction in the kernel which would silently break
> if __UNIQUE_ID() isn't actually unique.  That makes sense, because you
> if the symbol wasn't exposed you wouldn't need a __UNIQUE_ID.  And if
> it's exposed, the compiler will give an error on multiple definition.
>
> (Obviously you can't have non-static __UNIQUE_ID() because it's only
> ever unique across a single gcc compile).
I was considering __COUNTER__, but in the end, others felt that the 
inconsistency could cause problems later.  However, with the 
__compiletime_error function attribute, the result will only be that the 
actual error message emitted will be incorrect, since the first error 
attribute assigned to the extern function will be used.  Thus, compiling 
this code:

{extern void func(void) __attribute__((error("hello")));}
{extern void func(void) __attribute__((error("goodbye"))); func();}

would result in an error with the message "hello".

>>>> #define __build_bug_on_failed(n) __build_bug_on_##n##_failed
>>>> #define _BUILD_BUG_ON(n, condition)				\
>>>> 	do {							\
>>>> 		extern void __compiletime_error(#condition)	\
>>>> 		__build_bug_on_failed(n)(void);			\
>>>> 		if (condition) __build_bug_on_failed(n)();	\
>>>> 	} while(0)
>> Actually, I just noticed that __linktime_error() really is a compile
>> time error when gcc supports __attribute__(__error__()), so
>> probably using that one instead of __compiletime_error() here
>> would be the cleaner approach.
>>
>> The one thing puzzling me here is that __linktime_error() gets
>> defined even if __CHECKER__ isn't defined, while
>> __compiletime_error() doesn't - an apparent inconsistency
>> between 746a2a838deec3ef86ef6b7c3edd4207b9a351aa
>> (Kosaki?) and 1399ff86f2a2bbacbbe68fa00c5f8c752b344723
>> (David?), with apparently the latter being too lax, as it only
>> introduced a use inside a !__CHECKER__ section.
> Yes, __linktime_error() makes no sense, since it's identical to
> __compiletime_error().  It's also a terrible name.  Let's kill it.
Definitely.  My patch set kills that sucker too.
>
> I hadn't noticed BUILD_BUG.  We should fix that as discussed here, and
> simply make BUILD_BUG_ON() call it.
>
>>> Subject: __UNIQUE_ID()
>>>
>>> Jan Beulich points out __COUNTER__ (gcc 4.3 and above), so let's use
>>> that to create unique ids.  This is better than __LINE__ which we use
>>> today, so provide a wrapper.
>>>
>>> Signed-off-by: Rusty Russell<rusty@rustcorp.com.au>
>> Acked-by: Jan Beulich<jbeulich@suse.com>
> Thanks,
> Rusty.
https://lkml.org/lkml/2012/9/28/1136

Re: [PATCH] utilize _Static_assert() for BUILD_BUG_ON() when the compiler supports it

[Jan Beulich]

>>> On 13.12.12 at 01:29, Daniel Santos <danielfsantos@att.net> wrote:
> Wow, it's really easy to miss parallel development on the same issue.  
> Sorry for my late response to this thread.  I started another thread 
> addressing these issues (as well as a few others) back in September 
> (https://lkml.org/lkml/2012/9/28/1136).  I've finally gotten ACKs from 
> maintainers with v6 of the patches (here 
> https://lkml.org/lkml/2012/11/20/621) and I'm just waiting for 3.8-rc1 
> to re-submit them.  I actually submitted these patches back in June as 
> part of a larger patch set, but broke it apart in September (I had way 
> to many changes for one patch set)

Since yours is apparently ready to go in, but doesn't use
_Static_assert, I guess I'll wait for it to appear until I re-work
whatever might be left to actually make use of _Static_assert.

Jan

Re: [PATCH] utilize _Static_assert() for BUILD_BUG_ON() when the compiler supports it

[Daniel Santos]

On 12/13/2012 03:43 AM, Jan Beulich wrote:
>>>> On 13.12.12 at 01:29, Daniel Santos<danielfsantos@att.net>  wrote:
>> Wow, it's really easy to miss parallel development on the same issue.
>> Sorry for my late response to this thread.  I started another thread
>> addressing these issues (as well as a few others) back in September
>> (https://lkml.org/lkml/2012/9/28/1136).  I've finally gotten ACKs from
>> maintainers with v6 of the patches (here
>> https://lkml.org/lkml/2012/11/20/621) and I'm just waiting for 3.8-rc1
>> to re-submit them.  I actually submitted these patches back in June as
>> part of a larger patch set, but broke it apart in September (I had way
>> to many changes for one patch set)
> Since yours is apparently ready to go in, but doesn't use
> _Static_assert, I guess I'll wait for it to appear until I re-work
> whatever might be left to actually make use of _Static_assert.
>
> Jan
>
Interesting! They've enabled it by default (I suppose as an extension?) 
in every standard (except -pedantic of course). One minor draw-back is 
that it appears to enjoy escaping tickmarks in the error message.  I've 
opened a bug for it (http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55678)

But realistically, the "compiletime_assert" macro I wrote in compiler.h 
can be renamed to "static_assert", analogous to C11's static_assert from 
assert.h (§7.2 of C11) and it can expand to the _Static_assert keyword, 
when that is available.

Something else that I didn't consider too much before was support for 
compiling -O0 or -O1, which will cause many expressions that are 
otherwise evaluated as compile-time constants to become non-constant and 
result in failed assertions.  This isn't anything new however, building 
-O0 has been broken for quite some time, but I presume it could help 
some development of out of tree modules.

Re: [PATCH] utilize _Static_assert() for BUILD_BUG_ON() when the compiler supports it

[Daniel Santos]

On 11/06/2012 03:23 AM, Jan Beulich wrote:
>> This sort of logic is normally performed via the
>> include/linux/compiler*.h system.  And
>>
>> 	grep __GNUC include/linux/*.h
>>
>> indicates that we've been pretty successful.  Can we do that here too?
>>
>> (eg: suppose the Intel compiler supports _Static_assert?)
>> Yeah, there are a lot of goodies here:
>>
>> _Static_assert:
>>          We could define __ASSERT_STRUCT_FIELD(e) for this:
>>          #define BUILD_BUG_ON_ZERO(e) \
>>                  sizeof(struct { __ASSERT_STRUCT_FIELD(e); })
> I considered something like this too, but it wouldn't work well: The
> diagnostic issued from a failed _Static_assert() would preferably
> refer to the original source expression, not an already macro
> expanded variant of it. That, however, precludes passing it
> through multiple levels of macro expansion. Which would leave
> passing e and #e to __ASSERT_STRUCT_FIELD(), but that's again
> ugly as it opens the door for someone adding a use where the two
> arguments don't match up.
I was under the assumption that _Static_assert doesn't exist unless you 
are using -std=c11 or -std=gnu11, but I admit I didn't look into it 
much. I'm very glad it was added to the standard. IMO, it would be 
better to use such a feature (presuming it behaves well) when it is 
available but, of course, we still need the best possible functionality 
when it isn't.

In either case, the new BUILD_BUG_ON macro simply stringifies the 
condition, so it will emit an error indicating the exact expression that 
failed. If you pass it a macro, macro expansion will not be performed 
prior to stringification.
>
>> __compiletime_error():
>>          I blame Arjan for this.  It disappears if not implemented, which
>>          is just lazy.  BUILD_BUG_ON() does this right, and he didn't fix
>>          that at the time :(
> Again, the name of the macro made me not use it, as the obvious
> fallback is a link time error. The only thing I would be agreeable to
> is something like __buildtime_error().

Yeah, I agree, since we speak more in terms of a "build" rather than 
compiling.  But I didn't mess with it in my patches.

>
>> I'd say we have three patches here, really:
>>
>> 1) Add __ASSERT_STRUCT_FIELD(e) to compiler.h
There are actually a number of reasons for having multiple mechanisms to 
break the build.  For instance, BUILD_BUG_ON_ZERO is used outside of 
function bodies, where a compound gcc expression ({expr; expr;}) isn't 
permitted.
>> 2) Add __UNIQUE_ID().
>> 3) Use them (I can think of at least one other place for __UNIQUE_ID()).
>>
>> Jan, do you want the glory?  :) If not, I'll respin.
> Depending on the answers to the above (i.e. how much of it
> actually would need re-doing and re-testing), it may take me
> some time to get to this, but beyond that I'm fine with trying
> to improve this to everyone's satisfaction.
Personally, I would rather we start a cpputil.h (or some such) to 
encapsulate all of our preprocessor kludgery, err, I mean wizardry.  
There must be 14 thousand paste macros in the kernel.  We could move 
stringify into it as well, but I suppose that's another issue.

Daniel