[PATCH/RFC] Deprecate BUG/BUG_ON in favour of BUG_AND_HALT/BUG_AND_HALT_ON

[PATCH/RFC] Deprecate BUG/BUG_ON in favour of BUG_AND_HALT/BUG_AND_HALT_ON

[Paul Gortmaker]

A long standing problem for us has been the misuse of BUG/BUG_ON.
The typical misuse is someone only thinking of what represents
a bug in their local code, and especially for people relatively
new to Linux, starting out in device drivers, the appeal of using
BUG w/o knowing what it really does is too great.

So you end up with some trivial non system critical driver bringing
the whole system to a grinding halt just because it detected an
internal inconsistency.  That just makes users unhappy and looks bad.

It is hopeless to think we can reclaim BUG/BUG_ON for their original
intent, given there are currently ~20k instances.  To make progress
here, we create BUG_AND_HALT variants, which leave no doubt as to
what they do in name alone.

Then we can incrementally move the real BUG users (unrecoverable
filesystem corruption, page table mangling, etc) onto BUG_AND_HALT,
and finally at some time in the future we'll simply make the old
BUG/BUG_ON be aliases for WARN/WARN_ON, once we've moved over the
bulk of the instances really needing to halt the system.

Suggested-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---

[This might not be a unique idea; but I'm pretty sure I'd first
heard of it during a discussion with Ingo at RT summit last year.]

 include/asm-generic/bug.h | 16 ++++++++++++++++
 scripts/checkpatch.pl     |  7 +++++++
 2 files changed, 23 insertions(+)

diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index 630dd2372238..57b79a394ceb 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -43,6 +43,14 @@ struct bug_entry {
  * If you're tempted to BUG(), think again:  is completely giving up
  * really the *only* solution?  There are usually better options, where
  * users don't need to reboot ASAP and can mostly shut down cleanly.
+ *
+ * Sadly nobody listens to the above, and trying to reclaim BUG/BUG_ON
+ * for their original intent is about as hopeful as wishing "selfie"
+ * wasn't headed for the OED.  So the plan is to avoid BUG/BUG_ON
+ * entirely.  Either use WARN/WARN_ON or BUG_AND_HALT/BUG_AND_HALT_ON.
+ * Once the critical (e.g. fs etc) BUG/BUG_ON users are updated to use
+ * the clearly named HALT variants, we can point the old BUG/BUG_ON
+ * defines below to be clones of the less drastic WARN variants.
  */
 #ifndef HAVE_ARCH_BUG
 #define BUG() do { \
@@ -51,10 +59,18 @@ struct bug_entry {
 } while (0)
 #endif
 
+#ifndef HAVE_ARCH_BUG_AND_HALT
+#define BUG_AND_HALT BUG
+#endif
+
 #ifndef HAVE_ARCH_BUG_ON
 #define BUG_ON(condition) do { if (unlikely(condition)) BUG(); } while (0)
 #endif
 
+#ifndef HAVE_ARCH_BUG_AND_HALT_ON
+#define BUG_AND_HALT_ON BUG_ON
+#endif
+
 /*
  * WARN(), WARN_ON(), WARN_ON_ONCE, and so on can be used to report
  * significant issues that need prompt attention if they should ever
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 34eb2160489d..3cbf3591cf76 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2010,6 +2010,13 @@ sub process {
 			$rpt_cleaners = 1;
 		}
 
+# Dont use BUG/BUG_ON; use WARN/WARN_ON or BUG_AND_HALT/BUG_AND_HALT_ON
+		if ($rawline =~ /^\+.*BUG\(/ || $rawline =~ /^\+.*BUG_ON\(/) {
+			my $herevet = "$here\n" . cat_vet($rawline) . "\n";
+			WARN("BUG/BUG_ON",
+			     "Use of BUG/BUG_ON is deprecated. Use WARN/WARN_ON or BUG_AND_HALT/BUG_AND_HALT_ON\n" . $herevet);
+		}
+
 # Check for FSF mailing addresses.
 		if ($rawline =~ /\bwrite to the Free/i ||
 		    $rawline =~ /\b59\s+Temple\s+Pl/i ||
-- 
1.9.0

Re: [PATCH/RFC] Deprecate BUG/BUG_ON in favour of BUG_AND_HALT/BUG_AND_HALT_ON

[Richard Weinberger]

On Wed, Apr 30, 2014 at 5:03 PM, Paul Gortmaker
<paul.gortmaker@windriver.com> wrote:
> A long standing problem for us has been the misuse of BUG/BUG_ON.
> The typical misuse is someone only thinking of what represents
> a bug in their local code, and especially for people relatively
> new to Linux, starting out in device drivers, the appeal of using
> BUG w/o knowing what it really does is too great.
>
> So you end up with some trivial non system critical driver bringing
> the whole system to a grinding halt just because it detected an
> internal inconsistency.  That just makes users unhappy and looks bad.
>
> It is hopeless to think we can reclaim BUG/BUG_ON for their original
> intent, given there are currently ~20k instances.  To make progress
> here, we create BUG_AND_HALT variants, which leave no doubt as to
> what they do in name alone.
>
> Then we can incrementally move the real BUG users (unrecoverable
> filesystem corruption, page table mangling, etc) onto BUG_AND_HALT,
> and finally at some time in the future we'll simply make the old
> BUG/BUG_ON be aliases for WARN/WARN_ON, once we've moved over the
> bulk of the instances really needing to halt the system.
>
> Suggested-by: Ingo Molnar <mingo@kernel.org>
> Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
> ---
>
> [This might not be a unique idea; but I'm pretty sure I'd first
> heard of it during a discussion with Ingo at RT summit last year.]
>
>  include/asm-generic/bug.h | 16 ++++++++++++++++
>  scripts/checkpatch.pl     |  7 +++++++
>  2 files changed, 23 insertions(+)
>
> diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
> index 630dd2372238..57b79a394ceb 100644
> --- a/include/asm-generic/bug.h
> +++ b/include/asm-generic/bug.h
> @@ -43,6 +43,14 @@ struct bug_entry {
>   * If you're tempted to BUG(), think again:  is completely giving up
>   * really the *only* solution?  There are usually better options, where
>   * users don't need to reboot ASAP and can mostly shut down cleanly.
> + *
> + * Sadly nobody listens to the above, and trying to reclaim BUG/BUG_ON
> + * for their original intent is about as hopeful as wishing "selfie"
> + * wasn't headed for the OED.  So the plan is to avoid BUG/BUG_ON
> + * entirely.  Either use WARN/WARN_ON or BUG_AND_HALT/BUG_AND_HALT_ON.
> + * Once the critical (e.g. fs etc) BUG/BUG_ON users are updated to use
> + * the clearly named HALT variants, we can point the old BUG/BUG_ON
> + * defines below to be clones of the less drastic WARN variants.
>   */
>  #ifndef HAVE_ARCH_BUG
>  #define BUG() do { \
> @@ -51,10 +59,18 @@ struct bug_entry {
>  } while (0)
>  #endif
>
> +#ifndef HAVE_ARCH_BUG_AND_HALT
> +#define BUG_AND_HALT BUG
> +#endif
> +
>  #ifndef HAVE_ARCH_BUG_ON
>  #define BUG_ON(condition) do { if (unlikely(condition)) BUG(); } while (0)
>  #endif
>
> +#ifndef HAVE_ARCH_BUG_AND_HALT_ON
> +#define BUG_AND_HALT_ON BUG_ON
> +#endif
> +
>  /*
>   * WARN(), WARN_ON(), WARN_ON_ONCE, and so on can be used to report
>   * significant issues that need prompt attention if they should ever
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 34eb2160489d..3cbf3591cf76 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2010,6 +2010,13 @@ sub process {
>                         $rpt_cleaners = 1;
>                 }
>
> +# Dont use BUG/BUG_ON; use WARN/WARN_ON or BUG_AND_HALT/BUG_AND_HALT_ON
> +               if ($rawline =~ /^\+.*BUG\(/ || $rawline =~ /^\+.*BUG_ON\(/) {
> +                       my $herevet = "$here\n" . cat_vet($rawline) . "\n";
> +                       WARN("BUG/BUG_ON",
> +                            "Use of BUG/BUG_ON is deprecated. Use WARN/WARN_ON or BUG_AND_HALT/BUG_AND_HALT_ON\n" . $herevet);
> +               }
> +
>  # Check for FSF mailing addresses.
>                 if ($rawline =~ /\bwrite to the Free/i ||
>                     $rawline =~ /\b59\s+Temple\s+Pl/i ||
> --

I like the idea but not the name.
What about DIE() and DIE_ON()?

-- 
Thanks,
//richard

Re: [PATCH/RFC] Deprecate BUG/BUG_ON in favour of BUG_AND_HALT/BUG_AND_HALT_ON

[Ingo Molnar]

* Richard Weinberger <richard.weinberger@gmail.com> wrote:

> On Wed, Apr 30, 2014 at 5:03 PM, Paul Gortmaker
> <paul.gortmaker@windriver.com> wrote:
> > A long standing problem for us has been the misuse of BUG/BUG_ON.
> > The typical misuse is someone only thinking of what represents
> > a bug in their local code, and especially for people relatively
> > new to Linux, starting out in device drivers, the appeal of using
> > BUG w/o knowing what it really does is too great.
> >
> > So you end up with some trivial non system critical driver bringing
> > the whole system to a grinding halt just because it detected an
> > internal inconsistency.  That just makes users unhappy and looks bad.
> >
> > It is hopeless to think we can reclaim BUG/BUG_ON for their original
> > intent, given there are currently ~20k instances.  To make progress
> > here, we create BUG_AND_HALT variants, which leave no doubt as to
> > what they do in name alone.
> >
> > Then we can incrementally move the real BUG users (unrecoverable
> > filesystem corruption, page table mangling, etc) onto BUG_AND_HALT,
> > and finally at some time in the future we'll simply make the old
> > BUG/BUG_ON be aliases for WARN/WARN_ON, once we've moved over the
> > bulk of the instances really needing to halt the system.
> >
> > Suggested-by: Ingo Molnar <mingo@kernel.org>
> > Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
> > ---
> >
> > [This might not be a unique idea; but I'm pretty sure I'd first
> > heard of it during a discussion with Ingo at RT summit last year.]
> >
> >  include/asm-generic/bug.h | 16 ++++++++++++++++
> >  scripts/checkpatch.pl     |  7 +++++++
> >  2 files changed, 23 insertions(+)
> >
> > diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
> > index 630dd2372238..57b79a394ceb 100644
> > --- a/include/asm-generic/bug.h
> > +++ b/include/asm-generic/bug.h
> > @@ -43,6 +43,14 @@ struct bug_entry {
> >   * If you're tempted to BUG(), think again:  is completely giving up
> >   * really the *only* solution?  There are usually better options, where
> >   * users don't need to reboot ASAP and can mostly shut down cleanly.
> > + *
> > + * Sadly nobody listens to the above, and trying to reclaim BUG/BUG_ON
> > + * for their original intent is about as hopeful as wishing "selfie"
> > + * wasn't headed for the OED.  So the plan is to avoid BUG/BUG_ON
> > + * entirely.  Either use WARN/WARN_ON or BUG_AND_HALT/BUG_AND_HALT_ON.
> > + * Once the critical (e.g. fs etc) BUG/BUG_ON users are updated to use
> > + * the clearly named HALT variants, we can point the old BUG/BUG_ON
> > + * defines below to be clones of the less drastic WARN variants.
> >   */
> >  #ifndef HAVE_ARCH_BUG
> >  #define BUG() do { \
> > @@ -51,10 +59,18 @@ struct bug_entry {
> >  } while (0)
> >  #endif
> >
> > +#ifndef HAVE_ARCH_BUG_AND_HALT
> > +#define BUG_AND_HALT BUG
> > +#endif
> > +
> >  #ifndef HAVE_ARCH_BUG_ON
> >  #define BUG_ON(condition) do { if (unlikely(condition)) BUG(); } while (0)
> >  #endif
> >
> > +#ifndef HAVE_ARCH_BUG_AND_HALT_ON
> > +#define BUG_AND_HALT_ON BUG_ON
> > +#endif
> > +
> >  /*
> >   * WARN(), WARN_ON(), WARN_ON_ONCE, and so on can be used to report
> >   * significant issues that need prompt attention if they should ever
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index 34eb2160489d..3cbf3591cf76 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -2010,6 +2010,13 @@ sub process {
> >                         $rpt_cleaners = 1;
> >                 }
> >
> > +# Dont use BUG/BUG_ON; use WARN/WARN_ON or BUG_AND_HALT/BUG_AND_HALT_ON
> > +               if ($rawline =~ /^\+.*BUG\(/ || $rawline =~ /^\+.*BUG_ON\(/) {
> > +                       my $herevet = "$here\n" . cat_vet($rawline) . "\n";
> > +                       WARN("BUG/BUG_ON",
> > +                            "Use of BUG/BUG_ON is deprecated. Use WARN/WARN_ON or BUG_AND_HALT/BUG_AND_HALT_ON\n" . $herevet);
> > +               }
> > +
> >  # Check for FSF mailing addresses.
> >                 if ($rawline =~ /\bwrite to the Free/i ||
> >                     $rawline =~ /\b59\s+Temple\s+Pl/i ||
> > --
> 
> I like the idea but not the name.
> What about DIE() and DIE_ON()?

CRASH_ON() might be a suggestive name as well, as from the user's 
point of view we are crashing her system.

Thanks,

	Ingo

Re: [PATCH/RFC] Deprecate BUG/BUG_ON in favour of BUG_AND_HALT/BUG_AND_HALT_ON

[Richard Weinberger]

Am 06.05.2014 09:35, schrieb Ingo Molnar:
> 
> * Richard Weinberger <richard.weinberger@gmail.com> wrote:
> 
>> On Wed, Apr 30, 2014 at 5:03 PM, Paul Gortmaker
>> <paul.gortmaker@windriver.com> wrote:
>>> A long standing problem for us has been the misuse of BUG/BUG_ON.
>>> The typical misuse is someone only thinking of what represents
>>> a bug in their local code, and especially for people relatively
>>> new to Linux, starting out in device drivers, the appeal of using
>>> BUG w/o knowing what it really does is too great.
>>>
>>> So you end up with some trivial non system critical driver bringing
>>> the whole system to a grinding halt just because it detected an
>>> internal inconsistency.  That just makes users unhappy and looks bad.
>>>
>>> It is hopeless to think we can reclaim BUG/BUG_ON for their original
>>> intent, given there are currently ~20k instances.  To make progress
>>> here, we create BUG_AND_HALT variants, which leave no doubt as to
>>> what they do in name alone.
>>>
>>> Then we can incrementally move the real BUG users (unrecoverable
>>> filesystem corruption, page table mangling, etc) onto BUG_AND_HALT,
>>> and finally at some time in the future we'll simply make the old
>>> BUG/BUG_ON be aliases for WARN/WARN_ON, once we've moved over the
>>> bulk of the instances really needing to halt the system.
>>>
>>> Suggested-by: Ingo Molnar <mingo@kernel.org>
>>> Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
>>> ---
>>>
>>> [This might not be a unique idea; but I'm pretty sure I'd first
>>> heard of it during a discussion with Ingo at RT summit last year.]
>>>
>>>  include/asm-generic/bug.h | 16 ++++++++++++++++
>>>  scripts/checkpatch.pl     |  7 +++++++
>>>  2 files changed, 23 insertions(+)
>>>
>>> diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
>>> index 630dd2372238..57b79a394ceb 100644
>>> --- a/include/asm-generic/bug.h
>>> +++ b/include/asm-generic/bug.h
>>> @@ -43,6 +43,14 @@ struct bug_entry {
>>>   * If you're tempted to BUG(), think again:  is completely giving up
>>>   * really the *only* solution?  There are usually better options, where
>>>   * users don't need to reboot ASAP and can mostly shut down cleanly.
>>> + *
>>> + * Sadly nobody listens to the above, and trying to reclaim BUG/BUG_ON
>>> + * for their original intent is about as hopeful as wishing "selfie"
>>> + * wasn't headed for the OED.  So the plan is to avoid BUG/BUG_ON
>>> + * entirely.  Either use WARN/WARN_ON or BUG_AND_HALT/BUG_AND_HALT_ON.
>>> + * Once the critical (e.g. fs etc) BUG/BUG_ON users are updated to use
>>> + * the clearly named HALT variants, we can point the old BUG/BUG_ON
>>> + * defines below to be clones of the less drastic WARN variants.
>>>   */
>>>  #ifndef HAVE_ARCH_BUG
>>>  #define BUG() do { \
>>> @@ -51,10 +59,18 @@ struct bug_entry {
>>>  } while (0)
>>>  #endif
>>>
>>> +#ifndef HAVE_ARCH_BUG_AND_HALT
>>> +#define BUG_AND_HALT BUG
>>> +#endif
>>> +
>>>  #ifndef HAVE_ARCH_BUG_ON
>>>  #define BUG_ON(condition) do { if (unlikely(condition)) BUG(); } while (0)
>>>  #endif
>>>
>>> +#ifndef HAVE_ARCH_BUG_AND_HALT_ON
>>> +#define BUG_AND_HALT_ON BUG_ON
>>> +#endif
>>> +
>>>  /*
>>>   * WARN(), WARN_ON(), WARN_ON_ONCE, and so on can be used to report
>>>   * significant issues that need prompt attention if they should ever
>>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>>> index 34eb2160489d..3cbf3591cf76 100755
>>> --- a/scripts/checkpatch.pl
>>> +++ b/scripts/checkpatch.pl
>>> @@ -2010,6 +2010,13 @@ sub process {
>>>                         $rpt_cleaners = 1;
>>>                 }
>>>
>>> +# Dont use BUG/BUG_ON; use WARN/WARN_ON or BUG_AND_HALT/BUG_AND_HALT_ON
>>> +               if ($rawline =~ /^\+.*BUG\(/ || $rawline =~ /^\+.*BUG_ON\(/) {
>>> +                       my $herevet = "$here\n" . cat_vet($rawline) . "\n";
>>> +                       WARN("BUG/BUG_ON",
>>> +                            "Use of BUG/BUG_ON is deprecated. Use WARN/WARN_ON or BUG_AND_HALT/BUG_AND_HALT_ON\n" . $herevet);
>>> +               }
>>> +
>>>  # Check for FSF mailing addresses.
>>>                 if ($rawline =~ /\bwrite to the Free/i ||
>>>                     $rawline =~ /\b59\s+Temple\s+Pl/i ||
>>> --
>>
>> I like the idea but not the name.
>> What about DIE() and DIE_ON()?
> 
> CRASH_ON() might be a suggestive name as well, as from the user's 
> point of view we are crashing her system.

I fear such users will think "Why should I crash the kernel?". ;-)

Thanks,
//richard

Re: [PATCH/RFC] Deprecate BUG/BUG_ON in favour of BUG_AND_HALT/BUG_AND_HALT_ON

[Ingo Molnar]

* Richard Weinberger <richard@nod.at> wrote:

> >> I like the idea but not the name.
> >> What about DIE() and DIE_ON()?
> > 
> > CRASH_ON() might be a suggestive name as well, as from the user's 
> > point of view we are crashing her system.
> 
> I fear such users will think "Why should I crash the kernel?". ;-)

That's exactly the impression that the naming should create in kernel 
developers why try to add CRASH_ON() in the future: only do it as an 
absolute last resort.

WARN_ON() and other non-destructive ways to deal with error conditions 
are almost always preferred.

Thanks,

	Ingo

Re: [PATCH/RFC] Deprecate BUG/BUG_ON in favour of BUG_AND_HALT/BUG_AND_HALT_ON

[Richard Weinberger]

Am 06.05.2014 11:35, schrieb Ingo Molnar:
> 
> * Richard Weinberger <richard@nod.at> wrote:
> 
>>>> I like the idea but not the name.
>>>> What about DIE() and DIE_ON()?
>>>
>>> CRASH_ON() might be a suggestive name as well, as from the user's 
>>> point of view we are crashing her system.
>>
>> I fear such users will think "Why should I crash the kernel?". ;-)
> 
> That's exactly the impression that the naming should create in kernel 
> developers why try to add CRASH_ON() in the future: only do it as an 
> absolute last resort.
> 
> WARN_ON() and other non-destructive ways to deal with error conditions 
> are almost always preferred.

Yeah, I was actually referring to the joke where one asks to remove all BUG() and BUG_ON() to
make the kernel bug free.

Thanks,
//richard

Re: [PATCH/RFC] Deprecate BUG/BUG_ON in favour of BUG_AND_HALT/BUG_AND_HALT_ON

[Paul Gortmaker]

On 14-05-06 03:35 AM, Ingo Molnar wrote:
> 
> * Richard Weinberger <richard.weinberger@gmail.com> wrote:
> 
>> On Wed, Apr 30, 2014 at 5:03 PM, Paul Gortmaker
>> <paul.gortmaker@windriver.com> wrote:
>>> A long standing problem for us has been the misuse of BUG/BUG_ON.
>>> The typical misuse is someone only thinking of what represents
>>> a bug in their local code, and especially for people relatively
>>> new to Linux, starting out in device drivers, the appeal of using
>>> BUG w/o knowing what it really does is too great.
>>>

[...]

>>>
>>> +# Dont use BUG/BUG_ON; use WARN/WARN_ON or BUG_AND_HALT/BUG_AND_HALT_ON
>>> +               if ($rawline =~ /^\+.*BUG\(/ || $rawline =~ /^\+.*BUG_ON\(/) {
>>> +                       my $herevet = "$here\n" . cat_vet($rawline) . "\n";
>>> +                       WARN("BUG/BUG_ON",
>>> +                            "Use of BUG/BUG_ON is deprecated. Use WARN/WARN_ON or BUG_AND_HALT/BUG_AND_HALT_ON\n" . $herevet);
>>> +               }
>>> +
>>>  # Check for FSF mailing addresses.
>>>                 if ($rawline =~ /\bwrite to the Free/i ||
>>>                     $rawline =~ /\b59\s+Temple\s+Pl/i ||
>>> --
>>
>> I like the idea but not the name.
>> What about DIE() and DIE_ON()?
> 
> CRASH_ON() might be a suggestive name as well, as from the user's 
> point of view we are crashing her system.

I'd considered HALT_AND_CATCH_FIRE_ON(...) but it was too much typing
and the PC police already forced us to remove that from the lpr driver
decades ago.  So CRASH_ON works for me.  If nobody dislikes the idea,
and we are only bikeshedding over the names, then that is a good thing.

I'll let the idea stew for another day or two and send a renamed v2.

Thanks,
Paul.
--

> 
> Thanks,
> 
> 	Ingo
>