[RFC PATCH] compiler.h: give up __compiletime_assert_fallback()

[RFC PATCH] compiler.h: give up __compiletime_assert_fallback()

[Masahiro Yamada]

__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 try this simple code:

    #include <linux/build_bug.h>
    void foo(void)
    {
            BUILD_BUG_ON(1);
    }

GCC (precisely, GCC 4.3 or later) immediately terminates the build,
but Clang does not report anything because Clang does not support the
"error" attribute now.  It will eventually fail in the link stage,
but at least __compiletime_assert_fallback() is not working.

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.

One way to fix this is to stop the variable assignment, i.e. to pass
'!(condition)' directly to __compiletime_error_fallback() at the cost
of the double evaluation of 'condition'.  However, all calls of
BUILD_BUG() would be turned into errors even if they are called from
dead-code.

This commit does not change the current behavior since it just rips
off the code that has not been effective for some years.

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

 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 42506e4..c062238f4 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -295,29 +295,14 @@ unsigned long read_word_at_a_time(const void *addr)
 #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 {								\
-		bool __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

Re: [RFC PATCH] compiler.h: give up __compiletime_assert_fallback()

[Kees Cook]

On Sat, Aug 18, 2018 at 10:51 PM, Masahiro Yamada
<yamada.masahiro@socionext.com> 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 try this simple code:
>
>     #include <linux/build_bug.h>
>     void foo(void)
>     {
>             BUILD_BUG_ON(1);
>     }
>
> GCC (precisely, GCC 4.3 or later) immediately terminates the build,
> but Clang does not report anything because Clang does not support the
> "error" attribute now.  It will eventually fail in the link stage,
> but at least __compiletime_assert_fallback() is not working.
>
> 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.
>
> One way to fix this is to stop the variable assignment, i.e. to pass
> '!(condition)' directly to __compiletime_error_fallback() at the cost
> of the double evaluation of 'condition'.  However, all calls of
> BUILD_BUG() would be turned into errors even if they are called from
> dead-code.
>
> This commit does not change the current behavior since it just rips
> off the code that has not been effective for some years.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>

Yeah, Clang would only complain about the VLA (and not error) and then
later fail at link time. This seems like a reasonable change to me.

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees

> ---
>
>  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 42506e4..c062238f4 100644
> --- a/include/linux/compiler.h
> +++ b/include/linux/compiler.h
> @@ -295,29 +295,14 @@ unsigned long read_word_at_a_time(const void *addr)
>  #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 {                                                            \
> -               bool __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
>



-- 
Kees Cook
Pixel Security

Re: [RFC PATCH] compiler.h: give up __compiletime_assert_fallback()

[Nick Desaulniers]

+ gbiv who wrote this cool paste (showing alternatives to
_Static_assert, which is supported by both compilers in -std=gnu89,
but not until gcc 4.6): https://godbolt.org/g/DuLsxu

I can't help but think that BUILD_BUG_ON_MSG should use
_Static_assert, then have fallbacks for gcc < 4.6.

On Sun, Aug 19, 2018 at 9:14 AM Kees Cook <keescook@chromium.org> wrote:
>
> On Sat, Aug 18, 2018 at 10:51 PM, Masahiro Yamada
> <yamada.masahiro@socionext.com> 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 try this simple code:
> >
> >     #include <linux/build_bug.h>
> >     void foo(void)
> >     {
> >             BUILD_BUG_ON(1);
> >     }
> >
> > GCC (precisely, GCC 4.3 or later) immediately terminates the build,
> > but Clang does not report anything because Clang does not support the
> > "error" attribute now.  It will eventually fail in the link stage,
> > but at least __compiletime_assert_fallback() is not working.
> >
> > 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.
> >
> > One way to fix this is to stop the variable assignment, i.e. to pass
> > '!(condition)' directly to __compiletime_error_fallback() at the cost
> > of the double evaluation of 'condition'.  However, all calls of
> > BUILD_BUG() would be turned into errors even if they are called from
> > dead-code.
> >
> > This commit does not change the current behavior since it just rips
> > off the code that has not been effective for some years.
> >
> > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>
> Yeah, Clang would only complain about the VLA (and not error) and then
> later fail at link time. This seems like a reasonable change to me.

Heh, we were just talking about this (-Wvla warnings from this macro).
Was there a previous thread before this patch?

>
> Reviewed-by: Kees Cook <keescook@chromium.org>
>
> -Kees
>
> > ---
> >
> >  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 42506e4..c062238f4 100644
> > --- a/include/linux/compiler.h
> > +++ b/include/linux/compiler.h
> > @@ -295,29 +295,14 @@ unsigned long read_word_at_a_time(const void *addr)
> >  #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)

Note that there are a few definitions of BUILD_BUG_ON that still use
this negative array size trick.  Should that pattern be removed from
them as well?  See:
* arch/x86/boot/boot.h#L33
* include/linux/build_bug.h#L66
* tools/include/linux/kernel.h#L38

Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
-- 
Thanks,
~Nick Desaulniers

Re: [RFC PATCH] compiler.h: give up __compiletime_assert_fallback()

