diff mbox series

[v3,3/3] build_bug.h: remove most of dummy BUILD_BUG_ON stubs for sparse

Message ID 1542623503-3755-3-git-send-email-yamada.masahiro@socionext.com
State Superseded
Headers show
Series [v3,1/3] compiler_types.h: make __builtin_types_compatible_p() noop for Sparse | expand

Commit Message

Masahiro Yamada Nov. 19, 2018, 10:31 a.m. UTC
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 *and* the link-time trick, like this:

  extern int __build_bug_on_failed;
  #define BUILD_BUG_ON(condition)                                \
          do {                                                   \
                  ((void)sizeof(char[1 - 2*!!(condition)]));     \
                  if (condition) __build_bug_on_failed = 1;      \
          } while(0)

Sparse is more strict about the negative array trick than GCC because
Sparse requires the array length to be really constant.

Here is the simple test code for the macro above:

  static const int x = 0;
  BUILD_BUG_ON(x);

GCC is absolutely fine with it (-Wvla was not enabled at that time),
but Sparse warns like this:

  error: bad constant expression
  error: cannot size expression

(If you are using a newer version of Sparse, you will see a different
warning message, "warning: Variable length array is used".)

Anyway, Sparse was producing many false positive warnings, hence
silenced.

With the previous commit, the leftover negative array trick is gone.
Sparse is fine with the current BUILD_BUG_ON(), which is implemented
by using the 'error' attribute. (assuming your Sparse version supports
-Wno-unknown-attribute option)

I am keeping the stub for BUILD_BUG_ON_ZERO(). Otherwise, Sparse
would complain about the following code, which GCC is fine with:

  static const int x = 0;
  int y = BUILD_BUG_ON_ZERO(x);

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

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

---

Changes in v3:
 - Add Kees' Acked-by
 - Clarify log that BUILD_BUG_ON() *was* producing false positives
 - Keep the stub for BUILD_BUG_ON_ZERO()

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

 include/linux/build_bug.h | 22 +++++++---------------
 1 file changed, 7 insertions(+), 15 deletions(-)

-- 
2.7.4

Comments

Luc Van Oostenryck Nov. 19, 2018, 12:37 p.m. UTC | #1
On Mon, Nov 19, 2018 at 07:31:43PM +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 *and* the link-time trick, like this:

> 

>   extern int __build_bug_on_failed;

>   #define BUILD_BUG_ON(condition)                                \

>           do {                                                   \

>                   ((void)sizeof(char[1 - 2*!!(condition)]));     \

>                   if (condition) __build_bug_on_failed = 1;      \

>           } while(0)

> 

> Sparse is more strict about the negative array trick than GCC because

> Sparse requires the array length to be really constant.

> 

> Here is the simple test code for the macro above:

> 

>   static const int x = 0;

>   BUILD_BUG_ON(x);

> 

> GCC is absolutely fine with it (-Wvla was not enabled at that time),

> but Sparse warns like this:

> 

>   error: bad constant expression

>   error: cannot size expression

> 

> (If you are using a newer version of Sparse, you will see a different

> warning message, "warning: Variable length array is used".)

> 

> Anyway, Sparse was producing many false positive warnings, hence

> silenced.

> 

> With the previous commit, the leftover negative array trick is gone.

> Sparse is fine with the current BUILD_BUG_ON(), which is implemented

> by using the 'error' attribute. (assuming your Sparse version supports

> -Wno-unknown-attribute option)

> 

> I am keeping the stub for BUILD_BUG_ON_ZERO(). Otherwise, Sparse

> would complain about the following code, which GCC is fine with:

> 

>   static const int x = 0;

>   int y = BUILD_BUG_ON_ZERO(x);

> 

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

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


Reviewed-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
Nick Desaulniers Nov. 19, 2018, 6 p.m. UTC | #2
On Mon, Nov 19, 2018 at 4:37 AM Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
>

> On Mon, Nov 19, 2018 at 07:31:43PM +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 *and* the link-time trick, like this:

> >

> >   extern int __build_bug_on_failed;

> >   #define BUILD_BUG_ON(condition)                                \

> >           do {                                                   \

> >                   ((void)sizeof(char[1 - 2*!!(condition)]));     \

> >                   if (condition) __build_bug_on_failed = 1;      \

> >           } while(0)

> >

> > Sparse is more strict about the negative array trick than GCC because

> > Sparse requires the array length to be really constant.

> >

> > Here is the simple test code for the macro above:

> >

> >   static const int x = 0;

> >   BUILD_BUG_ON(x);

> >

> > GCC is absolutely fine with it (-Wvla was not enabled at that time),

> > but Sparse warns like this:

> >

> >   error: bad constant expression

> >   error: cannot size expression

