[PATCH v2 1/2] build_bug.h: remove negative-array fallback for BUILD_BUG_ON()

[PATCH v2 1/2] build_bug.h: remove negative-array fallback for BUILD_BUG_ON()

[Masahiro Yamada]

The kernel can only be compiled with an optimization option (-O2, -Os,
or the currently proposed -Og). Hence, __OPTIMIZE__ is always defined
in the kernel source.

The fallback for -O0 case is just hypothetical and pointless. Moreover,
commit 0bb95f80a38f ("Makefile: Globally enable VLA warning") enabled
-Wvla warning. The use of variable length arrays is banned.

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

Changes in v2: None

 include/linux/build_bug.h | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/include/linux/build_bug.h b/include/linux/build_bug.h
index 43d1fd5..d415c64 100644
--- a/include/linux/build_bug.h
+++ b/include/linux/build_bug.h
@@ -51,23 +51,9 @@
  * If you have some code which relies on certain constants being equal, or
  * some other compile-time-evaluated condition, you should use BUILD_BUG_ON to
  * detect if someone changes it.
- *
- * The implementation uses gcc's reluctance to create a negative array, but gcc
- * (as of 4.4) only emits that error for obvious cases (e.g. not arguments to
- * inline functions).  Luckily, in 4.3 they added the "error" function
- * attribute just for this type of case.  Thus, we use a negative sized array
- * (should always create an error on gcc versions older than 4.4) and then call
- * an undefined function with the error attribute (should always create an
- * error on gcc 4.3 and later).  If for some reason, neither creates a
- * compile-time error, we'll still have a link-time error, which is harder to
- * track down.
  */
-#ifndef __OPTIMIZE__
-#define BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
-#else
 #define BUILD_BUG_ON(condition) \
 	BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)
-#endif
 
 /**
  * BUILD_BUG - break compile if used.
-- 
2.7.4

[PATCH v2 2/2] build_bug.h: remove all dummy BUILD_BUG_ON stubs for sparse

[Masahiro Yamada]

The introduction of these dummy BUILD_BUG_ON stubs dates back to
commit 903c0c7cdc21 ("sparse: define dummy BUILD_BUG_ON definition
for sparse"). At that time, BUILD_BUG_ON() was implemented with the
negative array trick, which Sparse complains about even if the
condition can be optimized and evaluated to 0 at compile-time.

With the previous commit, the leftover negative array trick is gone.
Sparse is happy with the current BUILD_BUG_ON(), which is implemented
by using the 'error' attribute.

There might be a little room for argument about BUILD_BUG_ON_ZERO().
Sparse reports 'invalid bitfield width, -1' for non-zero value,
and 'bad integer constant expression' for non-constant value.
This is the same criteria as GCC uses. So, if those Sparse errors
occurred, they would cause errors for GCC as well. (Hence, such
errors would have been detected by the normal compile test process.)

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

Changes in v2:
 - Fix a coding style error (two consecutive blank lines)

 include/linux/build_bug.h | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/include/linux/build_bug.h b/include/linux/build_bug.h
index d415c64..6625c88 100644
--- a/include/linux/build_bug.h
+++ b/include/linux/build_bug.h
@@ -4,16 +4,6 @@
 
 #include <linux/compiler.h>
 
-#ifdef __CHECKER__
-#define __BUILD_BUG_ON_NOT_POWER_OF_2(n) (0)
-#define BUILD_BUG_ON_NOT_POWER_OF_2(n) (0)
-#define BUILD_BUG_ON_ZERO(e) (0)
-#define BUILD_BUG_ON_INVALID(e) (0)
-#define BUILD_BUG_ON_MSG(cond, msg) (0)
-#define BUILD_BUG_ON(condition) (0)
-#define BUILD_BUG() (0)
-#else /* __CHECKER__ */
-
 /* Force a compilation error if a constant expression is not a power of 2 */
 #define __BUILD_BUG_ON_NOT_POWER_OF_2(n)	\
 	BUILD_BUG_ON(((n) & ((n) - 1)) != 0)
@@ -64,6 +54,4 @@
  */
 #define BUILD_BUG() BUILD_BUG_ON_MSG(1, "BUILD_BUG failed")
 
-#endif	/* __CHECKER__ */
-
 #endif	/* _LINUX_BUILD_BUG_H */
-- 
2.7.4

Re: [PATCH v2 2/2] build_bug.h: remove all dummy BUILD_BUG_ON stubs for sparse

[Kees Cook]