[Linus Torvalds]

On Sun, Aug 19, 2018 at 1:25 PM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> + gbiv who wrote this cool paste (showing alternatives to
> _Static_assert, which is supported by both compilers in -std=gnu89,
> but not until gcc 4.6): https://godbolt.org/g/DuLsxu
>
> I can't help but think that BUILD_BUG_ON_MSG should use
> _Static_assert, then have fallbacks for gcc < 4.6.

Well, it turns out that we effectively stopped supporting gcc < 4.6
during this merge window for other reasons, so..

             Linus

Re: [RFC PATCH] compiler.h: give up __compiletime_assert_fallback()

[Nick Desaulniers]

On Sun, Aug 19, 2018 at 1:28 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Sun, Aug 19, 2018 at 1:25 PM Nick Desaulniers
> <ndesaulniers@google.com> wrote:
> >
> > + gbiv who wrote this cool paste (showing alternatives to
> > _Static_assert, which is supported by both compilers in -std=gnu89,
> > but not until gcc 4.6): https://godbolt.org/g/DuLsxu
> >
> > I can't help but think that BUILD_BUG_ON_MSG should use
> > _Static_assert, then have fallbacks for gcc < 4.6.
>
> Well, it turns out that we effectively stopped supporting gcc < 4.6
> during this merge window for other reasons, so..

For the whole kernel (or just a particular arch)?  Which commit?  Do
we keep track of minimal versions somewhere?
Documentation/process/changes.rst mentions gcc 3.2 (eek), but I assume
there's a compiler version check somewhere (if you're using gcc and
it's version is less than X, abort. Ditto for clang.)
-- 
Thanks,
~Nick Desaulniers

Re: [RFC PATCH] compiler.h: give up __compiletime_assert_fallback()

[Linus Torvalds]

On Sun, Aug 19, 2018 at 1:36 PM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Sun, Aug 19, 2018 at 1:28 PM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > Well, it turns out that we effectively stopped supporting gcc < 4.6
> > during this merge window for other reasons, so..
>
> For the whole kernel (or just a particular arch)?  Which commit?  Do
> we keep track of minimal versions somewhere?

It's effectively for the whole kernel right now. See:

    https://lore.kernel.org/lkml/20180814170904.GA12768@roeck-us.net/

although it might be fixable. Nobody really *wants* to fix it, though,
because we've had that initializer issue before too, and various other
issues with old gcc versions.

So we have long had reasons why we'd _want_ to upgrade to at least gcc-4.6

The "we support gcc-3.2" in Documentation/process/changes.rst is
complete fantasy.

              Linus

Re: [RFC PATCH] compiler.h: give up __compiletime_assert_fallback()

[Daniel Santos]

On 08/19/2018 03:25 PM, Nick Desaulniers wrote:
> + gbiv who wrote this cool paste (showing alternatives to
> _Static_assert, which is supported by both compilers in -std=gnu89,
> but not until gcc 4.6): https://godbolt.org/g/DuLsxu
>
> I can't help but think that BUILD_BUG_ON_MSG should use
> _Static_assert, then have fallbacks for gcc < 4.6.

Unfortunately _Static_assert is a woefully inadequate replacement
because it requires a C constant expression.  Example:

    int a = 1;
    _Static_assert(a == 1, "a != 1");

results in "error: expression in static assertion is not constant." 
Language standards tend to shy away from defining implementation details
like optimizations, but we need to have completed a good data flow
analysis and constant propagation in order to do BUILD_BUG_ON_MSG, et.
al.; this is why they only work when optimizations are enabled.  As the
optimizer improves, new expressions can be used with BUILD_BUG_ON*.  I
did an analysis of this back in 2012 of how various types of variables
could be resolved to constants at compile-time and how that evolved from
gcc 3.4 to 4.7:

https://drive.google.com/open?id=1cQRAAOzjFy6Aw7CDc4QauHvd_spVkd5a

This changed again when -findirect-inline was added -- i.e.,
BUILD_BUG_ON could be used on parameters of inline functions even when
called by pointer, although the caller needed __flatten in some cases --
a bit messy.

Daniel

Re: [RFC PATCH] compiler.h: give up __compiletime_assert_fallback()

[Masahiro Yamada]

Hi Nick,


