[PATCH RESEND] bug: When !CONFIG_BUG, simplify WARN_ON_ONCE and family

[PATCH RESEND] bug: When !CONFIG_BUG, simplify WARN_ON_ONCE and family

[Josh Triplett]

When !CONFIG_BUG, WARN_ON and family become simple passthroughs of their
condition argument; however, WARN_ON_ONCE and family still have
conditions and a boolean to detect one-time invocation, even though the
warning they'd emit doesn't exist.  Make the existing definitions
conditional on CONFIG_BUG, and map them all to the passthrough WARN_ON
when !CONFIG_BUG.

This saves 4.4k on a minimized configuration (smaller than
allnoconfig), and 20.6k with defconfig plus CONFIG_BUG=n.

Signed-off-by: Josh Triplett <josh@joshtriplett.org>
---

diff's presentation here is not the easiest for reviewing purposes.
Effectively, this is moving the existing definitions of WARN_ON_ONCE and
family up into the CONFIG_BUG branch of the #ifdef, and then adding new
stub versions in the !CONFIG_BUG branch.

 include/asm-generic/bug.h | 57 +++++++++++++++++++++++++----------------------
 1 file changed, 30 insertions(+), 27 deletions(-)

diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index 7d10f96..34223e0 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -106,33 +106,6 @@ extern void warn_slowpath_null(const char *file, const int line);
 	unlikely(__ret_warn_on);					\
 })
 
-#else /* !CONFIG_BUG */
-#ifndef HAVE_ARCH_BUG
-#define BUG() do {} while(0)
-#endif
-
-#ifndef HAVE_ARCH_BUG_ON
-#define BUG_ON(condition) do { if (condition) ; } while(0)
-#endif
-
-#ifndef HAVE_ARCH_WARN_ON
-#define WARN_ON(condition) ({						\
-	int __ret_warn_on = !!(condition);				\
-	unlikely(__ret_warn_on);					\
-})
-#endif
-
-#ifndef WARN
-#define WARN(condition, format...) ({					\
-	int __ret_warn_on = !!(condition);				\
-	unlikely(__ret_warn_on);					\
-})
-#endif
-
-#define WARN_TAINT(condition, taint, format...) WARN_ON(condition)
-
-#endif
-
 #define WARN_ON_ONCE(condition)	({				\
 	static bool __section(.data.unlikely) __warned;		\
 	int __ret_warn_once = !!(condition);			\
@@ -163,6 +136,36 @@ extern void warn_slowpath_null(const char *file, const int line);
 	unlikely(__ret_warn_once);				\
 })
 
+#else /* !CONFIG_BUG */
+#ifndef HAVE_ARCH_BUG
+#define BUG() do {} while(0)
+#endif
+
+#ifndef HAVE_ARCH_BUG_ON
+#define BUG_ON(condition) do { if (condition) ; } while(0)
+#endif
+
+#ifndef HAVE_ARCH_WARN_ON
+#define WARN_ON(condition) ({						\
+	int __ret_warn_on = !!(condition);				\
+	unlikely(__ret_warn_on);					\
+})
+#endif
+
+#ifndef WARN
+#define WARN(condition, format...) ({					\
+	int __ret_warn_on = !!(condition);				\
+	unlikely(__ret_warn_on);					\
+})
+#endif
+
+#define WARN_ON_ONCE(condition) WARN_ON(condition)
+#define WARN_ONCE(condition, format...) WARN_ON(condition)
+#define WARN_TAINT(condition, taint, format...) WARN_ON(condition)
+#define WARN_TAINT_ONCE(condition, taint, format...) WARN_ON(condition)
+
+#endif
+
 /*
  * WARN_ON_SMP() is for cases that the warning is either
  * meaningless for !SMP or may even cause failures.
-- 
1.9.0

Re: [PATCH RESEND] bug: When !CONFIG_BUG, simplify WARN_ON_ONCE and family

[Randy Dunlap]

On 02/22/2014 11:23 AM, Josh Triplett wrote:

Hi Josh,

If you redo these patches, please make while(0) not look like a
function call, i.e., use while (0) instead.

> +#else /* !CONFIG_BUG */
> +#ifndef HAVE_ARCH_BUG
> +#define BUG() do {} while(0)
> +#endif
> +
> +#ifndef HAVE_ARCH_BUG_ON
> +#define BUG_ON(condition) do { if (condition) ; } while(0)
> +#endif