> >

> > (If you are using a newer version of Sparse, you will see a different

> > warning message, "warning: Variable length array is used".)

> >

> > Anyway, Sparse was producing many false positive warnings, hence

> > silenced.

> >

> > With the previous commit, the leftover negative array trick is gone.

> > Sparse is fine with the current BUILD_BUG_ON(), which is implemented

> > by using the 'error' attribute. (assuming your Sparse version supports

> > -Wno-unknown-attribute option)

> >

> > I am keeping the stub for BUILD_BUG_ON_ZERO(). Otherwise, Sparse

> > would complain about the following code, which GCC is fine with:

> >

> >   static const int x = 0;

> >   int y = BUILD_BUG_ON_ZERO(x);

> >

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

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

>

> Reviewed-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>


Clang builds not affected. Tested a quick arm64 defconfig build with
Clang + this patch.
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

Tested-by: Nick Desaulniers <ndesaulniers@google.com>


-- 
Thanks,
~Nick Desaulniers
Masahiro Yamada Nov. 20, 2018, 1:30 a.m. UTC | #3
On Tue, Nov 20, 2018 at 3:02 AM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>

> On Mon, Nov 19, 2018 at 4:37 AM Luc Van Oostenryck

> <luc.vanoostenryck@gmail.com> wrote:

> >

> > On Mon, Nov 19, 2018 at 07:31:43PM +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 *and* the link-time trick, like this:

> > >

> > >   extern int __build_bug_on_failed;

> > >   #define BUILD_BUG_ON(condition)                                \

> > >           do {                                                   \

> > >                   ((void)sizeof(char[1 - 2*!!(condition)]));     \

> > >                   if (condition) __build_bug_on_failed = 1;      \

> > >           } while(0)

> > >

> > > Sparse is more strict about the negative array trick than GCC because

> > > Sparse requires the array length to be really constant.

> > >

> > > Here is the simple test code for the macro above:

> > >

> > >   static const int x = 0;

> > >   BUILD_BUG_ON(x);

> > >

> > > GCC is absolutely fine with it (-Wvla was not enabled at that time),

> > > but Sparse warns like this:

> > >

> > >   error: bad constant expression

> > >   error: cannot size expression

> > >

> > > (If you are using a newer version of Sparse, you will see a different

> > > warning message, "warning: Variable length array is used".)

> > >

> > > Anyway, Sparse was producing many false positive warnings, hence

> > > silenced.

> > >

> > > With the previous commit, the leftover negative array trick is gone.

> > > Sparse is fine with the current BUILD_BUG_ON(), which is implemented

> > > by using the 'error' attribute. (assuming your Sparse version supports

> > > -Wno-unknown-attribute option)

> > >

> > > I am keeping the stub for BUILD_BUG_ON_ZERO(). Otherwise, Sparse

> > > would complain about the following code, which GCC is fine with:

> > >

> > >   static const int x = 0;

> > >   int y = BUILD_BUG_ON_ZERO(x);

> > >

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

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

> >

> > Reviewed-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>

>

> Clang builds not affected. Tested a quick arm64 defconfig build with

> Clang + this patch.

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

> Tested-by: Nick Desaulniers <ndesaulniers@google.com>

>



This patch can go in only when 1/3 is acceptable.

But, I see 1/3 is controversial.



-- 
Best Regards
Masahiro Yamada
diff mbox series

Patch

diff --git a/include/linux/build_bug.h b/include/linux/build_bug.h
index d415c64..faeec74 100644
--- a/include/linux/build_bug.h
+++ b/include/linux/build_bug.h
@@ -5,21 +5,8 @@ 
 #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)
-#define BUILD_BUG_ON_NOT_POWER_OF_2(n)			\
-	BUILD_BUG_ON((n) == 0 || (((n) & ((n) - 1)) != 0))
-
 /*
  * Force a compilation error if condition is true, but also produce a
  * result (of value 0 and type size_t), so the expression can be used
@@ -27,6 +14,13 @@ 
  * aren't permitted).
  */
 #define BUILD_BUG_ON_ZERO(e) (sizeof(struct { int:(-!!(e)); }))
+#endif /* __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)
+#define BUILD_BUG_ON_NOT_POWER_OF_2(n)			\
+	BUILD_BUG_ON((n) == 0 || (((n) & ((n) - 1)) != 0))
 
 /*
  * BUILD_BUG_ON_INVALID() permits the compiler to check the validity of the
@@ -64,6 +58,4 @@ 
  */
 #define BUILD_BUG() BUILD_BUG_ON_MSG(1, "BUILD_BUG failed")
 
-#endif	/* __CHECKER__ */
-
 #endif	/* _LINUX_BUILD_BUG_H */