__compiletime_assert_fallback() is supposed to stop building earlier
by using the negative-array-size method in case the compiler does not
support "error" attribute, but has never worked like that.
You can simply try:
BUILD_BUG_ON(1);
GCC immediately terminates the build, but Clang does not report
anything because Clang does not support the "error" attribute now.
It will later fail at link time, but __compiletime_assert_fallback()
is not working at least.
The root cause is commit 1d6a0d19c855 ("bug.h: prevent double evaluation
of `condition' in BUILD_BUG_ON"). Prior to that commit, BUILD_BUG_ON()
was checked by the negative-array-size method *and* the link-time trick.
Since that commit, the negative-array-size is not effective because
'__cond' is no longer constant. As the comment in <linux/build_bug.h>
says, GCC (and Clang as well) only emits the error for obvious cases.
When '__cond' is a variable,
((void)sizeof(char[1 - 2 * __cond]))
... is not obvious for the compiler to know the array size is negative.
Reverting that commit would break BUILD_BUG() because negative-size-array
is evaluated before the code is optimized out.
Let's give up __compiletime_assert_fallback(). This commit does not
change the current behavior since it just rips off the useless code.
Signed-off-by: Masahiro Yamada <[email protected]>
Reviewed-by: Kees Cook <[email protected]>
Reviewed-by: Nick Desaulniers <[email protected]>
---
Changes in v2:
- Rebase
include/linux/compiler.h | 17 +----------------
1 file changed, 1 insertion(+), 16 deletions(-)
diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 681d866..87c776c 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -314,29 +314,14 @@ static inline void *offset_to_ptr(const int *off)
#endif
#ifndef __compiletime_error
# define __compiletime_error(message)
-/*
- * Sparse complains of variable sized arrays due to the temporary variable in
- * __compiletime_assert. Unfortunately we can't just expand it out to make
- * sparse see a constant array size without breaking compiletime_assert on old
- * versions of GCC (e.g. 4.2.4), so hide the array from sparse altogether.
- */
-# ifndef __CHECKER__
-# define __compiletime_error_fallback(condition) \
- do { ((void)sizeof(char[1 - 2 * condition])); } while (0)
-# endif
-#endif
-#ifndef __compiletime_error_fallback
-# define __compiletime_error_fallback(condition) do { } while (0)
#endif
#ifdef __OPTIMIZE__
# define __compiletime_assert(condition, msg, prefix, suffix) \
do { \
- int __cond = !(condition); \
extern void prefix ## suffix(void) __compiletime_error(msg); \
- if (__cond) \
+ if (!(condition)) \
prefix ## suffix(); \
- __compiletime_error_fallback(__cond); \
} while (0)
#else
# define __compiletime_assert(condition, msg, prefix, suffix) do { } while (0)
--
2.7.4
Hello Masahiro,
On 08/25/2018 01:16 PM, Masahiro Yamada wrote:
> __compiletime_assert_fallback() is supposed to stop building earlier
> by using the negative-array-size method in case the compiler does not
> support "error" attribute, but has never worked like that.
>
> You can simply try:
>
> BUILD_BUG_ON(1);
>
> GCC immediately terminates the build, but Clang does not report
> anything because Clang does not support the "error" attribute now.
> It will later fail at link time, but __compiletime_assert_fallback()
> is not working at least.
>
> The root cause is commit 1d6a0d19c855 ("bug.h: prevent double evaluation
> of `condition' in BUILD_BUG_ON").
I didn't really think this particular patch was necessary, but it was
requested that I eliminate double evaluation and I didn't feel like
arguing it at the time. :) In my philosophy however, one should *never*
use an expression with side effects in any type of assert.
> Prior to that commit, BUILD_BUG_ON()
> was checked by the negative-array-size method *and* the link-time trick.
> Since that commit, the negative-array-size is not effective because
> '__cond' is no longer constant.
Now we're back to the question of "what do you mean by 'constant'"? If
you mean a C constant expression (as defined in the C standard) than
almost none of this code fits that criteria. For these compile-time
assertions to work, we are concerned with the data flow analysis and
constant propagation performed by the compiler during optimization. You
will notice in include/linux/compiler.h that __compiletime_assert is a
no-op when __OPTIMIZE__ is not defined.
> As the comment in <linux/build_bug.h>
> says, GCC (and Clang as well) only emits the error for obvious cases.
>
> When '__cond' is a variable,
>
> ((void)sizeof(char[1 - 2 * __cond]))
>
> ... is not obvious for the compiler to know the array size is negative.
>
> Reverting that commit would break BUILD_BUG() because negative-size-array
> is evaluated before the code is optimized out.
>
> Let's give up __compiletime_assert_fallback(). This commit does not
> change the current behavior since it just rips off the useless code.
Clang is not the only target audience of
__compiletime_assert_fallback(). Instead of ripping out something that
may benefit builds with gcc 4.2 and earlier, why not override its
definition in compiler-clang.h with something that will break the build
for Clang? It would need an #ifndef __compiletime_error_fallback here
though.
> Signed-off-by: Masahiro Yamada <[email protected]>
> Reviewed-by: Kees Cook <[email protected]>
> Reviewed-by: Nick Desaulniers <[email protected]>
> ---
>
> Changes in v2:
> - Rebase
>
> include/linux/compiler.h | 17 +----------------
> 1 file changed, 1 insertion(+), 16 deletions(-)
>
> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> index 681d866..87c776c 100644
> --- a/include/linux/compiler.h
> +++ b/include/linux/compiler.h
> @@ -314,29 +314,14 @@ static inline void *offset_to_ptr(const int *off)
> #endif
> #ifndef __compiletime_error
> # define __compiletime_error(message)
> -/*
> - * Sparse complains of variable sized arrays due to the temporary variable in
> - * __compiletime_assert. Unfortunately we can't just expand it out to make
> - * sparse see a constant array size without breaking compiletime_assert on old
> - * versions of GCC (e.g. 4.2.4), so hide the array from sparse altogether.
> - */
> -# ifndef __CHECKER__
> -# define __compiletime_error_fallback(condition) \
> - do { ((void)sizeof(char[1 - 2 * condition])); } while (0)
> -# endif
> -#endif
> -#ifndef __compiletime_error_fallback
> -# define __compiletime_error_fallback(condition) do { } while (0)
> #endif
>
> #ifdef __OPTIMIZE__
> # define __compiletime_assert(condition, msg, prefix, suffix) \
> do { \
> - int __cond = !(condition); \
> extern void prefix ## suffix(void) __compiletime_error(msg); \
> - if (__cond) \
> + if (!(condition)) \
> prefix ## suffix(); \
> - __compiletime_error_fallback(__cond); \
> } while (0)
> #else
> # define __compiletime_assert(condition, msg, prefix, suffix) do { } while (0)
To give any more meaningful feedback I think I will need to experiment
with Clang, older GCC versions and icc. It occurred to me that I should
probably clean up and publish my __builtin_constant_p test program and
also generate results for more recent compilers. I can extend it to
test various negative sized array constructs and it could help inform
this decision.
IMO, the most ideal solution would be a set of C2x (or future)
extensions providing something similar to C++'s constexpr, GCC's
__builtin_constant_p and our BUILD_BUG_ON. This would cross deeply into
territory traditionally considered to belong to the implementation, so
it's no small request. A lot would have to be resolved for it to work
in the standard.
Daniel
On Mon, Aug 27, 2018 at 1:05 PM Daniel Santos <[email protected]> wrote:
>
> Hello Masahiro,
>
>
> On 08/25/2018 01:16 PM, Masahiro Yamada wrote:
> > __compiletime_assert_fallback() is supposed to stop building earlier
> > by using the negative-array-size method in case the compiler does not
> > support "error" attribute, but has never worked like that.
> >
> > You can simply try:
> >
> > BUILD_BUG_ON(1);
> >
> > GCC immediately terminates the build, but Clang does not report
> > anything because Clang does not support the "error" attribute now.
> > It will later fail at link time, but __compiletime_assert_fallback()
> > is not working at least.
> >
> > The root cause is commit 1d6a0d19c855 ("bug.h: prevent double evaluation
> > of `condition' in BUILD_BUG_ON").
>
> I didn't really think this particular patch was necessary, but it was
> requested that I eliminate double evaluation and I didn't feel like
> arguing it at the time. :) In my philosophy however, one should *never*
> use an expression with side effects in any type of assert.
>
> > Prior to that commit, BUILD_BUG_ON()
> > was checked by the negative-array-size method *and* the link-time trick.
> > Since that commit, the negative-array-size is not effective because
> > '__cond' is no longer constant.
>
> Now we're back to the question of "what do you mean by 'constant'"? If
> you mean a C constant expression (as defined in the C standard) than
> almost none of this code fits that criteria. For these compile-time
> assertions to work, we are concerned with the data flow analysis and
> constant propagation performed by the compiler during optimization. You
> will notice in include/linux/compiler.h that __compiletime_assert is a
> no-op when __OPTIMIZE__ is not defined.
Depending on optimizations for static assertions sounds problematic.
>
> > As the comment in <linux/build_bug.h>
> > says, GCC (and Clang as well) only emits the error for obvious cases.
> >
> > When '__cond' is a variable,
> >
> > ((void)sizeof(char[1 - 2 * __cond]))
> >
> > ... is not obvious for the compiler to know the array size is negative.
> >
> > Reverting that commit would break BUILD_BUG() because negative-size-array
> > is evaluated before the code is optimized out.
> >
> > Let's give up __compiletime_assert_fallback(). This commit does not
> > change the current behavior since it just rips off the useless code.
>
> Clang is not the only target audience of
> __compiletime_assert_fallback(). Instead of ripping out something that
> may benefit builds with gcc 4.2 and earlier, why not override its
Note that with commit cafa0010cd51 ("Raise the minimum required gcc
version to 4.6") that gcc < 4.6 is irrelevant.
> definition in compiler-clang.h with something that will break the build
> for Clang? It would need an #ifndef __compiletime_error_fallback here
> though.
>
> > Signed-off-by: Masahiro Yamada <[email protected]>
> > Reviewed-by: Kees Cook <[email protected]>
> > Reviewed-by: Nick Desaulniers <[email protected]>
> > ---
> >
> > Changes in v2:
> > - Rebase
> >
> > include/linux/compiler.h | 17 +----------------
> > 1 file changed, 1 insertion(+), 16 deletions(-)
> >
> > diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> > index 681d866..87c776c 100644
> > --- a/include/linux/compiler.h
> > +++ b/include/linux/compiler.h
> > @@ -314,29 +314,14 @@ static inline void *offset_to_ptr(const int *off)
> > #endif
> > #ifndef __compiletime_error
> > # define __compiletime_error(message)
> > -/*
> > - * Sparse complains of variable sized arrays due to the temporary variable in
> > - * __compiletime_assert. Unfortunately we can't just expand it out to make
> > - * sparse see a constant array size without breaking compiletime_assert on old
> > - * versions of GCC (e.g. 4.2.4), so hide the array from sparse altogether.
> > - */
> > -# ifndef __CHECKER__
> > -# define __compiletime_error_fallback(condition) \
> > - do { ((void)sizeof(char[1 - 2 * condition])); } while (0)
> > -# endif
> > -#endif
> > -#ifndef __compiletime_error_fallback
> > -# define __compiletime_error_fallback(condition) do { } while (0)
> > #endif
> >
> > #ifdef __OPTIMIZE__
> > # define __compiletime_assert(condition, msg, prefix, suffix) \
> > do { \
> > - int __cond = !(condition); \
> > extern void prefix ## suffix(void) __compiletime_error(msg); \
> > - if (__cond) \
> > + if (!(condition)) \
> > prefix ## suffix(); \
> > - __compiletime_error_fallback(__cond); \
> > } while (0)
> > #else
> > # define __compiletime_assert(condition, msg, prefix, suffix) do { } while (0)
>
> To give any more meaningful feedback I think I will need to experiment
> with Clang, older GCC versions and icc. It occurred to me that I should
> probably clean up and publish my __builtin_constant_p test program and
> also generate results for more recent compilers. I can extend it to
> test various negative sized array constructs and it could help inform
> this decision.
>
> IMO, the most ideal solution would be a set of C2x (or future)
> extensions providing something similar to C++'s constexpr, GCC's
> __builtin_constant_p and our BUILD_BUG_ON. This would cross deeply into
Note that __builtin_constant_p is a wild beast with lots of edge
cases, and dependencies on compiler and optimization level. I'm
trying to sort out some of these differences right now with llvm
developers.
> territory traditionally considered to belong to the implementation, so
> it's no small request. A lot would have to be resolved for it to work
> in the standard.
>
> Daniel
--
Thanks,
~Nick Desaulniers
Hello Nick,
On 08/27/2018 03:09 PM, Nick Desaulniers wrote:
>> Now we're back to the question of "what do you mean by 'constant'"? If
>> you mean a C constant expression (as defined in the C standard) than
>> almost none of this code fits that criteria. For these compile-time
>> assertions to work, we are concerned with the data flow analysis and
>> constant propagation performed by the compiler during optimization. You
>> will notice in include/linux/compiler.h that __compiletime_assert is a
>> no-op when __OPTIMIZE__ is not defined.
> Depending on optimizations for static assertions sounds problematic.
(with my best Palpatine voice) It is unavoidable.
Actually it's theoretically possible, but the compiler would have to do
something akin to copying it's control flow graph et. al, run -O2-ish
optimizations, perform the static assertions and then throw away the
optimized control flow graph and emit code based upon the original.
>>> As the comment in <linux/build_bug.h>
>>> says, GCC (and Clang as well) only emits the error for obvious cases.
>>>
>>> When '__cond' is a variable,
>>>
>>> ((void)sizeof(char[1 - 2 * __cond]))
>>>
>>> ... is not obvious for the compiler to know the array size is negative.
>>>
>>> Reverting that commit would break BUILD_BUG() because negative-size-array
>>> is evaluated before the code is optimized out.
>>>
>>> Let's give up __compiletime_assert_fallback(). This commit does not
>>> change the current behavior since it just rips off the useless code.
>> Clang is not the only target audience of
>> __compiletime_assert_fallback(). Instead of ripping out something that
>> may benefit builds with gcc 4.2 and earlier, why not override its
> Note that with commit cafa0010cd51 ("Raise the minimum required gcc
> version to 4.6") that gcc < 4.6 is irrelevant.
Ah, I guess I'm not keeping up, that's wonderful news! Considering that
I guess I would be OK with its removal, but I still think it would be
better if a similar mechanism to break the Clang build could be found.
>> definition in compiler-clang.h with something that will break the build
>> for Clang? It would need an #ifndef __compiletime_error_fallback here
>> though.
>>
>>> Signed-off-by: Masahiro Yamada <[email protected]>
>>> Reviewed-by: Kees Cook <[email protected]>
>>> Reviewed-by: Nick Desaulniers <[email protected]>
>>> ---
>>>
>>> Changes in v2:
>>> - Rebase
>>>
>>> include/linux/compiler.h | 17 +----------------
>>> 1 file changed, 1 insertion(+), 16 deletions(-)
>>>
>>> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
>>> index 681d866..87c776c 100644
>>> --- a/include/linux/compiler.h
>>> +++ b/include/linux/compiler.h
>>> @@ -314,29 +314,14 @@ static inline void *offset_to_ptr(const int *off)
>>> #endif
>>> #ifndef __compiletime_error
>>> # define __compiletime_error(message)
>>> -/*
>>> - * Sparse complains of variable sized arrays due to the temporary variable in
>>> - * __compiletime_assert. Unfortunately we can't just expand it out to make
>>> - * sparse see a constant array size without breaking compiletime_assert on old
>>> - * versions of GCC (e.g. 4.2.4), so hide the array from sparse altogether.
>>> - */
>>> -# ifndef __CHECKER__
>>> -# define __compiletime_error_fallback(condition) \
>>> - do { ((void)sizeof(char[1 - 2 * condition])); } while (0)
>>> -# endif
>>> -#endif
>>> -#ifndef __compiletime_error_fallback
>>> -# define __compiletime_error_fallback(condition) do { } while (0)
>>> #endif
>>>
>>> #ifdef __OPTIMIZE__
>>> # define __compiletime_assert(condition, msg, prefix, suffix) \
>>> do { \
>>> - int __cond = !(condition); \
>>> extern void prefix ## suffix(void) __compiletime_error(msg); \
>>> - if (__cond) \
>>> + if (!(condition)) \
>>> prefix ## suffix(); \
>>> - __compiletime_error_fallback(__cond); \
>>> } while (0)
>>> #else
>>> # define __compiletime_assert(condition, msg, prefix, suffix) do { } while (0)
>> To give any more meaningful feedback I think I will need to experiment
>> with Clang, older GCC versions and icc. It occurred to me that I should
>> probably clean up and publish my __builtin_constant_p test program and
>> also generate results for more recent compilers. I can extend it to
>> test various negative sized array constructs and it could help inform
>> this decision.
>>
>> IMO, the most ideal solution would be a set of C2x (or future)
>> extensions providing something similar to C++'s constexpr, GCC's
>> __builtin_constant_p and our BUILD_BUG_ON. This would cross deeply into
> Note that __builtin_constant_p is a wild beast with lots of edge
> cases, and dependencies on compiler and optimization level. I'm
> trying to sort out some of these differences right now with llvm
> developers.
Yes it is. IIRC, I even found cases where __builtin_constant_p returned
false, but the emitted code replaced the value in question with a
constant. So at least at one point, constant propagation and
__builtin_constant_p weren't always entirely coherent.
I was working on a GCC extension that would solve this problem earlier
this year but bills and paying work interrupted me. :) The solution
involved adding a "const" attribute for function parameters and
variables that would simply create a warning or error if the value
wasn't compiled away at the time middle-end optimizations completed and
RTL emitted. It isn't finished -- maybe for gcc10.
Daniel
>
>> territory traditionally considered to belong to the implementation, so
>> it's no small request. A lot would have to be resolved for it to work
>> in the standard.
>>
>> Daniel
On Mon, Aug 27, 2018 at 1:42 PM Daniel Santos <[email protected]> wrote:
>
> Hello Nick,
>
> On 08/27/2018 03:09 PM, Nick Desaulniers wrote:
> >> Now we're back to the question of "what do you mean by 'constant'"? If
> >> you mean a C constant expression (as defined in the C standard) than
> >> almost none of this code fits that criteria. For these compile-time
> >> assertions to work, we are concerned with the data flow analysis and
> >> constant propagation performed by the compiler during optimization. You
> >> will notice in include/linux/compiler.h that __compiletime_assert is a
> >> no-op when __OPTIMIZE__ is not defined.
> > Depending on optimizations for static assertions sounds problematic.
>
> (with my best Palpatine voice) It is unavoidable.
LOL
>
> Actually it's theoretically possible, but the compiler would have to do
> something akin to copying it's control flow graph et. al, run -O2-ish
> optimizations, perform the static assertions and then throw away the
> optimized control flow graph and emit code based upon the original.
>
>
> >>> As the comment in <linux/build_bug.h>
> >>> says, GCC (and Clang as well) only emits the error for obvious cases.
> >>>
> >>> When '__cond' is a variable,
> >>>
> >>> ((void)sizeof(char[1 - 2 * __cond]))
> >>>
> >>> ... is not obvious for the compiler to know the array size is negative.
> >>>
> >>> Reverting that commit would break BUILD_BUG() because negative-size-array
> >>> is evaluated before the code is optimized out.
> >>>
> >>> Let's give up __compiletime_assert_fallback(). This commit does not
> >>> change the current behavior since it just rips off the useless code.
> >> Clang is not the only target audience of
> >> __compiletime_assert_fallback(). Instead of ripping out something that
> >> may benefit builds with gcc 4.2 and earlier, why not override its
> > Note that with commit cafa0010cd51 ("Raise the minimum required gcc
> > version to 4.6") that gcc < 4.6 is irrelevant.
>
> Ah, I guess I'm not keeping up, that's wonderful news! Considering that
> I guess I would be OK with its removal, but I still think it would be
> better if a similar mechanism to break the Clang build could be found.
>
> >> definition in compiler-clang.h with something that will break the build
> >> for Clang? It would need an #ifndef __compiletime_error_fallback here
> >> though.
> >>
> >>> Signed-off-by: Masahiro Yamada <[email protected]>
> >>> Reviewed-by: Kees Cook <[email protected]>
> >>> Reviewed-by: Nick Desaulniers <[email protected]>
> >>> ---
> >>>
> >>> Changes in v2:
> >>> - Rebase
> >>>
> >>> include/linux/compiler.h | 17 +----------------
> >>> 1 file changed, 1 insertion(+), 16 deletions(-)
> >>>
> >>> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> >>> index 681d866..87c776c 100644
> >>> --- a/include/linux/compiler.h
> >>> +++ b/include/linux/compiler.h
> >>> @@ -314,29 +314,14 @@ static inline void *offset_to_ptr(const int *off)
> >>> #endif
> >>> #ifndef __compiletime_error
> >>> # define __compiletime_error(message)
> >>> -/*
> >>> - * Sparse complains of variable sized arrays due to the temporary variable in
> >>> - * __compiletime_assert. Unfortunately we can't just expand it out to make
> >>> - * sparse see a constant array size without breaking compiletime_assert on old
> >>> - * versions of GCC (e.g. 4.2.4), so hide the array from sparse altogether.
> >>> - */
> >>> -# ifndef __CHECKER__
> >>> -# define __compiletime_error_fallback(condition) \
> >>> - do { ((void)sizeof(char[1 - 2 * condition])); } while (0)
> >>> -# endif
> >>> -#endif
> >>> -#ifndef __compiletime_error_fallback
> >>> -# define __compiletime_error_fallback(condition) do { } while (0)
> >>> #endif
> >>>
> >>> #ifdef __OPTIMIZE__
> >>> # define __compiletime_assert(condition, msg, prefix, suffix) \
> >>> do { \
> >>> - int __cond = !(condition); \
> >>> extern void prefix ## suffix(void) __compiletime_error(msg); \
> >>> - if (__cond) \
> >>> + if (!(condition)) \
> >>> prefix ## suffix(); \
> >>> - __compiletime_error_fallback(__cond); \
> >>> } while (0)
> >>> #else
> >>> # define __compiletime_assert(condition, msg, prefix, suffix) do { } while (0)
> >> To give any more meaningful feedback I think I will need to experiment
> >> with Clang, older GCC versions and icc. It occurred to me that I should
> >> probably clean up and publish my __builtin_constant_p test program and
> >> also generate results for more recent compilers. I can extend it to
> >> test various negative sized array constructs and it could help inform
> >> this decision.
> >>
> >> IMO, the most ideal solution would be a set of C2x (or future)
> >> extensions providing something similar to C++'s constexpr, GCC's
> >> __builtin_constant_p and our BUILD_BUG_ON. This would cross deeply into
> > Note that __builtin_constant_p is a wild beast with lots of edge
> > cases, and dependencies on compiler and optimization level. I'm
> > trying to sort out some of these differences right now with llvm
> > developers.
>
> Yes it is. IIRC, I even found cases where __builtin_constant_p returned
> false, but the emitted code replaced the value in question with a
> constant. So at least at one point, constant propagation and
> __builtin_constant_p weren't always entirely coherent.
What?! Do you have a link to a repro on godbolt or a gcc bugreport?
Here's a different case I found that looks problematic:
https://godbolt.org/z/g_iqwh
>
> I was working on a GCC extension that would solve this problem earlier
> this year but bills and paying work interrupted me. :) The solution
> involved adding a "const" attribute for function parameters and
> variables that would simply create a warning or error if the value
> wasn't compiled away at the time middle-end optimizations completed and
> RTL emitted. It isn't finished -- maybe for gcc10.
>
Speaking with a coworker internally now, discussing Clang's
diagnose_if/enable_if attributes. It seems that _Static_assert always
(between compilers) considers parameters non-ICE, which sounds like a
defect to me.
--
Thanks,
~Nick Desaulniers
On Mon, Aug 27, 2018 at 10:44 PM Daniel Santos <[email protected]> wrote:
>
> On 08/27/2018 03:09 PM, Nick Desaulniers wrote:
> >> Now we're back to the question of "what do you mean by 'constant'"? If
> >> you mean a C constant expression (as defined in the C standard) than
> >> almost none of this code fits that criteria. For these compile-time
> >> assertions to work, we are concerned with the data flow analysis and
> >> constant propagation performed by the compiler during optimization. You
> >> will notice in include/linux/compiler.h that __compiletime_assert is a
> >> no-op when __OPTIMIZE__ is not defined.
> > Depending on optimizations for static assertions sounds problematic.
>
> (with my best Palpatine voice) It is unavoidable.
>
> Actually it's theoretically possible, but the compiler would have to do
> something akin to copying it's control flow graph et. al, run -O2-ish
> optimizations, perform the static assertions and then throw away the
> optimized control flow graph and emit code based upon the original.
In the context of the kernel, compiling with anything less than -O2 or
-Os is not an issue, we don't do it anyway. -O0 never worked, and
AFAICT we only build one file with -O1, but that is something we can do
away with as well:
from fs/reiserfs/Makefile:
# gcc -O2 (the kernel default) is overaggressive on ppc32 when many inline
# functions are used. This causes the compiler to advance the stack
# pointer out of the available stack space, corrupting kernel space,
# and causing a panic. Since this behavior only affects ppc32, this ifeq
# will work around it. If any other architecture displays this behavior,
# add it here.
ccflags-$(CONFIG_PPC32) := $(call cc-ifversion, -lt, 0400, -O1)
Arnd
2018-08-28 19:55 GMT+09:00 Arnd Bergmann <[email protected]>:
> On Mon, Aug 27, 2018 at 10:44 PM Daniel Santos <[email protected]> wrote:
>>
>> On 08/27/2018 03:09 PM, Nick Desaulniers wrote:
>> >> Now we're back to the question of "what do you mean by 'constant'"? If
>> >> you mean a C constant expression (as defined in the C standard) than
>> >> almost none of this code fits that criteria. For these compile-time
>> >> assertions to work, we are concerned with the data flow analysis and
>> >> constant propagation performed by the compiler during optimization. You
>> >> will notice in include/linux/compiler.h that __compiletime_assert is a
>> >> no-op when __OPTIMIZE__ is not defined.
>> > Depending on optimizations for static assertions sounds problematic.
>>
>> (with my best Palpatine voice) It is unavoidable.
>>
>> Actually it's theoretically possible, but the compiler would have to do
>> something akin to copying it's control flow graph et. al, run -O2-ish
>> optimizations, perform the static assertions and then throw away the
>> optimized control flow graph and emit code based upon the original.
>
> In the context of the kernel, compiling with anything less than -O2 or
> -Os is not an issue, we don't do it anyway. -O0 never worked, and
> AFAICT we only build one file with -O1, but that is something we can do
> away with as well:
>
> from fs/reiserfs/Makefile:
> # gcc -O2 (the kernel default) is overaggressive on ppc32 when many inline
> # functions are used. This causes the compiler to advance the stack
> # pointer out of the available stack space, corrupting kernel space,
> # and causing a panic. Since this behavior only affects ppc32, this ifeq
> # will work around it. If any other architecture displays this behavior,
> # add it here.
> ccflags-$(CONFIG_PPC32) := $(call cc-ifversion, -lt, 0400, -O1)
>
> Arnd
Recently, I sent out patches to remove redundant GCC version checks,
including this one.
https://lore.kernel.org/patchwork/patch/977808/
I do not know who is maintaining reiserfs, though.
--
Best Regards
Masahiro Yamada
On Mon, Aug 27, 2018 at 1:42 PM Daniel Santos <[email protected]> wrote:
>
> Hello Nick,
>
> On 08/27/2018 03:09 PM, Nick Desaulniers wrote:
> >>> Let's give up __compiletime_assert_fallback(). This commit does not
> >>> change the current behavior since it just rips off the useless code.
> >> Clang is not the only target audience of
> >> __compiletime_assert_fallback(). Instead of ripping out something that
> >> may benefit builds with gcc 4.2 and earlier, why not override its
> > Note that with commit cafa0010cd51 ("Raise the minimum required gcc
> > version to 4.6") that gcc < 4.6 is irrelevant.
>
> Ah, I guess I'm not keeping up, that's wonderful news! Considering that
> I guess I would be OK with its removal, but I still think it would be
> better if a similar mechanism to break the Clang build could be found.
I'm consulting with our best language lawyers to see what combinations
of _Static_assert and __builtin_constant_p would do the trick.
On Tue, Aug 28, 2018 at 4:00 PM Nick Desaulniers
<[email protected]> wrote:
>
> On Mon, Aug 27, 2018 at 1:42 PM Daniel Santos <[email protected]> wrote:
> >
> > Hello Nick,
> >
> > On 08/27/2018 03:09 PM, Nick Desaulniers wrote:
> > >>> Let's give up __compiletime_assert_fallback(). This commit does not
> > >>> change the current behavior since it just rips off the useless code.
> > >> Clang is not the only target audience of
> > >> __compiletime_assert_fallback(). Instead of ripping out something that
> > >> may benefit builds with gcc 4.2 and earlier, why not override its
> > > Note that with commit cafa0010cd51 ("Raise the minimum required gcc
> > > version to 4.6") that gcc < 4.6 is irrelevant.
> >
> > Ah, I guess I'm not keeping up, that's wonderful news! Considering that
> > I guess I would be OK with its removal, but I still think it would be
> > better if a similar mechanism to break the Clang build could be found.
>
> I'm consulting with our best language lawyers to see what combinations
> of _Static_assert and __builtin_constant_p would do the trick.
Linus,
Can this patch be merged in the meantime?
--
Thanks,
~Nick Desaulniers
On Fri, Aug 31, 2018 at 09:46:02AM -0700, Nick Desaulniers wrote:
> On Tue, Aug 28, 2018 at 4:00 PM Nick Desaulniers
> <[email protected]> wrote:
> >
> > On Mon, Aug 27, 2018 at 1:42 PM Daniel Santos <[email protected]> wrote:
> > >
> > > Hello Nick,
> > >
> > > On 08/27/2018 03:09 PM, Nick Desaulniers wrote:
> > > >>> Let's give up __compiletime_assert_fallback(). This commit does not
> > > >>> change the current behavior since it just rips off the useless code.
> > > >> Clang is not the only target audience of
> > > >> __compiletime_assert_fallback(). Instead of ripping out something that
> > > >> may benefit builds with gcc 4.2 and earlier, why not override its
> > > > Note that with commit cafa0010cd51 ("Raise the minimum required gcc
> > > > version to 4.6") that gcc < 4.6 is irrelevant.
> > >
> > > Ah, I guess I'm not keeping up, that's wonderful news! Considering that
> > > I guess I would be OK with its removal, but I still think it would be
> > > better if a similar mechanism to break the Clang build could be found.
> >
> > I'm consulting with our best language lawyers to see what combinations
> > of _Static_assert and __builtin_constant_p would do the trick.
>
> Linus,
> Can this patch be merged in the meantime?
friendly ping :)
With c5c2b11894f4 ("drm/i915: Warn against variable length arrays")
clang raises plenty of vla warnings about
__compiletime_error_fallback() in the i915 driver. Would be great to
get rid of those without having to revert that commit.
On Wed, Sep 26, 2018 at 11:00 AM Matthias Kaehlcke <[email protected]> wrote:
>
> On Fri, Aug 31, 2018 at 09:46:02AM -0700, Nick Desaulniers wrote:
> > On Tue, Aug 28, 2018 at 4:00 PM Nick Desaulniers
> > <[email protected]> wrote:
> > >
> > > On Mon, Aug 27, 2018 at 1:42 PM Daniel Santos <[email protected]> wrote:
> > > >
> > > > Hello Nick,
> > > >
> > > > On 08/27/2018 03:09 PM, Nick Desaulniers wrote:
> > > > >>> Let's give up __compiletime_assert_fallback(). This commit does not
> > > > >>> change the current behavior since it just rips off the useless code.
> > > > >> Clang is not the only target audience of
> > > > >> __compiletime_assert_fallback(). Instead of ripping out something that
> > > > >> may benefit builds with gcc 4.2 and earlier, why not override its
> > > > > Note that with commit cafa0010cd51 ("Raise the minimum required gcc
> > > > > version to 4.6") that gcc < 4.6 is irrelevant.
> > > >
> > > > Ah, I guess I'm not keeping up, that's wonderful news! Considering that
> > > > I guess I would be OK with its removal, but I still think it would be
> > > > better if a similar mechanism to break the Clang build could be found.
> > >
> > > I'm consulting with our best language lawyers to see what combinations
> > > of _Static_assert and __builtin_constant_p would do the trick.
> >
> > Linus,
> > Can this patch be merged in the meantime?
>
> friendly ping :)
>
> With c5c2b11894f4 ("drm/i915: Warn against variable length arrays")
> clang raises plenty of vla warnings about
> __compiletime_error_fallback() in the i915 driver. Would be great to
> get rid of those without having to revert that commit.
I've been meaning to follow up on this, thanks Matthias. I too would
really like this patch.
--
Thanks,
~Nick Desaulniers
On Wed, Sep 26, 2018 at 11:03 AM, Nick Desaulniers
<[email protected]> wrote:
> On Wed, Sep 26, 2018 at 11:00 AM Matthias Kaehlcke <[email protected]> wrote:
>>
>> On Fri, Aug 31, 2018 at 09:46:02AM -0700, Nick Desaulniers wrote:
>> > On Tue, Aug 28, 2018 at 4:00 PM Nick Desaulniers
>> > <[email protected]> wrote:
>> > >
>> > > On Mon, Aug 27, 2018 at 1:42 PM Daniel Santos <[email protected]> wrote:
>> > > >
>> > > > Hello Nick,
>> > > >
>> > > > On 08/27/2018 03:09 PM, Nick Desaulniers wrote:
>> > > > >>> Let's give up __compiletime_assert_fallback(). This commit does not
>> > > > >>> change the current behavior since it just rips off the useless code.
>> > > > >> Clang is not the only target audience of
>> > > > >> __compiletime_assert_fallback(). Instead of ripping out something that
>> > > > >> may benefit builds with gcc 4.2 and earlier, why not override its
>> > > > > Note that with commit cafa0010cd51 ("Raise the minimum required gcc
>> > > > > version to 4.6") that gcc < 4.6 is irrelevant.
>> > > >
>> > > > Ah, I guess I'm not keeping up, that's wonderful news! Considering that
>> > > > I guess I would be OK with its removal, but I still think it would be
>> > > > better if a similar mechanism to break the Clang build could be found.
>> > >
>> > > I'm consulting with our best language lawyers to see what combinations
>> > > of _Static_assert and __builtin_constant_p would do the trick.
>> >
>> > Linus,
>> > Can this patch be merged in the meantime?
>>
>> friendly ping :)
>>
>> With c5c2b11894f4 ("drm/i915: Warn against variable length arrays")
>> clang raises plenty of vla warnings about
>> __compiletime_error_fallback() in the i915 driver. Would be great to
>> get rid of those without having to revert that commit.
>
> I've been meaning to follow up on this, thanks Matthias. I too would
> really like this patch.
Adding Greg to the thread. Between Masahiro's detailed commit log and
the Clang-familiar reviewers, I think this should land for 4.19 (as
part of the other Clang-sanity patches that are already in 4.19). This
has no impact on gcc now that we're requiring 4.6+.
https://lore.kernel.org/patchwork/patch/977668/
-Kees
--
Kees Cook
Pixel Security
On Wed, Sep 26, 2018 at 11:26:46AM -0700, Kees Cook wrote:
> On Wed, Sep 26, 2018 at 11:03 AM, Nick Desaulniers
> <[email protected]> wrote:
> > On Wed, Sep 26, 2018 at 11:00 AM Matthias Kaehlcke <[email protected]> wrote:
> >>
> >> On Fri, Aug 31, 2018 at 09:46:02AM -0700, Nick Desaulniers wrote:
> >> > On Tue, Aug 28, 2018 at 4:00 PM Nick Desaulniers
> >> > <[email protected]> wrote:
> >> > >
> >> > > On Mon, Aug 27, 2018 at 1:42 PM Daniel Santos <[email protected]> wrote:
> >> > > >
> >> > > > Hello Nick,
> >> > > >
> >> > > > On 08/27/2018 03:09 PM, Nick Desaulniers wrote:
> >> > > > >>> Let's give up __compiletime_assert_fallback(). This commit does not
> >> > > > >>> change the current behavior since it just rips off the useless code.
> >> > > > >> Clang is not the only target audience of
> >> > > > >> __compiletime_assert_fallback(). Instead of ripping out something that
> >> > > > >> may benefit builds with gcc 4.2 and earlier, why not override its
> >> > > > > Note that with commit cafa0010cd51 ("Raise the minimum required gcc
> >> > > > > version to 4.6") that gcc < 4.6 is irrelevant.
> >> > > >
> >> > > > Ah, I guess I'm not keeping up, that's wonderful news! Considering that
> >> > > > I guess I would be OK with its removal, but I still think it would be
> >> > > > better if a similar mechanism to break the Clang build could be found.
> >> > >
> >> > > I'm consulting with our best language lawyers to see what combinations
> >> > > of _Static_assert and __builtin_constant_p would do the trick.
> >> >
> >> > Linus,
> >> > Can this patch be merged in the meantime?
> >>
> >> friendly ping :)
> >>
> >> With c5c2b11894f4 ("drm/i915: Warn against variable length arrays")
> >> clang raises plenty of vla warnings about
> >> __compiletime_error_fallback() in the i915 driver. Would be great to
> >> get rid of those without having to revert that commit.
> >
> > I've been meaning to follow up on this, thanks Matthias. I too would
> > really like this patch.
>
> Adding Greg to the thread. Between Masahiro's detailed commit log and
> the Clang-familiar reviewers, I think this should land for 4.19 (as
> part of the other Clang-sanity patches that are already in 4.19). This
> has no impact on gcc now that we're requiring 4.6+.
>
> https://lore.kernel.org/patchwork/patch/977668/
I'm not digging up a compiler.h patch from a web site and adding it to
the tree this late in the release cycle. Especially given that it
hasn't had any testing anywhere...
nice try though :)
greg k-h
On Wed, Sep 26, 2018 at 11:42 AM, Greg KH <[email protected]> wrote:
> I'm not digging up a compiler.h patch from a web site and adding it to
> the tree this late in the release cycle. Especially given that it
> hasn't had any testing anywhere...
Good point about it not living in -next.
Who should be carrying these sorts of patches? In the past it's been
Andrew or Masahiro, yes? For linux-next, maybe it can go via -mm?
-Kees
--
Kees Cook
Pixel Security
On Wed, Sep 26, 2018 at 11:45:19AM -0700, Kees Cook wrote:
> On Wed, Sep 26, 2018 at 11:42 AM, Greg KH <[email protected]> wrote:
> > I'm not digging up a compiler.h patch from a web site and adding it to
> > the tree this late in the release cycle. Especially given that it
> > hasn't had any testing anywhere...
>
> Good point about it not living in -next.
>
> Who should be carrying these sorts of patches? In the past it's been
> Andrew or Masahiro, yes? For linux-next, maybe it can go via -mm?
Either is fine with me, as long as it isn't one of my trees :)
thanks,
greg k-h
On Wed, Sep 26, 2018 at 12:03 PM Greg KH <[email protected]> wrote:
>
> On Wed, Sep 26, 2018 at 11:45:19AM -0700, Kees Cook wrote:
> > On Wed, Sep 26, 2018 at 11:42 AM, Greg KH <[email protected]> wrote:
> > > I'm not digging up a compiler.h patch from a web site and adding it to
> > > the tree this late in the release cycle. Especially given that it
> > > hasn't had any testing anywhere...
> >
> > Good point about it not living in -next.
> >
> > Who should be carrying these sorts of patches? In the past it's been
> > Andrew or Masahiro, yes? For linux-next, maybe it can go via -mm?
>
> Either is fine with me, as long as it isn't one of my trees :)
>
> thanks,
>
> greg k-h
Besides, I think we want the v2: https://lkml.org/lkml/2018/8/25/103
--
Thanks,
~Nick Desaulniers
On Wed, Sep 26, 2018 at 12:29 PM, Nick Desaulniers
<[email protected]> wrote:
> On Wed, Sep 26, 2018 at 12:03 PM Greg KH <[email protected]> wrote:
>>
>> On Wed, Sep 26, 2018 at 11:45:19AM -0700, Kees Cook wrote:
>> > On Wed, Sep 26, 2018 at 11:42 AM, Greg KH <[email protected]> wrote:
>> > > I'm not digging up a compiler.h patch from a web site and adding it to
>> > > the tree this late in the release cycle. Especially given that it
>> > > hasn't had any testing anywhere...
>> >
>> > Good point about it not living in -next.
>> >
>> > Who should be carrying these sorts of patches? In the past it's been
>> > Andrew or Masahiro, yes? For linux-next, maybe it can go via -mm?
>>
>> Either is fine with me, as long as it isn't one of my trees :)
>>
>> thanks,
>>
>> greg k-h
>
> Besides, I think we want the v2: https://lkml.org/lkml/2018/8/25/103
Yeah, that's what I'd linked to in the patchwork URL. Andrew, can you take this?
-Kees
--
Kees Cook
Pixel Security
On Thu, 27 Sep 2018 at 05:07, Kees Cook <[email protected]> wrote:
>
> On Wed, Sep 26, 2018 at 12:29 PM, Nick Desaulniers
> <[email protected]> wrote:
> > On Wed, Sep 26, 2018 at 12:03 PM Greg KH <[email protected]> wrote:
> >>
> >> On Wed, Sep 26, 2018 at 11:45:19AM -0700, Kees Cook wrote:
> >> > On Wed, Sep 26, 2018 at 11:42 AM, Greg KH <[email protected]> wrote:
> >> > > I'm not digging up a compiler.h patch from a web site and adding it to
> >> > > the tree this late in the release cycle. Especially given that it
> >> > > hasn't had any testing anywhere...
> >> >
> >> > Good point about it not living in -next.
> >> >
> >> > Who should be carrying these sorts of patches? In the past it's been
> >> > Andrew or Masahiro, yes? For linux-next, maybe it can go via -mm?
> >>
> >> Either is fine with me, as long as it isn't one of my trees :)
> >>
> >> thanks,
> >>
> >> greg k-h
> >
> > Besides, I think we want the v2: https://lkml.org/lkml/2018/8/25/103
>
> Yeah, that's what I'd linked to in the patchwork URL. Andrew, can you take this?
clang built -next is blowing up now that Kees' -Wvla patch has been
included. This patch fixes it.
Kees, perhaps it should go in your tree along side of the -Wvla patch
if no one else wants to take it?
Cheers,
Joel
On Wed, Oct 10, 2018 at 8:12 AM Joel Stanley <[email protected]> wrote:
>
> On Thu, 27 Sep 2018 at 05:07, Kees Cook <[email protected]> wrote:
> >
> > Yeah, that's what I'd linked to in the patchwork URL. Andrew, can you take this?
>
> clang built -next is blowing up now that Kees' -Wvla patch has been
> included. This patch fixes it.
>
> Kees, perhaps it should go in your tree along side of the -Wvla patch
> if no one else wants to take it?
>
I can take it along in the compiler attributes tree, since that
touches the compiler*.h stuff. Although that would make it
not-only-attributes, i.e. slightly lying :-)
Cheers,
Miguel
On Wed, Oct 10, 2018 at 12:03 AM, Miguel Ojeda
<[email protected]> wrote:
> On Wed, Oct 10, 2018 at 8:12 AM Joel Stanley <[email protected]> wrote:
>>
>> On Thu, 27 Sep 2018 at 05:07, Kees Cook <[email protected]> wrote:
>> >
>> > Yeah, that's what I'd linked to in the patchwork URL. Andrew, can you take this?
>>
>> clang built -next is blowing up now that Kees' -Wvla patch has been
>> included. This patch fixes it.
>>
>> Kees, perhaps it should go in your tree along side of the -Wvla patch
>> if no one else wants to take it?
>>
>
> I can take it along in the compiler attributes tree, since that
> touches the compiler*.h stuff. Although that would make it
> not-only-attributes, i.e. slightly lying :-)
Oh, I had assumed Masahiro was going to carry it. If that's not true,
sure I'll pick it up as part of my VLA "series".
-Kees
--
Kees Cook
Pixel Security
On Wed, Oct 10, 2018 at 11:51 PM Kees Cook <[email protected]> wrote:
>
> On Wed, Oct 10, 2018 at 12:03 AM, Miguel Ojeda
> <[email protected]> wrote:
> > On Wed, Oct 10, 2018 at 8:12 AM Joel Stanley <[email protected]> wrote:
> >>
> >> On Thu, 27 Sep 2018 at 05:07, Kees Cook <[email protected]> wrote:
> >> >
> >> > Yeah, that's what I'd linked to in the patchwork URL. Andrew, can you take this?
> >>
> >> clang built -next is blowing up now that Kees' -Wvla patch has been
> >> included. This patch fixes it.
> >>
> >> Kees, perhaps it should go in your tree along side of the -Wvla patch
> >> if no one else wants to take it?
> >>
> >
> > I can take it along in the compiler attributes tree, since that
> > touches the compiler*.h stuff. Although that would make it
> > not-only-attributes, i.e. slightly lying :-)
>
> Oh, I had assumed Masahiro was going to carry it.
No, I am not.
Putting all sort of things into kbuild basket
is painful for me.
> If that's not true,
> sure I'll pick it up as part of my VLA "series".
--
Best Regards
Masahiro Yamada
On Wed, Oct 10, 2018 at 7:48 PM, Masahiro Yamada
<[email protected]> wrote:
> On Wed, Oct 10, 2018 at 11:51 PM Kees Cook <[email protected]> wrote:
>>
>> On Wed, Oct 10, 2018 at 12:03 AM, Miguel Ojeda
>> <[email protected]> wrote:
>> > On Wed, Oct 10, 2018 at 8:12 AM Joel Stanley <[email protected]> wrote:
>> >>
>> >> On Thu, 27 Sep 2018 at 05:07, Kees Cook <[email protected]> wrote:
>> >> >
>> >> > Yeah, that's what I'd linked to in the patchwork URL. Andrew, can you take this?
>> >>
>> >> clang built -next is blowing up now that Kees' -Wvla patch has been
>> >> included. This patch fixes it.
>> >>
>> >> Kees, perhaps it should go in your tree along side of the -Wvla patch
>> >> if no one else wants to take it?
>> >>
>> >
>> > I can take it along in the compiler attributes tree, since that
>> > touches the compiler*.h stuff. Although that would make it
>> > not-only-attributes, i.e. slightly lying :-)
>>
>> Oh, I had assumed Masahiro was going to carry it.
>
> No, I am not.
>
> Putting all sort of things into kbuild basket
> is painful for me.
Okay, I'll take it for the VLA series.
-Kees
--
Kees Cook
Pixel Security