On Fri, Nov 16, 2018 at 12:27 AM, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> The introduction of these dummy BUILD_BUG_ON stubs dates back to
> commit 903c0c7cdc21 ("sparse: define dummy BUILD_BUG_ON definition
> for sparse"). At that time, BUILD_BUG_ON() was implemented with the
> negative array trick, which Sparse complains about even if the
> condition can be optimized and evaluated to 0 at compile-time.
>
> With the previous commit, the leftover negative array trick is gone.
> Sparse is happy with the current BUILD_BUG_ON(), which is implemented
> by using the 'error' attribute.
>
> There might be a little room for argument about BUILD_BUG_ON_ZERO().
> Sparse reports 'invalid bitfield width, -1' for non-zero value,
> and 'bad integer constant expression' for non-constant value.
> This is the same criteria as GCC uses. So, if those Sparse errors
> occurred, they would cause errors for GCC as well. (Hence, such
> errors would have been detected by the normal compile test process.)
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>

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

-Kees

> ---
>
> Changes in v2:
>  - Fix a coding style error (two consecutive blank lines)
>
>  include/linux/build_bug.h | 12 ------------
>  1 file changed, 12 deletions(-)
>
> diff --git a/include/linux/build_bug.h b/include/linux/build_bug.h
> index d415c64..6625c88 100644
> --- a/include/linux/build_bug.h
> +++ b/include/linux/build_bug.h
> @@ -4,16 +4,6 @@
>
>  #include <linux/compiler.h>
>
> -#ifdef __CHECKER__
> -#define __BUILD_BUG_ON_NOT_POWER_OF_2(n) (0)
> -#define BUILD_BUG_ON_NOT_POWER_OF_2(n) (0)
> -#define BUILD_BUG_ON_ZERO(e) (0)
> -#define BUILD_BUG_ON_INVALID(e) (0)
> -#define BUILD_BUG_ON_MSG(cond, msg) (0)
> -#define BUILD_BUG_ON(condition) (0)
> -#define BUILD_BUG() (0)
> -#else /* __CHECKER__ */
> -
>  /* Force a compilation error if a constant expression is not a power of 2 */
>  #define __BUILD_BUG_ON_NOT_POWER_OF_2(n)       \
>         BUILD_BUG_ON(((n) & ((n) - 1)) != 0)
> @@ -64,6 +54,4 @@
>   */
>  #define BUILD_BUG() BUILD_BUG_ON_MSG(1, "BUILD_BUG failed")
>
> -#endif /* __CHECKER__ */
> -
>  #endif /* _LINUX_BUILD_BUG_H */
> --
> 2.7.4
>



-- 
Kees Cook

Re: [PATCH v2 2/2] build_bug.h: remove all dummy BUILD_BUG_ON stubs for sparse

[Luc Van Oostenryck]

On Fri, Nov 16, 2018 at 03:27:25PM +0900, Masahiro Yamada wrote:
> The introduction of these dummy BUILD_BUG_ON stubs dates back to
> commit 903c0c7cdc21 ("sparse: define dummy BUILD_BUG_ON definition
> for sparse"). At that time, BUILD_BUG_ON() was implemented with the
> negative array trick, which Sparse complains about even if the
> condition can be optimized and evaluated to 0 at compile-time.

OK, but from what I understand, the motivation for commit 903c0c7cdc21
was not to avoid false warnings but to avoid having twice the same
warnings: "... So it causes sparse to detect an error too. This
reduces sparse's usefulness.").

I'm not opposed to this patch (on the contrary, I think it's better
to have exactly the same code for GCC than for sparse) but I think
that your commit message need to be adjusted.

Kind regards,
-- Luc

Re: [PATCH v2 2/2] build_bug.h: remove all dummy BUILD_BUG_ON stubs for sparse

[Masahiro Yamada]

On Sat, Nov 17, 2018 at 9:33 AM Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
>
> On Fri, Nov 16, 2018 at 03:27:25PM +0900, Masahiro Yamada wrote:
> > The introduction of these dummy BUILD_BUG_ON stubs dates back to
> > commit 903c0c7cdc21 ("sparse: define dummy BUILD_BUG_ON definition
> > for sparse"). At that time, BUILD_BUG_ON() was implemented with the
> > negative array trick, which Sparse complains about even if the
> > condition can be optimized and evaluated to 0 at compile-time.
>
> OK, but from what I understand, the motivation for commit 903c0c7cdc21
> was not to avoid false warnings but to avoid having twice the same
> warnings: "... So it causes sparse to detect an error too. This
> reduces sparse's usefulness.").

In fact, Sparse was producing false positives.

I mentioned this in the commit message of v3.


> I'm not opposed to this patch (on the contrary, I think it's better
> to have exactly the same code for GCC than for sparse) but I think
> that your commit message need to be adjusted.
>
> Kind regards,
> -- Luc



-- 
Best Regards
Masahiro Yamada