Thanks.
-- 
~Randy

Re: [PATCH RESEND] bug: When !CONFIG_BUG, simplify WARN_ON_ONCE and family

[Josh Triplett]

On Sat, Feb 22, 2014 at 01:54:49PM -0800, Randy Dunlap wrote:
> On 02/22/2014 11:23 AM, Josh Triplett wrote:
> 
> Hi Josh,
> 
> If you redo these patches, please make while(0) not look like a
> function call, i.e., use while (0) instead.

Good catch.  Fixing in v2.

- Josh Triplett

Re: [PATCH RESEND] bug: When !CONFIG_BUG, simplify WARN_ON_ONCE and family

[Josh Triplett]

On Sat, Feb 22, 2014 at 02:31:20PM -0800, Josh Triplett wrote:
> On Sat, Feb 22, 2014 at 01:54:49PM -0800, Randy Dunlap wrote:
> > On 02/22/2014 11:23 AM, Josh Triplett wrote:
> > 
> > Hi Josh,
> > 
> > If you redo these patches, please make while(0) not look like a
> > function call, i.e., use while (0) instead.
> 
> Good catch.  Fixing in v2.

Looking at the patch, those while(0)s were all in the original code, not
introduced by my patch.  (diff claims my patch moves them, but a better
diff would have shown that I left them alone and just moved other code
above them.)

With that in mind, I didn't want to change that style issue in the same
patch, but I wrote a separate style patch to be applied on top:

----- >8 -----
>From 2a80f851b507f44142d219af3cf00555e333f4c4 Mon Sep 17 00:00:00 2001
From: Josh Triplett <josh@joshtriplett.org>
Date: Sat, 22 Feb 2014 14:33:01 -0800
Subject: [PATCH] include/asm-generic/bug.h: Style fix: s/while(0)/while (0)/

