Re: What policy for BUG_ON()?

Re: What policy for BUG_ON()?

[Albert Cahalan]

On Mon, 30 Aug 2004, Adrian Bunk wrote:

> Let me try to summarize the different options regarding BUG_ON, 
> concerning whether the argument to BUG_ON might contain side effects, 
> and whether it should be allowed in some "do this only if you _really_ 
> know what you are doing" situations to let BUG_ON do nothing.
> 
> Options:
> 1. BUG_ON must not be defined to do nothing
> 1a. side effects are allowed in the argument of BUG_ON
> 1b. side effects are not allowed in the argument of BUG_ON
> 2. BUG_ON is allowed to be defined to do nothing
> 2a. side effects are allowed in the argument of BUG_ON
> 2b. side effects are not allowed in the argument of BUG_ON

It comes down to the relative importance of:

i.  BUG_ON(expensive_and_unneeded_debug_test())
ii. BUG_ON(something_that_must_execute())

I think case i should get priority, since then the
removal of side effects is a nice way to eliminate
the expensive code for non-debug builds.

For case ii, it's easy enough to split out the
need-to-execute code and assign results to a
variable that can be checked later. Since it is
something that must execute, you probably need
the return value anyway.

The normal expectation for non-debug builds
would be this:

#define BUG_ON(x)

Re: What policy for BUG_ON()?

[Linus Torvalds]

On Tue, 31 Aug 2004, Albert Cahalan wrote:
> 
> The normal expectation for non-debug builds
> would be this:
> 
> #define BUG_ON(x)

No, this is bad, for one big reason: it generates compiler warnings if 'x'
happens to be the only thing that uses some value. You get things like
"unused variable 'mode'" etc in perfectly good code.

For example, the code might look something like

	int count = 0;

	for_each_online_cpu(cpu) {
		... do something ..
		.. update count ..
	}

	BUG_ON(!count);

and if you now compile on UP and with debugging enabled, the compiler
might complain that you're computing "count" but never _using_ the value.

This is generally why you should have macros like this not become empty, 
but become something that the compiler can compile away. Which is why I'd 
much rather see

	#define BUG_ON(x) (void)(x)

regardless of any side-effect issues - it's a way to let the compiler 
optimize the thing away, but still show that something was used at least 
"conceptually"..

		Linus

Re: What policy for BUG_ON()?

[Albert Cahalan]

On Tue, 2004-08-31 at 12:52, Linus Torvalds wrote:
> On Tue, 31 Aug 2004, Albert Cahalan wrote:
> > 
> > The normal expectation for non-debug builds
> > would be this:
> > 
> > #define BUG_ON(x)
> 
> No, this is bad, for one big reason: it generates compiler
> warnings if 'x' happens to be the only thing that uses some value.
...
> This is generally why you should have macros like this not
> become empty, but become something that the compiler can
> compile away. Which is why I'd much rather see
> 
> 	#define BUG_ON(x) (void)(x)
> 
> regardless of any side-effect issues - it's a way to let the
> compiler optimize the thing away, but still show that
> something was used at least "conceptually"..

Expensive function calls won't get optimized away unless you
mark them __attribute__((__const__)) or __attribute__((__pure__)).
(perhaps that should be encouraged)

Then of course the compiler must assume that the function
really needed the arguments it was passed, and that it
might have modified memory, and so on.

Eh, how about a BUG_ON_WITH_SIDE_EFFECT() macro?

Re: What policy for BUG_ON()?

[Kyle Moffett]

On Aug 31, 2004, at 13:39, Albert Cahalan wrote:
> Expensive function calls won't get optimized away unless you
> mark them __attribute__((__const__)) or __attribute__((__pure__)).
> (perhaps that should be encouraged)
>
> Then of course the compiler must assume that the function
> really needed the arguments it was passed, and that it
> might have modified memory, and so on.
>
> Eh, how about a BUG_ON_WITH_SIDE_EFFECT() macro?

Due to the potentially large number of existing BUG_ON() usages with 
side
effects, it might be better to do this:

#if DEBUG
# define	BUG_ON(cond)		do { if (cond) BUG(); } while(0)
# define	BUG_CHECK(cond)		do { if (cond) BUG(); } while(0)
#else
# define	BUG_ON(cond)		do { if (cond); } while(0)
# define	BUG_CHECK(cond)		do { } while(0)
#endif

Then in most cases new statements would use BUG_CHECK, especially if
they contain expensive unnecessary function calls or critical sections.

