What policy for 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)

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

...

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------

Quoting Kyle Moffett <[email protected]>:

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 ]

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.

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------