| Message ID | 1534657880-11573-1-git-send-email-yamada.masahiro@socionext.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RFC] compiler.h: give up __compiletime_assert_fallback() | expand |
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
+ 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
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
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
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
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
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
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
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
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)
__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(-) -- 2.7.4