This would break the least amount of existing code, and provide both
methods to kernel developers.

Cheers,
Kyle Moffett

-----BEGIN GEEK CODE BLOCK-----
Version: 3.12
GCM/CS/IT/U d- s++: a17 C++++>$ UB/L/X/*++++(+)>$ P+++(++++)>$
L++++(+++) E W++(+) N+++(++) o? K? w--- O? M++ V? PS+() PE+(-) Y+
PGP+++ t+(+++) 5 X R? tv-(--) b++++(++) DI+ D+ G e->++++$ h!*()>++$ r  
!y?(-)
------END GEEK CODE BLOCK------

Re: What policy for BUG_ON()?

[Michael Buesch]

[-- Attachment #1: Type: text/plain, Size: 1567 bytes --]

Quoting Kyle Moffett <mrmacman_g4@mac.com>:
> Then in most cases new statements would use BUG_CHECK, especially if
> they contain expensive unnecessary function calls or critical sections.
> 
> This would break the least amount of existing code, and provide both
> methods to kernel developers.

So, back to the real world. ;)
- Where do we insert BUG_ON()s?
Only in places, where we are going to crash or corrupt data soon.

- Do we insert "expensive unnecessary function calls" in a BUG_ON()?
No we don't. Could you give a good example, which
needs an expensive call inside a BUG_ON()?

In a shiny good world we expect BUG()s to never trigger. So I think
it's a bit crazy to check for things that theoretically can't happen
and waste tons of CPU cycles for this, with expensive calls.
If we really want to check this while debugging, I think we
should explicitely honor the DEBUG define in the code and have
our own debug printk() that shows the mess.

I think here's a general confusion about what BUG_ON() is intended
for. I think (I'm not the author of it, so I can't say 100%. :) )
it is _not_ for debugging while development. It is for checking unpossible
things, that blow up the whole machine if they trigger nevertheless.
So I think it's wrong to let BUG_ON() depend on a DEBUG define, because
DEBUG is, by definition, not enabled in the kernels people use.
But I think we _want_ that people evaluate the BUG_ON()s.

I'm not talking of embedded systems, etc... .

-- 
Regards Michael Buesch  [ http://www.tuxsoft.de.vu ]


[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: What policy for BUG_ON()?

[Kyle Moffett]

On Aug 31, 2004, at 18:16, Michael Buesch wrote:
> So, back to the real world. ;)
> - Where do we insert BUG_ON()s?
> Only in places, where we are going to crash or corrupt data soon.
>
> - Do we insert "expensive unnecessary function calls" in a BUG_ON()?
> No we don't. Could you give a good example, which
> needs an expensive call inside a BUG_ON()?

BUG_ON(len != strlen(string));

I don't want the strlen() executed on an embedded machine every time
I hit that piece of code, heck, probably not even my server if string 
is big
or if this code is executed many times.

> In a shiny good world we expect BUG()s to never trigger. So I think
> it's a bit crazy to check for things that theoretically can't happen
> and waste tons of CPU cycles for this, with expensive calls.
> If we really want to check this while debugging, I think we
> should explicitely honor the DEBUG define in the code and have
> our own debug printk() that shows the mess.
>
> I think here's a general confusion about what BUG_ON() is intended
> for. I think (I'm not the author of it, so I can't say 100%. :) )
> it is _not_ for debugging while development. It is for checking 
> unpossible
> things, that blow up the whole machine if they trigger nevertheless.
> So I think it's wrong to let BUG_ON() depend on a DEBUG define, because
> DEBUG is, by definition, not enabled in the kernels people use.
> But I think we _want_ that people evaluate the BUG_ON()s.

Ok, so then we need an additional config directive:

CONFIG_EMBEDDED_NODEBUG

Then:

#ifdef CONFIG_EMBEDDED_NODEBUG
# define BUG_ON(cond) do { if (cond); } while(0)
# define BUG_CHECK(cond) do { } while(0)
#else
# define BUG_ON(cond) do { if (cond) BUG(); } while(0)
# define BUG_CHECK(cond) do { if (cond) BUG(); } while(0)
#endif

#ifdef CONFIG_DEBUG
# define DEBUG_ON(cond) do { } while(0)
#else
# define DEBUG_ON(cond) do { if (cond) BUG(); } while(0)
#endif

It's the _exact_same_ problem, with different definitions for which
mode is "debugging mode" in this particular case.  I agree with the
above email, but I think that for the embedded people, we should
define an extra macro that removes all excess tests, whether they
are expensive or inexpensive.  Then the BUG_ON() macro would
leave any checks in place in such a specialized compile, because
they may have required side effects.  A similar DEBUG_ON()
macro would be disabled for general users, but could be enabled
with DEBUG to provide the expensive checks, when a user is
experiencing problems.

Cheers,
Kyle Moffett

-----BEGIN GEEK CODE BLOCK-----
Version: 3.12
GCM/CS/IT/U d- s++: a17 C++++>$ UB/L/X/*++++(+)>$ P+++(++++)>$
L++++(+++) E W++(+) N+++(++) o? K? w--- O? M++ V? PS+() PE+(-) Y+
PGP+++ t+(+++) 5 X R? tv-(--) b++++(++) DI+ D+ G e->++++$ h!*()>++$ r  
!y?(-)
------END GEEK CODE BLOCK------

What policy for BUG_ON()?

[Adrian Bunk]

Let me try to summarize the different options regarding BUG_ON, 
concerning whether the argument to BUG_ON might contain side effects, 
and whether it should be allowed in some "do this only if you _really_ 
know what you are doing" situations to let BUG_ON do nothing.

Options:
1. BUG_ON must not be defined to do nothing
1a. side effects are allowed in the argument of BUG_ON
1b. side effects are not allowed in the argument of BUG_ON
2. BUG_ON is allowed to be defined to do nothing
2a. side effects are allowed in the argument of BUG_ON
2b. side effects are not allowed in the argument of BUG_ON

It would be good if there was a decision which of the four choices 
should become documented policy.


<--  snip  -->

My personal opinions:

IMHO, 1b doesn't make much sense, since in the case of 1. side effects 
are never a problem.

IMHO, 2b is bad since it might cause nasty heisenbugs if BUG_ON does  
nothing, and preserving the side effects is easy.

<--  snip  -->


cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: What policy for BUG_ON()?

[Arjan van de Ven]

[-- Attachment #1: Type: text/plain, Size: 933 bytes --]

On Mon, 2004-08-30 at 22:15, Adrian Bunk wrote:
> Let me try to summarize the different options regarding BUG_ON, 
> concerning whether the argument to BUG_ON might contain side effects, 
> and whether it should be allowed in some "do this only if you _really_ 
> know what you are doing" situations to let BUG_ON do nothing.
> 
> Options:
> 1. BUG_ON must not be defined to do nothing
> 1a. side effects are allowed in the argument of BUG_ON
> 1b. side effects are not allowed in the argument of BUG_ON
> 2. BUG_ON is allowed to be defined to do nothing
> 2a. side effects are allowed in the argument of BUG_ON
> 2b. side effects are not allowed in the argument of BUG_ON

since you quoted me earlier my 2 cents:
1) I would prefer BUG_ON() arguments to not have side effects; its just 
cleaner that way. (similar to assert)

2) if one wants to compiel out BUG_ON, I rather alias it to panic() than
to nothing.


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: What policy for BUG_ON()?

[Jens Axboe]

On Mon, Aug 30 2004, Arjan van de Ven wrote:
> On Mon, 2004-08-30 at 22:15, Adrian Bunk wrote:
> > Let me try to summarize the different options regarding BUG_ON, 
> > concerning whether the argument to BUG_ON might contain side effects, 
> > and whether it should be allowed in some "do this only if you _really_ 
> > know what you are doing" situations to let BUG_ON do nothing.
> > 
> > Options:
> > 1. BUG_ON must not be defined to do nothing
> > 1a. side effects are allowed in the argument of BUG_ON
> > 1b. side effects are not allowed in the argument of BUG_ON
> > 2. BUG_ON is allowed to be defined to do nothing
> > 2a. side effects are allowed in the argument of BUG_ON
> > 2b. side effects are not allowed in the argument of BUG_ON
> 
> since you quoted me earlier my 2 cents:
> 1) I would prefer BUG_ON() arguments to not have side effects; its just 
> cleaner that way. (similar to assert)
> 
> 2) if one wants to compiel out BUG_ON, I rather alias it to panic() than
> to nothing.

I agree completely with that.

-- 
Jens Axboe

Re: What policy for BUG_ON()?

[Paulo Marques]

Jens Axboe wrote:
> On Mon, Aug 30 2004, Arjan van de Ven wrote:
> 
>>On Mon, 2004-08-30 at 22:15, Adrian Bunk wrote:
>>
>>>Let me try to summarize the different options regarding BUG_ON, 
>>>concerning whether the argument to BUG_ON might contain side effects, 
>>>and whether it should be allowed in some "do this only if you _really_ 
>>>know what you are doing" situations to let BUG_ON do nothing.
>>>
>>>Options:
>>>1. BUG_ON must not be defined to do nothing
>>>1a. side effects are allowed in the argument of BUG_ON
>>>1b. side effects are not allowed in the argument of BUG_ON
>>>2. BUG_ON is allowed to be defined to do nothing
>>>2a. side effects are allowed in the argument of BUG_ON
>>>2b. side effects are not allowed in the argument of BUG_ON
>>
>>since you quoted me earlier my 2 cents:
>>1) I would prefer BUG_ON() arguments to not have side effects; its just 
>>cleaner that way. (similar to assert)
>>
>>2) if one wants to compiel out BUG_ON, I rather alias it to panic() than
>>to nothing.
> 
> 
> I agree completely with that.

This would mean that the condition would still have to be
tested which kind of defeats the purpose of removing the
BUG_ON in the first place, doesn't it?

-- 
Paulo Marques - www.grupopie.com

To err is human, but to really foul things up requires a computer.
Farmers' Almanac, 1978

Re: What policy for BUG_ON()?

[Linus Torvalds]

On Mon, 30 Aug 2004, Adrian Bunk wrote:
>
> Let me try to summarize the different options regarding BUG_ON, 
> concerning whether the argument to BUG_ON might contain side effects, 
> and whether it should be allowed in some "do this only if you _really_ 
> know what you are doing" situations to let BUG_ON do nothing.
> 
> Options:
> 1. BUG_ON must not be defined to do nothing
> 1a. side effects are allowed in the argument of BUG_ON
> 1b. side effects are not allowed in the argument of BUG_ON
> 2. BUG_ON is allowed to be defined to do nothing
> 2a. side effects are allowed in the argument of BUG_ON
> 2b. side effects are not allowed in the argument of BUG_ON
> 
> It would be good if there was a decision which of the four choices 
> should become documented policy.

I'd suggest we strongly discourage side-effects in BUG_ON(). 

That said, it might be safest to just go for 1b - we make side-effects of 
BUG_ON() be _documented_ as a bug, but just for safety, I'd suggest doing

	#define BUG_ON(x) (void)(x)

anyway, if somebody wants to compile without debugging. That will still 
make the side-effects happen if somebody has them (and if there are none, 
the compiler will not generate any code anyway).

In other words: nobody should _depend_ on the side effects, and we should 
flame people who have them. A quick grep shows this:

	mm/slab.c:      BUG_ON(spin_trylock(&cachep->spinlock));

which makes no real sense - it looks like slab really wants to use 
"spin_is_locked()".

I could make some sparse extension that warns about side effects. I 
already calculate the "side-effectiveness" of an expression for other 
reasons..

		Linus

Re: What policy for BUG_ON()?

[Adrian Bunk]

On Mon, Aug 30, 2004 at 05:25:52PM -0700, Linus Torvalds wrote:
> 
> 
> On Mon, 30 Aug 2004, Adrian Bunk wrote:
> >
> > Let me try to summarize the different options regarding BUG_ON, 
> > concerning whether the argument to BUG_ON might contain side effects, 
> > and whether it should be allowed in some "do this only if you _really_ 
> > know what you are doing" situations to let BUG_ON do nothing.
> > 
> > Options:
> > 1. BUG_ON must not be defined to do nothing
> > 1a. side effects are allowed in the argument of BUG_ON
> > 1b. side effects are not allowed in the argument of BUG_ON
> > 2. BUG_ON is allowed to be defined to do nothing
> > 2a. side effects are allowed in the argument of BUG_ON
> > 2b. side effects are not allowed in the argument of BUG_ON
> > 
> > It would be good if there was a decision which of the four choices 
> > should become documented policy.
> 
> I'd suggest we strongly discourage side-effects in BUG_ON(). 
> 
> That said, it might be safest to just go for 1b - we make side-effects of 
> BUG_ON() be _documented_ as a bug, but just for safety, I'd suggest doing
> 
> 	#define BUG_ON(x) (void)(x)
> 
> anyway, if somebody wants to compile without debugging. That will still 
> make the side-effects happen if somebody has them (and if there are none, 
> the compiler will not generate any code anyway).
>...

You say 1b but describe 2b...

The difference between 1b and 2b is that a patch to
  #define BUG_ON(x) (void)(x)
with an own option under EMBEDDED might be accepted into the kernel
with 2b, but not with 1b.

> 		Linus

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed