bug: Fix CONFIG_BUG=n BUG_ON()
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().
--- include/asm-generic/bug.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
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
Signature · 1 line
1.8.4.5
Quoted 4 lines · Bart Van Assche
Patch "bug: Make
BUG()always stop the machine" changed the behavior ofBUG()withCONFIG_BUG=nfrom a no-op into an infinite loop. Modify the definition ofBUG_ON()accordingly such that the behavior ofBUG_ON(1)is identical to that ofBUG().
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
Arnd Bergmann
maintainerQuoted 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 ofBUG()withCONFIG_BUG=nfrom a no-op into an infinite loop. Modify the definition ofBUG_ON()accordingly such that the behavior ofBUG_ON(1)is identical to that ofBUG().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
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
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 ofBUG()withCONFIG_BUG=nfrom a no-op into an infinite loop. Modify the definition ofBUG_ON()accordingly such that the behavior ofBUG_ON(1)is identical to that ofBUG().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
-0include/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
Quoted 4 lines · On 06/19/14 19:21, [email protected] wrote:
That's exactly what
BUG_ONbecomes ifCONFIG_BUG=y, and that significantly increases kernel size; if you want that, setCONFIG_BUG=y.BUG_ONshould continue to compile to nothing ifCONFIG_BUG=n, orCONFIG_BUG=nhas 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.
Quoted 6 lines · Bart Van Assche
Quoted 4 lines · On 06/19/14 19:21, [email protected] wrote:
That's exactly what
BUG_ONbecomes ifCONFIG_BUG=y, and that significantly increases kernel size; if you want that, setCONFIG_BUG=y.BUG_ONshould continue to compile to nothing ifCONFIG_BUG=n, orCONFIG_BUG=nhas no reason to exist.
Quoted 8 lines
Hello Josh,
I wasn't aware that the current behavior of
BUG_ON()withCONFIG_BUG=nwas intentional. The reason I started looking into this is because different compiler warnings are generated for code withBUG_ON(1)statements when building against a kernel withCONFIG_BUG=yorCONFIG_BUG=n. There is an easy alternative though: changingBUG_ON(1)intoBUG()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