2010-11-17 03:45:47

by Arnaud Lacombe

[permalink] [raw]
Subject: Behavior of BUG() [Was: Re: [PATCH 2/5] of/fdt: add kernel command line option for dtb_compat string]

Hi,

[CC: list reduced as starting a new thread, most on the context
removed as this concern a different issue.]

On Tue, Nov 16, 2010 at 7:16 PM, Grant Likely <[email protected]> wrote:
> On Tue, Nov 16, 2010 at 3:41 PM, ?<[email protected]> wrote:
>> From: Dirk Brandewie <[email protected]>
>> [...]
> The kernel needs to complain *loudly* if this occurs because it
> represents a bug. ?I'm tempted to say use BUG, but that would halt the
> kernel and prevent any possibility of kernel log output.
> [...]
does it ? if CONFIG_BUG is not enabled and the arch has no define for
it, the default does _nothing_:

from `include/asm-generic/bug.h':

#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
[...]

gcc is triggering about ~30 warnings (like [0]) on code path using
BUG(). Most of these path assume BUG() will never return, which is not
true.

The commit message introducing this behavior state:

commit c8538a7aa5527d02c7191ac5da124efadf6a2827
Author: Matt Mackall <[email protected]>
Date: Sun May 1 08:59:01 2005 -0700

[PATCH] remove all kernel BUGs

This patch eliminates all kernel BUGs, trims about 35k off the typical
kernel, and makes the system slightly faster.

Signed-off-by: Matt Mackall <[email protected]>
Signed-off-by: Andrew Morton <[email protected]>
Signed-off-by: Linus Torvalds <[email protected]>

[...]

+config BUG
+ bool "BUG() support" if EMBEDDED
+ default y
+ help
+ Disabling this option eliminates support for BUG and WARN, reducing
+ the size of your kernel image and potentially quietly ignoring
+ numerous fatal conditions. You should only consider disabling this
+ option for embedded systems with no facilities for reporting errors.

So how should BUG() been used ?
1) assuming it will never _ever_ return ?
2) assuming it may return ?

if (1), its definition has to be changed, if (2) a lot of path have to be fixed.

Thanks in advance,
- Arnaud

[0]: fs/dcache.c:1899:1: warning: control reaches end of non-void
function, line match an old revision, but the BUG() is still there.


2010-11-17 04:07:27

by Matt Mackall

[permalink] [raw]
Subject: Re: Behavior of BUG() [Was: Re: [PATCH 2/5] of/fdt: add kernel command line option for dtb_compat string]

On Tue, 2010-11-16 at 22:45 -0500, Arnaud Lacombe wrote:
> Hi,
>
> [CC: list reduced as starting a new thread, most on the context
> removed as this concern a different issue.]
>
> On Tue, Nov 16, 2010 at 7:16 PM, Grant Likely <[email protected]> wrote:
> > On Tue, Nov 16, 2010 at 3:41 PM, <[email protected]> wrote:
> >> From: Dirk Brandewie <[email protected]>
> >> [...]
> > The kernel needs to complain *loudly* if this occurs because it
> > represents a bug. I'm tempted to say use BUG, but that would halt the
> > kernel and prevent any possibility of kernel log output.
> > [...]
> does it ? if CONFIG_BUG is not enabled and the arch has no define for
> it, the default does _nothing_:

That's because CONFIG_BUG=N is intended for machines where logging is
irrelevant/impossible. You are encouraged to ignore all options under
CONFIG_EMBEDDED when trying to have a sensible discussion.

--
Mathematics is the supreme nostalgia of our time.

2010-11-17 04:13:41

by Grant Likely

[permalink] [raw]
Subject: Re: Behavior of BUG() [Was: Re: [PATCH 2/5] of/fdt: add kernel command line option for dtb_compat string]

On Tue, Nov 16, 2010 at 8:45 PM, Arnaud Lacombe <[email protected]> wrote:
> Hi,
>
> [CC: list reduced as starting a new thread, most on the context
> removed as this concern a different issue.]
>
> On Tue, Nov 16, 2010 at 7:16 PM, Grant Likely <[email protected]> wrote:
>> On Tue, Nov 16, 2010 at 3:41 PM, ?<[email protected]> wrote:
>>> From: Dirk Brandewie <[email protected]>
>>> [...]
>> The kernel needs to complain *loudly* if this occurs because it
>> represents a bug. ?I'm tempted to say use BUG, but that would halt the
>> kernel and prevent any possibility of kernel log output.
>> [...]
> does it ? if CONFIG_BUG is not enabled and the arch has no define for
> it, the default does _nothing_:
>
> from `include/asm-generic/bug.h':
>
> #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
> [...]
>
> gcc is triggering about ~30 warnings (like [0]) on code path using
> BUG(). Most of these path assume BUG() will never return, which is not
> true.

