[PATCH] bug: Fix CONFIG_BUG=n BUG_ON()

[PATCH] bug: Fix CONFIG_BUG=n BUG_ON()

[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().

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Josh Triplett <josh@joshtriplett.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
 include/asm-generic/bug.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index 630dd23..f3241cd 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -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
-- 
1.8.4.5

Re: [PATCH] bug: Fix CONFIG_BUG=n BUG_ON()

[Josh Triplett]

On Thu, Jun 19, 2014 at 03:28:19PM +0200, 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().

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:

> --- a/include/asm-generic/bug.h
> +++ b/include/asm-generic/bug.h
> @@ -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

Re: [PATCH] bug: Fix CONFIG_BUG=n BUG_ON()

[Arnd Bergmann]

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 <bvanassche@acm.org>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Josh Triplett <josh@joshtriplett.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> ---
>  include/asm-generic/bug.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
> index 630dd23..f3241cd 100644
> --- a/include/asm-generic/bug.h
> +++ b/include/asm-generic/bug.h
> @@ -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

Re: [PATCH] bug: Fix CONFIG_BUG=n BUG_ON()

[josh]

On Thu, Jun 19, 2014 at 07:00:14PM +0200, Arnd Bergmann wrote:
> 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 <bvanassche@acm.org>
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Cc: Josh Triplett <josh@joshtriplett.org>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > ---
> >  include/asm-generic/bug.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
> > index 630dd23..f3241cd 100644
> > --- a/include/asm-generic/bug.h
> > +++ b/include/asm-generic/bug.h
> > @@ -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.

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

Re: [PATCH] bug: Fix CONFIG_BUG=n BUG_ON()

[Bart Van Assche]

On 06/19/14 19:21, josh@joshtriplett.org 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.

Re: [PATCH] bug: Fix CONFIG_BUG=n BUG_ON()

[josh]

On Thu, Jun 19, 2014 at 07:51:42PM +0200, Bart Van Assche wrote:
> On 06/19/14 19:21, josh@joshtriplett.org 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.

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