Reported-by: Randy Dunlap <rdunlap@infradead.org>
Signed-off-by: Josh Triplett <josh@joshtriplett.org>
---
 include/asm-generic/bug.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index 34223e0..5f5a277 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -52,7 +52,7 @@ struct bug_entry {
 #endif
 
 #ifndef HAVE_ARCH_BUG_ON
-#define BUG_ON(condition) do { if (unlikely(condition)) BUG(); } while(0)
+#define BUG_ON(condition) do { if (unlikely(condition)) BUG(); } while (0)
 #endif
 
 /*
@@ -138,11 +138,11 @@ extern void warn_slowpath_null(const char *file, const int line);
 
 #else /* !CONFIG_BUG */
 #ifndef HAVE_ARCH_BUG
-#define BUG() do {} while(0)
+#define BUG() do {} while (0)
 #endif
 
 #ifndef HAVE_ARCH_BUG_ON
-#define BUG_ON(condition) do { if (condition) ; } while(0)
+#define BUG_ON(condition) do { if (condition) ; } while (0)
 #endif
 
 #ifndef HAVE_ARCH_WARN_ON
-- 
1.9.0

Re: [PATCH RESEND] bug: When !CONFIG_BUG, simplify WARN_ON_ONCE and family

[Josh Triplett]

On Sat, Feb 22, 2014 at 02:31:20PM -0800, Josh Triplett wrote:
> On Sat, Feb 22, 2014 at 01:54:49PM -0800, Randy Dunlap wrote:
> > On 02/22/2014 11:23 AM, Josh Triplett wrote:
> > 
> > Hi Josh,
> > 
> > If you redo these patches, please make while(0) not look like a
> > function call, i.e., use while (0) instead.
> 
> Good catch.  Fixing in v2.

Looking at the patch, those while(0)s were all in the original code, not
introduced by my patch.  (diff claims my patch moves them, but a better
diff would have shown that I left them alone and just moved other code
above them.)

With that in mind, I didn't want to change that style issue in the same
patch, but I wrote a separate style patch to be applied on top:

----- >8 -----
From 2a80f851b507f44142d219af3cf00555e333f4c4 Mon Sep 17 00:00:00 2001
From: Josh Triplett <josh@joshtriplett.org>
Date: Sat, 22 Feb 2014 14:33:01 -0800
Subject: [PATCH] include/asm-generic/bug.h: Style fix: s/while(0)/while (0)/

Reported-by: Randy Dunlap <rdunlap@infradead.org>
Signed-off-by: Josh Triplett <josh@joshtriplett.org>
---
 include/asm-generic/bug.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index 34223e0..5f5a277 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -52,7 +52,7 @@ struct bug_entry {
 #endif
 
 #ifndef HAVE_ARCH_BUG_ON
-#define BUG_ON(condition) do { if (unlikely(condition)) BUG(); } while(0)
+#define BUG_ON(condition) do { if (unlikely(condition)) BUG(); } while (0)
 #endif
 
 /*
@@ -138,11 +138,11 @@ extern void warn_slowpath_null(const char *file, const int line);
 
 #else /* !CONFIG_BUG */
 #ifndef HAVE_ARCH_BUG
-#define BUG() do {} while(0)
+#define BUG() do {} while (0)
 #endif
 
 #ifndef HAVE_ARCH_BUG_ON
-#define BUG_ON(condition) do { if (condition) ; } while(0)
+#define BUG_ON(condition) do { if (condition) ; } while (0)
 #endif
 
 #ifndef HAVE_ARCH_WARN_ON
-- 
1.9.0

Re: [PATCH RESEND] bug: When !CONFIG_BUG, simplify WARN_ON_ONCE and family

[Arnd Bergmann]

On Saturday 22 February 2014, Josh Triplett wrote:
> When !CONFIG_BUG, WARN_ON and family become simple passthroughs of their
> condition argument; however, WARN_ON_ONCE and family still have
> conditions and a boolean to detect one-time invocation, even though the
> warning they'd emit doesn't exist.  Make the existing definitions
> conditional on CONFIG_BUG, and map them all to the passthrough WARN_ON
> when !CONFIG_BUG.
> 
> This saves 4.4k on a minimized configuration (smaller than
> allnoconfig), and 20.6k with defconfig plus CONFIG_BUG=n.

This looks good, but it reminds me of a patch that I did a while ago
and that got lost while I was on leave:

> +#else /* !CONFIG_BUG */
> +#ifndef HAVE_ARCH_BUG
> +#define BUG() do {} while(0)
> +#endif
> +
> +#ifndef HAVE_ARCH_BUG_ON
> +#define BUG_ON(condition) do { if (condition) ; } while(0)
> +#endif

I've done some analysis of this before[1] and came to the conclusion that
this definition (which I realize you are not changing) is bad.

For one thing, it will cause lots of gcc warnings about code that
should have been unreachable being compiled. It also causes
misoptimizations for code that should be detected as unused or
(worse) lets us run into undefined behavior if we ever get into
the BUG() case.

This means we actually want BUG() to end with __builtin_unreachable()
as in the CONFIG_BUG=y case, and also ensure it actually is
unreachable. As I have shown in [1], the there is a small overhead
of doing this in terms of code size.

> +#ifndef HAVE_ARCH_WARN_ON
> +#define WARN_ON(condition) ({						\
> +	int __ret_warn_on = !!(condition);				\
> +	unlikely(__ret_warn_on);					\
> +})
> +#endif
> +
> +#ifndef WARN
> +#define WARN(condition, format...) ({					\
> +	int __ret_warn_on = !!(condition);				\
> +	unlikely(__ret_warn_on);					\
> +})
> +#endif


FWIW, there is an easy extension to this to get rid of some "unused variable"
warnings, but using the format string in an unreachable part of the macro,
as I did in my patch (but didn't explain there):

@@ -125,6 +126,8 @@ extern void warn_slowpath_null(const char *file, const int line);
 #ifndef WARN
 #define WARN(condition, format...) ({					\
 	int __ret_warn_on = !!(condition);				\
+	if (0 && (__ret_warn_on))					\
+		printk(format);						\
 	unlikely(__ret_warn_on);					\
 })
 #endif


	Arnd

[1] http://lkml.org/lkml/2013/7/12/121

Re: [PATCH RESEND] bug: When !CONFIG_BUG, simplify WARN_ON_ONCE and family