2018-08-20 5:25 GMT+09:00 Nick Desaulniers <ndesaulniers@google.com>:
> + gbiv who wrote this cool paste (showing alternatives to
> _Static_assert, which is supported by both compilers in -std=gnu89,
> but not until gcc 4.6): https://godbolt.org/g/DuLsxu
>
> I can't help but think that BUILD_BUG_ON_MSG should use
> _Static_assert, then have fallbacks for gcc < 4.6.
>
> On Sun, Aug 19, 2018 at 9:14 AM Kees Cook <keescook@chromium.org> wrote:
>>
>> On Sat, Aug 18, 2018 at 10:51 PM, Masahiro Yamada
>> <yamada.masahiro@socionext.com> 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 try this simple code:
>> >
>> >     #include <linux/build_bug.h>
>> >     void foo(void)
>> >     {
>> >             BUILD_BUG_ON(1);
>> >     }
>> >
>> > GCC (precisely, GCC 4.3 or later) immediately terminates the build,
>> > but Clang does not report anything because Clang does not support the
>> > "error" attribute now.  It will eventually fail in the link stage,
>> > but at least __compiletime_assert_fallback() is not working.
>> >
>> > 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.
>> >
>> > One way to fix this is to stop the variable assignment, i.e. to pass
>> > '!(condition)' directly to __compiletime_error_fallback() at the cost
>> > of the double evaluation of 'condition'.  However, all calls of
>> > BUILD_BUG() would be turned into errors even if they are called from
>> > dead-code.
>> >
>> > This commit does not change the current behavior since it just rips
>> > off the code that has not been effective for some years.
>> >
>> > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>>
>> Yeah, Clang would only complain about the VLA (and not error) and then
>> later fail at link time. This seems like a reasonable change to me.
>
> Heh, we were just talking about this (-Wvla warnings from this macro).
> Was there a previous thread before this patch?


No.
I had noticed that this code was not working some months before,
but have been wondering what to do.


I was not tracking the -Wvla thread because
the discussion was very long.


>>
>> Reviewed-by: Kees Cook <keescook@chromium.org>
>>
>> -Kees
>>
>> > ---
>> >
>> >  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 42506e4..c062238f4 100644
>> > --- a/include/linux/compiler.h
>> > +++ b/include/linux/compiler.h
>> > @@ -295,29 +295,14 @@ unsigned long read_word_at_a_time(const void *addr)
>> >  #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)
>
> Note that there are a few definitions of BUILD_BUG_ON that still use
> this negative array size trick.  Should that pattern be removed from
> them as well?  See:
> * arch/x86/boot/boot.h#L33
> * include/linux/build_bug.h#L66
> * tools/include/linux/kernel.h#L38

At this moment, -Wvla is the warning-3 level
in scripts/Makefile.extrawarn.

Personally, I do not care that much.


Thanks.



> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
> --
> Thanks,
> ~Nick Desaulniers



-- 
Best Regards
Masahiro Yamada

Re: [RFC PATCH] compiler.h: give up __compiletime_assert_fallback()

[Kees Cook]

On Tue, Aug 21, 2018 at 9:15 AM, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
>> Note that there are a few definitions of BUILD_BUG_ON that still use
>> this negative array size trick.  Should that pattern be removed from
>> them as well?  See:
>> * arch/x86/boot/boot.h#L33
>> * include/linux/build_bug.h#L66
>> * tools/include/linux/kernel.h#L38
>
> At this moment, -Wvla is the warning-3 level
> in scripts/Makefile.extrawarn.

We're down to a handful of VLA uses, but I don't think the negative
cases should matter since they're _intended_ to break a compile.

-Kees

-- 
Kees Cook
Pixel Security

Re: [RFC PATCH] compiler.h: give up __compiletime_assert_fallback()

[Masahiro Yamada]

Hi Daniel,


2018-08-21 17:11 GMT+09:00 Daniel Santos <daniel.santos@pobox.com>:
> On 08/19/2018 03:25 PM, Nick Desaulniers wrote:
>> + gbiv who wrote this cool paste (showing alternatives to
>> _Static_assert, which is supported by both compilers in -std=gnu89,
>> but not until gcc 4.6): https://godbolt.org/g/DuLsxu
>>
>> I can't help but think that BUILD_BUG_ON_MSG should use
>> _Static_assert, then have fallbacks for gcc < 4.6.
>
> Unfortunately _Static_assert is a woefully inadequate replacement
> because it requires a C constant expression.  Example:
>
>     int a = 1;
>     _Static_assert(a == 1, "a != 1");
>
> results in "error: expression in static assertion is not constant."


You are right.




I tried diagnose_if from Clang:

static inline void assert_d(int i) __attribute__((diagnose_if(!i, "oh
no", "error")))
{
}



Clang is silent about

       int a = 1;
       assert_d(a);




But,

       if (0)
              assert_d(0);


is error.

Hence, it cannot be used for BUILD_BUG().







Anyway, I will just try to rebase this patch and send it to Linus.



> Language standards tend to shy away from defining implementation details
> like optimizations, but we need to have completed a good data flow
> analysis and constant propagation in order to do BUILD_BUG_ON_MSG, et.
> al.; this is why they only work when optimizations are enabled.  As the
> optimizer improves, new expressions can be used with BUILD_BUG_ON*.  I
> did an analysis of this back in 2012 of how various types of variables
> could be resolved to constants at compile-time and how that evolved from
> gcc 3.4 to 4.7:
>
> https://drive.google.com/open?id=1cQRAAOzjFy6Aw7CDc4QauHvd_spVkd5a
>
> This changed again when -findirect-inline was added -- i.e.,
> BUILD_BUG_ON could be used on parameters of inline functions even when
> called by pointer, although the caller needed __flatten in some cases --
> a bit messy.
>
> Daniel



-- 
Best Regards
Masahiro Yamada