As far as I know, BUG() is not supposed to return. Period. The patch
below is part of the linux-tiny work, and should only ever be used on
embedded systems where small size is more important than debugability.

g.

>
> The commit message introducing this behavior state:
>
> commit c8538a7aa5527d02c7191ac5da124efadf6a2827
> Author: Matt Mackall <[email protected]>
> Date: ? Sun May 1 08:59:01 2005 -0700
>
> ? ?[PATCH] remove all kernel BUGs
>
> ? ?This patch eliminates all kernel BUGs, trims about 35k off the typical
> ? ?kernel, and makes the system slightly faster.
>
> ? ?Signed-off-by: Matt Mackall <[email protected]>
> ? ?Signed-off-by: Andrew Morton <[email protected]>
> ? ?Signed-off-by: Linus Torvalds <[email protected]>
>
> [...]
>
> +config BUG
> + ? ? ? bool "BUG() support" if EMBEDDED
> + ? ? ? default y
> + ? ? ? help
> + ? ? ? ? ?Disabling this option eliminates support for BUG and WARN, reducing
> + ? ? ? ? ?the size of your kernel image and potentially quietly ignoring
> + ? ? ? ? ?numerous fatal conditions. You should only consider disabling this
> + ? ? ? ? ?option for embedded systems with no facilities for reporting errors.
>
> So how should BUG() been used ?
> ?1) assuming it will never _ever_ return ?
> ?2) assuming it may return ?
>
> if (1), its definition has to be changed, if (2) a lot of path have to be fixed.

The answer is 1).

g.

2010-11-17 04:21:01

by Arnaud Lacombe

[permalink] [raw]
Subject: Re: Behavior of BUG() [Was: Re: [PATCH 2/5] of/fdt: add kernel command line option for dtb_compat string]

Hi,

On Tue, Nov 16, 2010 at 11:07 PM, Matt Mackall <[email protected]> wrote:
> On Tue, 2010-11-16 at 22:45 -0500, Arnaud Lacombe wrote:
>> Hi,
>>
>> [CC: list reduced as starting a new thread, most on the context
>> removed as this concern a different issue.]
>>
>> On Tue, Nov 16, 2010 at 7:16 PM, Grant Likely <[email protected]> wrote:
>> > On Tue, Nov 16, 2010 at 3:41 PM, ?<[email protected]> wrote:
>> >> From: Dirk Brandewie <[email protected]>
>> >> [...]
>> > The kernel needs to complain *loudly* if this occurs because it
>> > represents a bug. ?I'm tempted to say use BUG, but that would halt the
>> > kernel and prevent any possibility of kernel log output.
>> > [...]
>> does it ? if CONFIG_BUG is not enabled and the arch has no define for
>> it, the default does _nothing_:
>
> That's because CONFIG_BUG=N is intended for machines where logging is
> irrelevant/impossible.
>
Yes, but it is still a no-op, where the original programmer made it
clear he did not want the kernel to go further.

> You are encouraged to ignore all options under
> CONFIG_EMBEDDED when trying to have a sensible discussion.
>
Unless it creates a situation where it does the opposite of what the
intended behavior, I would guess.

- Arnaud

2010-11-17 04:30:01

by Arnaud Lacombe

[permalink] [raw]
Subject: Re: Behavior of BUG() [Was: Re: [PATCH 2/5] of/fdt: add kernel command line option for dtb_compat string]

Hi,

