[v2,3/5] Introduce CONFIG_ENABLE_BUG_CHECKS to disable BUG{_ON} by default

Message ID 1512358624-6309-4-git-send-email-yamada.masahiro@socionext.com
State New
Headers show
Series
  • Remove assert()
Related show

Commit Message

Masahiro Yamada Dec. 4, 2017, 3:37 a.m.
BUG() and BUG_ON() are generally used to test a condition that should
never happen.  If it does, it is a bug.

Linux always enables them, but doing so in U-Boot causes image size
problems on some platforms.  Introduce CONFIG_ENABLE_BUG_CHECKS to
make them no-op by default.  Platforms without image size constraint
are free to enable this option to catch bugs easily.

Likewise, silence WARN_ON() unless this option is enabled.

Suggested-by: Tom Rini <trini@konsulko.com>
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

Changes in v2:
  - Newly added

 include/linux/bug.h | 9 ++++++++-
 lib/Kconfig         | 7 +++++++
 2 files changed, 15 insertions(+), 1 deletion(-)

Comments

Simon Glass Dec. 12, 2017, 4:38 a.m. | #1
On 3 December 2017 at 20:37, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> BUG() and BUG_ON() are generally used to test a condition that should
> never happen.  If it does, it is a bug.
>
> Linux always enables them, but doing so in U-Boot causes image size
> problems on some platforms.  Introduce CONFIG_ENABLE_BUG_CHECKS to
> make them no-op by default.  Platforms without image size constraint
> are free to enable this option to catch bugs easily.
>
> Likewise, silence WARN_ON() unless this option is enabled.
>
> Suggested-by: Tom Rini <trini@konsulko.com>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
>
> Changes in v2:
>   - Newly added
>
>  include/linux/bug.h | 9 ++++++++-
>  lib/Kconfig         | 7 +++++++
>  2 files changed, 15 insertions(+), 1 deletion(-)

Reviewed-by: Simon Glass <sjg@chromium.org>
Lothar Waßmann Dec. 12, 2017, 9:06 a.m. | #2
Hi,

On Mon,  4 Dec 2017 12:37:02 +0900 Masahiro Yamada wrote:
> BUG() and BUG_ON() are generally used to test a condition that should
> never happen.  If it does, it is a bug.
> 
> Linux always enables them, but doing so in U-Boot causes image size
> problems on some platforms.  Introduce CONFIG_ENABLE_BUG_CHECKS to
> make them no-op by default.  Platforms without image size constraint
> are free to enable this option to catch bugs easily.
> 
> Likewise, silence WARN_ON() unless this option is enabled.
> 
> Suggested-by: Tom Rini <trini@konsulko.com>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
> 
> Changes in v2:
>   - Newly added
> 
>  include/linux/bug.h | 9 ++++++++-
>  lib/Kconfig         | 7 +++++++
>  2 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/bug.h b/include/linux/bug.h
> index f07bb71..ac1c7de 100644
> --- a/include/linux/bug.h
> +++ b/include/linux/bug.h
> @@ -6,17 +6,24 @@
>  #include <linux/compiler.h>
>  #include <linux/printk.h>
>  
> +#ifdef CONFIG_ENABLE_BUG_CHECKS
>  #define BUG() do { \
>  	printk("BUG at %s:%d/%s()!\n", __FILE__, __LINE__, __func__); \
>  	panic("BUG!"); \
>  } while (0)
> +#define __WARN() 	\
> +	printk("WARNING at %s:%d/%s()!\n", __FILE__, __LINE__, __func__)
> +#else
> +#define BUG()
> +#define __WARN()
> +#endif
>  
>  #define BUG_ON(condition) do { if (unlikely(condition)) BUG(); } while (0)
>  
>  #define WARN_ON(condition) ({						\
>  	int __ret_warn_on = !!(condition);				\
>  	if (unlikely(__ret_warn_on))					\
> -		printk("WARNING at %s:%d/%s()!\n", __FILE__, __LINE__, __func__); \
> +		__WARN();						\
>  	unlikely(__ret_warn_on);					\
>  })
>  
> diff --git a/lib/Kconfig b/lib/Kconfig
> index 00ac650..36b1b3b 100644
> --- a/lib/Kconfig
> +++ b/lib/Kconfig
> @@ -45,6 +45,13 @@ config USE_TINY_PRINTF
>  
>  	  The supported format specifiers are %c, %s, %u/%d and %x.
>  
> +config ENABLE_BUG_CHECKS
> +	bool "Enable BUG/BUG_ON() checks and WARN_ON() logs"
> +	help
This should be 'default y' IMO to keep the current behaviour for all
existing platforms.


