Skip to contentSkip to conversation

bug: Fix CONFIG_BUG=n BUG_ON()

PATCH In review Bart Van Assche 3 participants 6 messages Jun 19, 2014 Generic include/asm header files

lore
Share & tools
lore

Bart Van Assche

author
#0 lore raw

PATCH 1/1 bug: Fix CONFIG_BUG=n BUG_ON()

Patch "bug: Make BUG() always stop the machine" changed the behavior of BUG() with CONFIG_BUG=n from a no-op into an infinite loop. Modify the definition of BUG_ON() accordingly such that the behavior of BUG_ON(1) is identical to that of BUG().

Signed-off-by Bart Van Assche <[email protected]>
Cc Arnd Bergmann <[email protected]>
Cc Josh Triplett <[email protected]>
Cc Andrew Morton <[email protected]>
---
 include/asm-generic/bug.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
include/asm-generic/bug.h +1 -1
index 630dd23..f3241cd 100644
@@ -142,7 +142,7 @@ extern void warn_slowpath_null(const char *file, const int line);
 #endif
 
 #ifndef HAVE_ARCH_BUG_ON
-#define BUG_ON(condition) do { if (condition) ; } while (0)
+#define BUG_ON(condition) do { } while (unlikely(condition))
 #endif
 
 #ifndef HAVE_ARCH_WARN_ON
Signature · 1 line
1.8.4.5
#1 lore raw
Quoted 4 lines · Bart Van Assche

Patch "bug: Make BUG() always stop the machine" changed the behavior of BUG() with CONFIG_BUG=n from a no-op into an infinite loop. Modify the definition of BUG_ON() accordingly such that the behavior of BUG_ON(1) is identical to that of BUG().

No, this patch should not go in. CONFIG_BUG=n exists for this exact purpose; if you want BUG_ON to do something then use CONFIG_BUG=y with BUGVERBOSE disabled instead.

Making BUG() stop the machine even with CONFIG_BUG=n eliminated a number of compiler warnings about code that should be unreachable, and added fairly little size to the kernel. BUG_ON, on the other hand, occurs in far more places, adds much more size if enabled, and tends to serve as an assert of a condition; it makes perfect sense to allow compiling out an assert, and compiling it out does not introduce any compiler warnings.

What problem are you trying to solve here? With this change, CONFIG_BUG=n and CONFIG_BUG=y would become effectively equivalent.

Also:

include/asm-generic/bug.h extern void warn_slowpath_null(const char *file, const int line); · line 145

Quoted 8 lines
include/asm-generic/bug.h +1 -1
@@ -142,7 +142,7 @@ extern void warn_slowpath_null(const char *file, const int line);
 #endif
 
 #ifndef HAVE_ARCH_BUG_ON
-#define BUG_ON(condition) do { if (condition) ; } while (0)
+#define BUG_ON(condition) do { } while (unlikely(condition))

Even if making this change, it should not take this form, which would evaluate the conditional repeatedly. See the form used with CONFIG_BUG=y (without HAVE_ARCH_BUG_ON), defined earlier in the same file.

- Josh Triplett

maintainer
#2 lore raw
In reply to Bart Van Assche
Quoted 19 lines · On Thursday 19 June 2014 15:28:19 Bart Van Assche wrote:

Patch "bug: Make BUG() always stop the machine" changed the behavior of BUG() with CONFIG_BUG=n from a no-op into an infinite loop. Modify the definition of BUG_ON() accordingly such that the behavior of BUG_ON(1) is identical to that of BUG().

Signed-off-by Bart Van Assche <[email protected]>
Cc Arnd Bergmann <[email protected]>
Cc Josh Triplett <[email protected]>
Cc Andrew Morton <[email protected]>
---
 include/asm-generic/bug.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
include/asm-generic/bug.h +0 -0
index 630dd23..f3241cd 100644

include/asm-generic/bug.h extern void warn_slowpath_null(const char *file, const int line); · line 148

Quoted 8 lines
include/asm-generic/bug.h +1 -1
@@ -142,7 +142,7 @@ extern void warn_slowpath_null(const char *file, const int line);
 #endif
 
 #ifndef HAVE_ARCH_BUG_ON