On Tue, Nov 16, 2010 at 11:13 PM, Grant Likely
<[email protected]> wrote:
> On Tue, Nov 16, 2010 at 8:45 PM, Arnaud Lacombe <[email protected]> wrote:
>> Hi,
>>
>> [CC: list reduced as starting a new thread, most on the context
>> removed as this concern a different issue.]
>>
>> On Tue, Nov 16, 2010 at 7:16 PM, Grant Likely <[email protected]> wrote:
>>> On Tue, Nov 16, 2010 at 3:41 PM, ?<[email protected]> wrote:
>>>> From: Dirk Brandewie <[email protected]>
>>>> [...]
>>> The kernel needs to complain *loudly* if this occurs because it
>>> represents a bug. ?I'm tempted to say use BUG, but that would halt the
>>> kernel and prevent any possibility of kernel log output.
>>> [...]
>> does it ? if CONFIG_BUG is not enabled and the arch has no define for
>> it, the default does _nothing_:
>>
>> from `include/asm-generic/bug.h':
>>
>> #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
>> [...]
>>
>> gcc is triggering about ~30 warnings (like [0]) on code path using
>> BUG(). Most of these path assume BUG() will never return, which is not
>> true.
>
> As far as I know, BUG() is not supposed to return. ?Period.
>
but the code I pointed out _do_ return.

> The patch
> below is part of the linux-tiny work, and should only ever be used on
> embedded systems where small size is more important than debugability.
>
AFAIK, this is not precised anywhere, but I may not have search enough.

Matt, any reason the generic code does not just spin (or OOPS) and
marked __noreturn in any case ?

- Arnaud

2010-11-17 04:44:12

by Matt Mackall

[permalink] [raw]
Subject: Re: Behavior of BUG() [Was: Re: [PATCH 2/5] of/fdt: add kernel command line option for dtb_compat string]

On Tue, 2010-11-16 at 23:29 -0500, Arnaud Lacombe wrote:
> Hi,
>
> On Tue, Nov 16, 2010 at 11:13 PM, Grant Likely
> <[email protected]> wrote:
> > On Tue, Nov 16, 2010 at 8:45 PM, Arnaud Lacombe <[email protected]> wrote:
> >> Hi,
> >>
> >> [CC: list reduced as starting a new thread, most on the context
> >> removed as this concern a different issue.]
> >>
> >> On Tue, Nov 16, 2010 at 7:16 PM, Grant Likely <[email protected]> wrote:
> >>> On Tue, Nov 16, 2010 at 3:41 PM, <[email protected]> wrote:
> >>>> From: Dirk Brandewie <[email protected]>
> >>>> [...]
> >>> The kernel needs to complain *loudly* if this occurs because it
> >>> represents a bug. I'm tempted to say use BUG, but that would halt the
> >>> kernel and prevent any possibility of kernel log output.
> >>> [...]
> >> does it ? if CONFIG_BUG is not enabled and the arch has no define for
> >> it, the default does _nothing_:
> >>
> >> from `include/asm-generic/bug.h':
> >>
> >> #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
> >> [...]
> >>
> >> gcc is triggering about ~30 warnings (like [0]) on code path using
> >> BUG(). Most of these path assume BUG() will never return, which is not
> >> true.
> >
> > As far as I know, BUG() is not supposed to return. Period.
> >
> but the code I pointed out _do_ return.
>
> > The patch
> > below is part of the linux-tiny work, and should only ever be used on
> > embedded systems where small size is more important than debugability.
> >
> AFAIK, this is not precised anywhere, but I may not have search enough.

Read the help for CONFIG_BUG and CONFIG_EMBEDDED.

> Matt, any reason the generic code does not just spin (or OOPS) and
> marked __noreturn in any case ?

Yes. Spinning and oopsing take space. Function calls take space. The
whole damn point of this option is to say is _we don't care about BUGs,
we care about space_. In other words, this code is a hack and is marked
as such. Arguing about "correctness" here is a waste of time, it's
intentionally not correct.

I seem to recall attempting to fool GCC with a __noreturn and failing.

--
Mathematics is the supreme nostalgia of our time.