[Josh Triplett]

On Mon, Feb 24, 2014 at 09:02:35AM +0100, Arnd Bergmann wrote:
> On Saturday 22 February 2014, Josh Triplett wrote:
> > When !CONFIG_BUG, WARN_ON and family become simple passthroughs of their
> > condition argument; however, WARN_ON_ONCE and family still have
> > conditions and a boolean to detect one-time invocation, even though the
> > warning they'd emit doesn't exist.  Make the existing definitions
> > conditional on CONFIG_BUG, and map them all to the passthrough WARN_ON
> > when !CONFIG_BUG.
> > 
> > This saves 4.4k on a minimized configuration (smaller than
> > allnoconfig), and 20.6k with defconfig plus CONFIG_BUG=n.
> 
> This looks good, but it reminds me of a patch that I did a while ago
> and that got lost while I was on leave:
> 
> > +#else /* !CONFIG_BUG */
> > +#ifndef HAVE_ARCH_BUG
> > +#define BUG() do {} while(0)
> > +#endif
> > +
> > +#ifndef HAVE_ARCH_BUG_ON
> > +#define BUG_ON(condition) do { if (condition) ; } while(0)
> > +#endif
> 
> I've done some analysis of this before[1] and came to the conclusion that
> this definition (which I realize you are not changing) is bad.
> 
> For one thing, it will cause lots of gcc warnings about code that
> should have been unreachable being compiled. It also causes
> misoptimizations for code that should be detected as unused or
> (worse) lets us run into undefined behavior if we ever get into
> the BUG() case.
> 
> This means we actually want BUG() to end with __builtin_unreachable()
> as in the CONFIG_BUG=y case, and also ensure it actually is
> unreachable. As I have shown in [1], the there is a small overhead
> of doing this in terms of code size.

I agree that allowing BUG() to become a no-op seems suboptimal, if only
because of the resulting warnings and mis-optimizations.  However, I
think the overhead could be cut down massively, such that BUG() just
compiles down to a one-byte undefined instruction.  (__builtin_trap()
might do the right thing here; worth checking.)

That said, I think there's still a reasonable argument to be made for
allowing it to be cut down to just __builtin_unreachable() without a
trap, for much the same reason that noreturn doesn't generate code
expecting the function to return.  A one-byte undefined instruction
isn't that bad, but that also means BUG_ON has to generate the
conditional and the conditional jump around that instruction.

(Consider the reasoning behind CONFIG_DOUBLEFAULT, and its Kconfig help
text.)

On the other hand, it's also a good argument for converting far more
BUGs to WARNs and BUG_ONs to WARN_ONs.

In any case, while I agree that the change you're proposing seems
reasonable, I don't think it should occur as part of this patch.  Do you
think this patch seems reasonable, and that further patches could occur
on top of it for the behavior you describe?

> > +#ifndef HAVE_ARCH_WARN_ON
> > +#define WARN_ON(condition) ({						\
> > +	int __ret_warn_on = !!(condition);				\
> > +	unlikely(__ret_warn_on);					\
> > +})
> > +#endif
> > +
> > +#ifndef WARN
> > +#define WARN(condition, format...) ({					\
> > +	int __ret_warn_on = !!(condition);				\
> > +	unlikely(__ret_warn_on);					\
> > +})
> > +#endif
> 
> FWIW, there is an easy extension to this to get rid of some "unused variable"
> warnings, but using the format string in an unreachable part of the macro,
> as I did in my patch (but didn't explain there):
> 
> @@ -125,6 +126,8 @@ extern void warn_slowpath_null(const char *file, const int line);
>  #ifndef WARN
>  #define WARN(condition, format...) ({					\
>  	int __ret_warn_on = !!(condition);				\
> +	if (0 && (__ret_warn_on))					\
> +		printk(format);						\
>  	unlikely(__ret_warn_on);					\
>  })
>  #endif

This seems better done via no_printk, but yes, that seems sensible.
The arguments should be used too.