-#define BUG_ON(condition) do { if (condition) ; } while (0)
+#define BUG_ON(condition) do { } while (unlikely(condition))
 #endif
 
 #ifndef HAVE_ARCH_WARN_ON

How about making it

	do { if (condition) BUG(); } while (0)

That way it can be optimized for architectures that have their own BUG but not BUG_ON.

Arnd

#3 lore raw
Quoted 31 lines · Arnd Bergmann
Quoted 19 lines · On Thursday 19 June 2014 15:28:19 Bart Van Assche wrote:

Patch "bug: Make BUG() always stop the machine" changed the behavior of BUG() with CONFIG_BUG=n from a no-op into an infinite loop. Modify the definition of BUG_ON() accordingly such that the behavior of BUG_ON(1) is identical to that of BUG().

Signed-off-by Bart Van Assche <[email protected]>
Cc Arnd Bergmann <[email protected]>
Cc Josh Triplett <[email protected]>
Cc Andrew Morton <[email protected]>
---
 include/asm-generic/bug.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
include/asm-generic/bug.h +0 -0
index 630dd23..f3241cd 100644

include/asm-generic/bug.h extern void warn_slowpath_null(const char *file, const int line); · line 148

Quoted 8 lines
include/asm-generic/bug.h +1 -1
@@ -142,7 +142,7 @@ extern void warn_slowpath_null(const char *file, const int line);
 #endif
 
 #ifndef HAVE_ARCH_BUG_ON
-#define BUG_ON(condition) do { if (condition) ; } while (0)
+#define BUG_ON(condition) do { } while (unlikely(condition))
 #endif
 
 #ifndef HAVE_ARCH_WARN_ON

How about making it

Quoted 4 lines
	do { if (condition) BUG(); } while (0)

That way it can be optimized for architectures that have their own BUG but not BUG_ON.

That's exactly what BUG_ON becomes if CONFIG_BUG=y, and that significantly increases kernel size; if you want that, set CONFIG_BUG=y. BUG_ON should continue to compile to nothing if CONFIG_BUG=n, or CONFIG_BUG=n has no reason to exist.

- Josh Triplett

author
#4 lore raw
Quoted 4 lines · On 06/19/14 19:21, [email protected] wrote:

That's exactly what BUG_ON becomes if CONFIG_BUG=y, and that significantly increases kernel size; if you want that, set CONFIG_BUG=y. BUG_ON should continue to compile to nothing if CONFIG_BUG=n, or CONFIG_BUG=n has no reason to exist.

Hello Josh,

I wasn't aware that the current behavior of BUG_ON() with CONFIG_BUG=n was intentional. The reason I started looking into this is because different compiler warnings are generated for code with BUG_ON(1) statements when building against a kernel with CONFIG_BUG=y or CONFIG_BUG=n. There is an easy alternative though: changing BUG_ON(1) into BUG() in my code.

Bart.

#5 lore raw
Quoted 6 lines · Bart Van Assche
Quoted 4 lines · On 06/19/14 19:21, [email protected] wrote:

That's exactly what BUG_ON becomes if CONFIG_BUG=y, and that significantly increases kernel size; if you want that, set CONFIG_BUG=y. BUG_ON should continue to compile to nothing if CONFIG_BUG=n, or CONFIG_BUG=n has no reason to exist.

Quoted 8 lines

Hello Josh,

I wasn't aware that the current behavior of BUG_ON() with CONFIG_BUG=n was intentional. The reason I started looking into this is because different compiler warnings are generated for code with BUG_ON(1) statements when building against a kernel with CONFIG_BUG=y or CONFIG_BUG=n. There is an easy alternative though: changing BUG_ON(1) into BUG() in my code.

You should definitely never use BUG_ON(1); use BUG() if you want to say "this (and the code after it) should never be reached". That should also avoid the compiler warnings.

If you encounter any compiler warnings caused by CONFIG_BUG=n that go away with CONFIG_BUG=y, please do report them; those should get fixed.

- Josh Triplett