Lothar Waßmann
Tom Rini Dec. 12, 2017, 1:47 p.m. | #3
On Tue, Dec 12, 2017 at 10:06:19AM +0100, Lothar Waßmann wrote:
> Hi,

> 

> On Mon,  4 Dec 2017 12:37:02 +0900 Masahiro Yamada wrote:

> > BUG() and BUG_ON() are generally used to test a condition that should

> > never happen.  If it does, it is a bug.

> > 

> > Linux always enables them, but doing so in U-Boot causes image size

> > problems on some platforms.  Introduce CONFIG_ENABLE_BUG_CHECKS to

> > make them no-op by default.  Platforms without image size constraint

> > are free to enable this option to catch bugs easily.

> > 

> > Likewise, silence WARN_ON() unless this option is enabled.

> > 

> > Suggested-by: Tom Rini <trini@konsulko.com>

> > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>

> > ---

> > 

> > Changes in v2:

> >   - Newly added

> > 

> >  include/linux/bug.h | 9 ++++++++-

> >  lib/Kconfig         | 7 +++++++

> >  2 files changed, 15 insertions(+), 1 deletion(-)

> > 

> > diff --git a/include/linux/bug.h b/include/linux/bug.h

> > index f07bb71..ac1c7de 100644

> > --- a/include/linux/bug.h

> > +++ b/include/linux/bug.h

> > @@ -6,17 +6,24 @@

> >  #include <linux/compiler.h>

> >  #include <linux/printk.h>

> >  

> > +#ifdef CONFIG_ENABLE_BUG_CHECKS

> >  #define BUG() do { \

> >  	printk("BUG at %s:%d/%s()!\n", __FILE__, __LINE__, __func__); \

> >  	panic("BUG!"); \

> >  } while (0)

> > +#define __WARN() 	\

> > +	printk("WARNING at %s:%d/%s()!\n", __FILE__, __LINE__, __func__)

> > +#else

> > +#define BUG()

> > +#define __WARN()

> > +#endif

> >  

> >  #define BUG_ON(condition) do { if (unlikely(condition)) BUG(); } while (0)

> >  

> >  #define WARN_ON(condition) ({						\

> >  	int __ret_warn_on = !!(condition);				\

> >  	if (unlikely(__ret_warn_on))					\

> > -		printk("WARNING at %s:%d/%s()!\n", __FILE__, __LINE__, __func__); \

> > +		__WARN();						\

> >  	unlikely(__ret_warn_on);					\

> >  })

> >  

> > diff --git a/lib/Kconfig b/lib/Kconfig

> > index 00ac650..36b1b3b 100644

> > --- a/lib/Kconfig

> > +++ b/lib/Kconfig

> > @@ -45,6 +45,13 @@ config USE_TINY_PRINTF

> >  

> >  	  The supported format specifiers are %c, %s, %u/%d and %x.

> >  

> > +config ENABLE_BUG_CHECKS

> > +	bool "Enable BUG/BUG_ON() checks and WARN_ON() logs"

> > +	help

> This should be 'default y' IMO to keep the current behaviour for all

> existing platforms.


I brought this up to Masahiro privately as I had been testing the
series, and with ENABLE_BUG_CHECKS=n we get a warning over in the USB
code, which in turn got me thinking harder.  We do want to be able to
disable this, for space reasons, when needed, but it should default to
enabled (even if this increases the overall size).