(But again, I'm not actually changing these in this patch; diff just
thinks I'm moving them.)

- Josh Triplett

Re: [PATCH RESEND] bug: When !CONFIG_BUG, simplify WARN_ON_ONCE and family

[Arnd Bergmann]

On Monday 24 February 2014 00:44:37 Josh Triplett wrote:
> 
> I agree that allowing BUG() to become a no-op seems suboptimal, if only
> because of the resulting warnings and mis-optimizations.  However, I
> think the overhead could be cut down massively, such that BUG() just
> compiles down to a one-byte undefined instruction.  (__builtin_trap()
> might do the right thing here; worth checking.)

__builtin_trap() sounds great. Is that available on all supported
architectures and gcc versions?

> In any case, while I agree that the change you're proposing seems
> reasonable, I don't think it should occur as part of this patch.  Do you
> think this patch seems reasonable, and that further patches could occur
> on top of it for the behavior you describe?

Yes, that's probably fine. I was hoping I could get you to address
both issues at once, but they are really different as you say, and
I'm pretty sure that applying my patch wouldn't change much regarding
the benifit of yours in terms of object code size changes.

One thing that would make it slightly easier for me later though:

> +#define WARN_ON_ONCE(condition) WARN_ON(condition)
> +#define WARN_ONCE(condition, format...) WARN_ON(condition)
> +#define WARN_TAINT(condition, taint, format...) WARN_ON(condition)
> +#define WARN_TAINT_ONCE(condition, taint, format...) WARN_ON(condition)

Can you change those that come with a format to call WARN() rather than
WARN_ON()?

> > FWIW, there is an easy extension to this to get rid of some "unused variable"
> > warnings, but using the format string in an unreachable part of the macro,
> > as I did in my patch (but didn't explain there):
> > 
> > @@ -125,6 +126,8 @@ extern void warn_slowpath_null(const char *file, const int line);
> >  #ifndef WARN
> >  #define WARN(condition, format...) ({					\
> >  	int __ret_warn_on = !!(condition);				\
> > +	if (0 && (__ret_warn_on))					\
> > +		printk(format);						\
> >  	unlikely(__ret_warn_on);					\
> >  })
> >  #endif
> 
> This seems better done via no_printk, but yes, that seems sensible.
> The arguments should be used too.
> 
> (But again, I'm not actually changing these in this patch; diff just
> thinks I'm moving them.)

I understand. It's also unrelated to what I was doing in my patch.
Do you mind adding a second patch on top of yours to do no_printk()
here? It's a trivial change and since you're touching that code already
I think it makes sense to keep it with your series, even if it comes
as a second patch.

	Arnd

Re: [PATCH RESEND] bug: When !CONFIG_BUG, simplify WARN_ON_ONCE and family

[David Howells]

Josh Triplett <josh@joshtriplett.org> wrote:

> > This means we actually want BUG() to end with __builtin_unreachable()
> > as in the CONFIG_BUG=y case, and also ensure it actually is
> > unreachable. As I have shown in [1], the there is a small overhead
> > of doing this in terms of code size.
> 
> I agree that allowing BUG() to become a no-op seems suboptimal, if only
> because of the resulting warnings and mis-optimizations.  However, I
> think the overhead could be cut down massively, such that BUG() just
> compiles down to a one-byte undefined instruction.  (__builtin_trap()
> might do the right thing here; worth checking.)

Is it possible to use an inline function with an empty body in this?  Let the
compiler prune away all the arguments?

David

Re: [PATCH RESEND] bug: When !CONFIG_BUG, simplify WARN_ON_ONCE and family

[Arnd Bergmann]

On Monday 24 February 2014 12:09:11 David Howells wrote:
> Josh Triplett <josh@joshtriplett.org> wrote:
> 
> > > This means we actually want BUG() to end with __builtin_unreachable()
> > > as in the CONFIG_BUG=y case, and also ensure it actually is
> > > unreachable. As I have shown in [1], the there is a small overhead
> > > of doing this in terms of code size.
> > 
> > I agree that allowing BUG() to become a no-op seems suboptimal, if only
> > because of the resulting warnings and mis-optimizations.  However, I
> > think the overhead could be cut down massively, such that BUG() just
> > compiles down to a one-byte undefined instruction.  (__builtin_trap()
> > might do the right thing here; worth checking.)
> 
> Is it possible to use an inline function with an empty body in this?  Let the
> compiler prune away all the arguments?
> 

I think you are thinking of a different problem. BUG() already takes no
arguments, but we get warnings for code like this:

int f(void)
{
	switch (global) {
	case 0:
		return 0;
	case 1:
		return do_something();
	default:
		BUG();
	}
}

BUG() normally causes a fault and we print helpful messages before killing
the task, and gcc knows we never continue because of the
__builtin_unreachable() annotation.

If BUG() is defined as 'do { } while (0)' in the example above, we get
a warning because the function may end without returning a number.
If we define it to 'do { unreachable(); } while (0)', we don't get a
warning, but we can get undefined behavior in the case we ever get to
the end of the function.

__builtin_trap() would be nice as an architecture independent method,
but I just checked what it does on ARM, and it just calls abort(),
which is probably not what we want in the kernel, especially as long
as the ARM abort() function calls BUG(), and most architectures don't
define it at all.

'do { } while (1)' is probably a reasonably generic way to not return
and should compile into a single instruction on most architectures.
panic() would be another option to do something that won't cause
additional data corruption, but it's a varargs function, so that won't
help you much when you just try to save object code size.

	Arnd

Re: [PATCH RESEND] bug: When !CONFIG_BUG, simplify WARN_ON_ONCE and family

[One Thousand Gnomes]

> BUG() normally causes a fault and we print helpful messages before killing
> the task, and gcc knows we never continue because of the
> __builtin_unreachable() annotation.
> 
> If BUG() is defined as 'do { } while (0)' in the example above, we get
> a warning because the function may end without returning a number.
> If we define it to 'do { unreachable(); } while (0)', we don't get a
> warning, but we can get undefined behavior in the case we ever get to
> the end of the function.

That warning is the right thing though. In a lot of cases BUG(); is
followed by code that can lead to serious corruption and potentially
things like disk corruption following or security compromise.

We *should* be warning if you are stupid enough to build a kernel where
BUG() does not terminate.

While I agree defining it as do {} while(1); would be a lot smarter,
simply making it required that a platform provides an implementation of
BUG() would be even better. 

Alan

Re: [PATCH RESEND] bug: When !CONFIG_BUG, simplify WARN_ON_ONCE and family

[Arnd Bergmann]

On Monday 24 February 2014 13:16:05 One Thousand Gnomes wrote:
> 
> While I agree defining it as do {} while(1); would be a lot smarter,
> simply making it required that a platform provides an implementation of
> BUG() would be even better. 

But how do we get there? The majority of architectures define BUG()
already, but a lot of them only if CONFIG_BUG is enabled. That is
of course trivial do change, and it is what my older patch does
for ARM and x86.

We also have seven architectures without a BUG() implementation (c6x,
m32r, meta, microblaze, score, sh, unicore32) and six more (arm64,
hexagon openrisc tile um xtensa) that have no bug.h at all.

	Arnd

Re: [PATCH RESEND] bug: When !CONFIG_BUG, simplify WARN_ON_ONCE and family

[Andrew Morton]

On Mon, 24 Feb 2014 09:02:35 +0100 Arnd Bergmann <arnd@arndb.de> wrote:

> On Saturday 22 February 2014, Josh Triplett wrote:
> > When !CONFIG_BUG, WARN_ON and family become simple passthroughs of their
> > condition argument; however, WARN_ON_ONCE and family still have
> > conditions and a boolean to detect one-time invocation, even though the
> > warning they'd emit doesn't exist.  Make the existing definitions
> > conditional on CONFIG_BUG, and map them all to the passthrough WARN_ON
> > when !CONFIG_BUG.
> > 
> > This saves 4.4k on a minimized configuration (smaller than
> > allnoconfig), and 20.6k with defconfig plus CONFIG_BUG=n.
> 
> This looks good, but it reminds me of a patch that I did a while ago
> and that got lost while I was on leave:
> 
> > +#else /* !CONFIG_BUG */
> > +#ifndef HAVE_ARCH_BUG
> > +#define BUG() do {} while(0)
> > +#endif
> > +
> > +#ifndef HAVE_ARCH_BUG_ON
> > +#define BUG_ON(condition) do { if (condition) ; } while(0)
> > +#endif
> 
> I've done some analysis of this before[1] and came to the conclusion that
> this definition (which I realize you are not changing) is bad.
> 
> For one thing, it will cause lots of gcc warnings about code that
> should have been unreachable being compiled. It also causes
> misoptimizations for code that should be detected as unused or
> (worse) lets us run into undefined behavior if we ever get into
> the BUG() case.
> 
> This means we actually want BUG() to end with __builtin_unreachable()
> as in the CONFIG_BUG=y case, and also ensure it actually is
> unreachable. As I have shown in [1], the there is a small overhead
> of doing this in terms of code size.

CONFIG_BUG=n causes all sorts of stupid problems.

And as kernel developers we don't *want* people disabling BUG - it
reduces our ability to detect and fix stuff and it adds all sorts of
hard-to-maintain nobody-tests-it things like this.

So...  how about we just do away with CONFIG_BUG?

- Do we know of anyone who is really using this and to good effect?

- Is their use case important/valuable enough for us to continue bothering?

Re: [PATCH RESEND] bug: When !CONFIG_BUG, simplify WARN_ON_ONCE and family

[Josh Triplett]

On Mon, Feb 24, 2014 at 03:17:58PM -0800, Andrew Morton wrote:
> On Mon, 24 Feb 2014 09:02:35 +0100 Arnd Bergmann <arnd@arndb.de> wrote:
> 
> > On Saturday 22 February 2014, Josh Triplett wrote:
> > > When !CONFIG_BUG, WARN_ON and family become simple passthroughs of their
> > > condition argument; however, WARN_ON_ONCE and family still have
> > > conditions and a boolean to detect one-time invocation, even though the
> > > warning they'd emit doesn't exist.  Make the existing definitions
> > > conditional on CONFIG_BUG, and map them all to the passthrough WARN_ON
> > > when !CONFIG_BUG.
> > > 
> > > This saves 4.4k on a minimized configuration (smaller than
> > > allnoconfig), and 20.6k with defconfig plus CONFIG_BUG=n.
> > 
> > This looks good, but it reminds me of a patch that I did a while ago
> > and that got lost while I was on leave:
> > 
> > > +#else /* !CONFIG_BUG */
> > > +#ifndef HAVE_ARCH_BUG
> > > +#define BUG() do {} while(0)
> > > +#endif
> > > +
> > > +#ifndef HAVE_ARCH_BUG_ON
> > > +#define BUG_ON(condition) do { if (condition) ; } while(0)
> > > +#endif
> > 
> > I've done some analysis of this before[1] and came to the conclusion that
> > this definition (which I realize you are not changing) is bad.
> > 
> > For one thing, it will cause lots of gcc warnings about code that
> > should have been unreachable being compiled. It also causes
> > misoptimizations for code that should be detected as unused or
> > (worse) lets us run into undefined behavior if we ever get into
> > the BUG() case.
> > 
> > This means we actually want BUG() to end with __builtin_unreachable()
> > as in the CONFIG_BUG=y case, and also ensure it actually is
> > unreachable. As I have shown in [1], the there is a small overhead
> > of doing this in terms of code size.
> 
> CONFIG_BUG=n causes all sorts of stupid problems.
> 
> And as kernel developers we don't *want* people disabling BUG - it
> reduces our ability to detect and fix stuff and it adds all sorts of
> hard-to-maintain nobody-tests-it things like this.

I think Arnd's approach addresses this case nicely: do away with the
message, keep something isomorphic to a messageless panic.  And ideally
that could be reduced to a one-byte undefined instruction potentially
wrapped in a conditional.

> So...  how about we just do away with CONFIG_BUG?
> 
> - Do we know of anyone who is really using this and to good effect?

I'm using it, or I wouldn't have submitted the patch.

If you're on a system with no output whatsoever, the kind of system
where CONFIG_PRINTK=n makes sense, then CONFIG_BUG=n also makes sense.
That said, I completely understand and agree with Arnd's argument that
BUG should still compile to a panic-equivalent, rather than allowing
execution to continue from that point and thus allow undefined behavior.
A reboot is preferable.

So, is it CONFIG_BUG=n you're objecting to, or just the current
implementation of it in which BUG() becomes a no-op rather than a
messageless panic?

- Josh Triplett