bug: When !CONFIG_BUG, simplify WARN_ON_ONCE and family
PATCH RESEND 1/1 bug: When !CONFIG_BUG, simplify WARN_ON_ONCE and family
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.
---
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(-)
include/asm-generic/bug.h +30 -27
@@ -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.
Signature · 1 line
1.9.0
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.
include/asm-generic/bug.h extern void warn_slowpath_null(const char *file, const int line); · line 146
Quoted 8 lines
include/asm-generic/bug.h +8
-0+#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.
Signature · 1 line
~Randy
Quoted 6 lines · 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.
Good catch. Fixing in v2.
- Josh Triplett
Quoted 9 lines · Josh Triplett
Quoted 6 lines · 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.
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 <[email protected]>
Date: Sat, 22 Feb 2014 14:33:01 -0800
Subject: [PATCH] include/asm-generic/bug.h: Style fix: s/while(0)/while (0)/
--- include/asm-generic/bug.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
include/asm-generic/bug.h +3 -3
@@ -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
Signature · 1 line
1.9.0
Arnd Bergmann
maintainerQuoted 9 lines · Josh Triplett
When !CONFIG_BUG,
WARN_ONand family become simple passthroughs of their condition argument; however,WARN_ON_ONCEand 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 onCONFIG_BUG, and map them all to the passthroughWARN_ONwhen !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:
include/asm-generic/bug.h extern void warn_slowpath_null(const char *file, const int line); · line 146
Quoted 8 lines
include/asm-generic/bug.h +8
-0+#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.
include/asm-generic/bug.h extern void warn_slowpath_null(const char *file, const int line); · line 160
Quoted 13 lines
include/asm-generic/bug.h +13
-0+#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
Quoted 32 lines · Arnd Bergmann
Quoted 9 lines · Josh Triplett
When !CONFIG_BUG,
WARN_ONand family become simple passthroughs of their condition argument; however,WARN_ON_ONCEand 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 onCONFIG_BUG, and map them all to the passthroughWARN_ONwhen !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:
include/asm-generic/bug.h extern void warn_slowpath_null(const char *file, const int line); · line 146
Quoted 8 lines
include/asm-generic/bug.h +8
-0+#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.
Quoted 4 lines
This means we actually want
BUG()to end with__builtin_unreachable()as in theCONFIG_BUG=ycase, 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?
Quoted 19 lines
include/asm-generic/bug.h extern void warn_slowpath_null(const char *file, const int line); · line 160
Quoted 13 lines
include/asm-generic/bug.h +13
-0+#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):
Review extern void warn_slowpath_null(const char *file, const int line); · line 133
Quoted 8 lines
(quoted diff) +2
-0@@ -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
Arnd Bergmann
maintainerQuoted 6 lines · 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 thatBUG()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?
Quoted 4 lines
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:
include/asm-generic/bug.h extern void warn_slowpath_null(const char *file, const int line); · line 165
Quoted 4 lines
include/asm-generic/bug.h +4
-0+#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()?
Quoted 17 lines
Review extern void warn_slowpath_null(const char *file, const int line); · line 133
Quoted 13 lines
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):
(quoted diff) +2
-0@@ -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); \ }) #endifThis seems better done via
no_printk, but yes, that seems sensible. The arguments should be used too.
Quoted 2 lines
(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
Quoted 10 lines · Josh Triplett
Quoted 4 lines
This means we actually want
BUG()to end with__builtin_unreachable()as in theCONFIG_BUG=ycase, 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 thatBUG()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
Arnd Bergmann
maintainerQuoted 7 lines · On Monday 24 February 2014 12:09:11 David Howells wrote:
Quoted 5 lines · Josh Triplett
Quoted 4 lines
This means we actually want
BUG()to end with__builtin_unreachable()as in theCONFIG_BUG=ycase, 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.
Quoted 9 lines
Quoted 5 lines
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 thatBUG()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
Quoted 9 lines
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
Arnd Bergmann
maintainerQuoted 4 lines · 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
Quoted 32 lines · On Mon, 24 Feb 2014 09:02:35 +0100 Arnd Bergmann <[email protected]> wrote:
Quoted 9 lines · Josh Triplett
When !CONFIG_BUG,
WARN_ONand family become simple passthroughs of their condition argument; however,WARN_ON_ONCEand 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 onCONFIG_BUG, and map them all to the passthroughWARN_ONwhen !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:
include/asm-generic/bug.h extern void warn_slowpath_null(const char *file, const int line); · line 146
Quoted 8 lines
include/asm-generic/bug.h +8
-0+#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.
Quoted 4 lines
This means we actually want
BUG()to end with__builtin_unreachable()as in theCONFIG_BUG=ycase, 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?
Quoted 39 lines · Andrew Morton
Quoted 32 lines · On Mon, 24 Feb 2014 09:02:35 +0100 Arnd Bergmann <[email protected]> wrote:
Quoted 9 lines · Josh Triplett
When !CONFIG_BUG,
WARN_ONand family become simple passthroughs of their condition argument; however,WARN_ON_ONCEand 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 onCONFIG_BUG, and map them all to the passthroughWARN_ONwhen !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:
include/asm-generic/bug.h extern void warn_slowpath_null(const char *file, const int line); · line 146
Quoted 8 lines
include/asm-generic/bug.h +8
-0+#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.Quoted 4 lines
This means we actually want
BUG()to end with__builtin_unreachable()as in theCONFIG_BUG=ycase, 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.
Quoted 5 lines
CONFIG_BUG=ncauses 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.
Quoted 3 lines
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