-- 
Tom
Masahiro Yamada Dec. 14, 2017, 9:35 p.m. | #4
2017-12-12 22:47 GMT+09:00 Tom Rini <trini@konsulko.com>:
> On Tue, Dec 12, 2017 at 10:06:19AM +0100, Lothar Waßmann wrote:
>> Hi,
>>
>> On Mon,  4 Dec 2017 12:37:02 +0900 Masahiro Yamada wrote:
>> > BUG() and BUG_ON() are generally used to test a condition that should
>> > never happen.  If it does, it is a bug.
>> >
>> > Linux always enables them, but doing so in U-Boot causes image size
>> > problems on some platforms.  Introduce CONFIG_ENABLE_BUG_CHECKS to
>> > make them no-op by default.  Platforms without image size constraint
>> > are free to enable this option to catch bugs easily.
>> >
>> > Likewise, silence WARN_ON() unless this option is enabled.
>> >
>> > Suggested-by: Tom Rini <trini@konsulko.com>
>> > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>> > ---
>> >
>> > Changes in v2:
>> >   - Newly added
>> >
>> >  include/linux/bug.h | 9 ++++++++-
>> >  lib/Kconfig         | 7 +++++++
>> >  2 files changed, 15 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/include/linux/bug.h b/include/linux/bug.h
>> > index f07bb71..ac1c7de 100644
>> > --- a/include/linux/bug.h
>> > +++ b/include/linux/bug.h
>> > @@ -6,17 +6,24 @@
>> >  #include <linux/compiler.h>
>> >  #include <linux/printk.h>
>> >
>> > +#ifdef CONFIG_ENABLE_BUG_CHECKS
>> >  #define BUG() do { \
>> >     printk("BUG at %s:%d/%s()!\n", __FILE__, __LINE__, __func__); \
>> >     panic("BUG!"); \
>> >  } while (0)
>> > +#define __WARN()   \
>> > +   printk("WARNING at %s:%d/%s()!\n", __FILE__, __LINE__, __func__)
>> > +#else
>> > +#define BUG()
>> > +#define __WARN()
>> > +#endif
>> >
>> >  #define BUG_ON(condition) do { if (unlikely(condition)) BUG(); } while (0)
>> >
>> >  #define WARN_ON(condition) ({                                              \
>> >     int __ret_warn_on = !!(condition);                              \
>> >     if (unlikely(__ret_warn_on))                                    \
>> > -           printk("WARNING at %s:%d/%s()!\n", __FILE__, __LINE__, __func__); \
>> > +           __WARN();                                               \
>> >     unlikely(__ret_warn_on);                                        \
>> >  })
>> >
>> > diff --git a/lib/Kconfig b/lib/Kconfig
>> > index 00ac650..36b1b3b 100644
>> > --- a/lib/Kconfig
>> > +++ b/lib/Kconfig
>> > @@ -45,6 +45,13 @@ config USE_TINY_PRINTF
>> >
>> >       The supported format specifiers are %c, %s, %u/%d and %x.
>> >
>> > +config ENABLE_BUG_CHECKS
>> > +   bool "Enable BUG/BUG_ON() checks and WARN_ON() logs"
>> > +   help
>> This should be 'default y' IMO to keep the current behaviour for all
>> existing platforms.
>
> I brought this up to Masahiro privately as I had been testing the
> series, and with ENABLE_BUG_CHECKS=n we get a warning over in the USB
> code, which in turn got me thinking harder.  We do want to be able to
> disable this, for space reasons, when needed, but it should default to
> enabled (even if this increases the overall size).
>
> --
> Tom


I will flip the default in v3.
Tom privately mentioned this series would be postponed by the next MW.
So, I will do v3 when he is ready to receive it.

Patch

diff --git a/include/linux/bug.h b/include/linux/bug.h
index f07bb71..ac1c7de 100644
--- a/include/linux/bug.h
+++ b/include/linux/bug.h
@@ -6,17 +6,24 @@ 
 #include <linux/compiler.h>
 #include <linux/printk.h>
 
+#ifdef CONFIG_ENABLE_BUG_CHECKS
 #define BUG() do { \
 	printk("BUG at %s:%d/%s()!\n", __FILE__, __LINE__, __func__); \
 	panic("BUG!"); \
 } while (0)
+#define __WARN() 	\
+	printk("WARNING at %s:%d/%s()!\n", __FILE__, __LINE__, __func__)
+#else
+#define BUG()
+#define __WARN()
+#endif
 
 #define BUG_ON(condition) do { if (unlikely(condition)) BUG(); } while (0)
 
 #define WARN_ON(condition) ({						\
 	int __ret_warn_on = !!(condition);				\
 	if (unlikely(__ret_warn_on))					\
-		printk("WARNING at %s:%d/%s()!\n", __FILE__, __LINE__, __func__); \
+		__WARN();						\
 	unlikely(__ret_warn_on);					\
 })
 
diff --git a/lib/Kconfig b/lib/Kconfig
index 00ac650..36b1b3b 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -45,6 +45,13 @@  config USE_TINY_PRINTF
 
 	  The supported format specifiers are %c, %s, %u/%d and %x.
 
+config ENABLE_BUG_CHECKS
+	bool "Enable BUG/BUG_ON() checks and WARN_ON() logs"
+	help
+	  BUG() and BUG_ON() are no-op by default.  This option enables
+	  them to print noisy messages, then reboot or halt the system.
+	  It also enables WARN_ON() messages.
+
 config PANIC_HANG
 	bool "Do not reset the system on fatal error"
 	help